8:29 p.m.
---
plugins/sim900.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 82 insertions(+), 9 deletions(-)
diff --git a/plugins/sim900.c b/plugins/sim900.c
index a7728cd..bf7b9ec 100644
--- a/plugins/sim900.c
+++ b/plugins/sim900.c
@@ -24,6 +24,7 @@
#endif
#include <errno.h>
+#include <string.h>
#include <stdlib.h>
#include <glib.h>
#include <gatchat.h>
@@ -60,13 +61,77 @@ static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net:
", "SMS: ",
static const char *none_prefix[] = { NULL };
+typedef enum type {
+ SIM800,
+ SIM900,
+ SIM_UNKNOWN,
+} type_t;
+
struct sim900_data {
GIOChannel *device;
GAtMux *mux;
GAtChat * dlcs[NUM_DLC];
guint frame_size;
+ type_t modem_type;
};
+static void set_modem_type (const char *type, struct ofono_modem *modem)
+{
+ struct sim900_data *data = ofono_modem_get_data(modem);
+
+ if (strcmp(type, "sim800") == 0)
+ data->modem_type = SIM800;
+ else if (strcmp(type, "sim900") == 0)
+ data->modem_type = SIM900;
+ else
+ data->modem_type = SIM_UNKNOWN;
+ DBG("modem type is %d",data->modem_type);
+}
+
+static void mux_ready_notify(GAtResult *result, gpointer user_data)
+{
+ struct ofono_modem *modem = user_data;
+ struct sim900_data *data = ofono_modem_get_data(modem);
+ struct ofono_gprs *gprs = NULL;
+ struct ofono_gprs_context *gc;
+
+ ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
+ data->dlcs[SMS_DLC]);
+
+ gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
+ if (gprs == NULL)
+ return;
+
+ gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM,
+ "atmodem", data->dlcs[GPRS_DLC]);
+ if (gc)
+ ofono_gprs_add_context(gprs, gc);
+
+
+}
+
+static void check_model(gboolean ok, GAtResult *result, gpointer user_data)
+{
+ struct ofono_modem *modem = user_data;
+ GAtResultIter iter;
+ char const *model;
+
+ DBG("");
+
+ g_at_result_iter_init(&iter, result);
+
+ while (g_at_result_iter_next(&iter, NULL)) {
+ if (!g_at_result_iter_next_unquoted_string(&iter, &model))
+ continue;
+
+ DBG("setting type %s", model);
+ if (strstr(model, "SIM800"))
+ set_modem_type("sim800", modem);
+ else if (strstr(model, "SIM900"))
+ set_modem_type("sim900", modem);
+ }
+}
+
static int sim900_probe(struct ofono_modem *modem)
{
struct sim900_data *data;
@@ -78,7 +143,6 @@ static int sim900_probe(struct ofono_modem *modem)
return -ENOMEM;
ofono_modem_set_data(modem, data);
-
return 0;
}
@@ -111,6 +175,7 @@ static GAtChat *open_device(struct ofono_modem *modem,
GHashTable *options;
device = ofono_modem_get_string(modem, key);
+
if (device == NULL)
return NULL;
@@ -232,6 +297,11 @@ static void setup_internal_mux(struct ofono_modem *modem)
goto error;
}
}
+ if (data->modem_type == SIM800) {
+ for (i = 0; i<NUM_DLC; i++) {
+ g_at_chat_register(data->dlcs[i], "SMS Ready", mux_ready_notify, FALSE,
modem, NULL);
+ }
+ }
ofono_modem_set_powered(modem, TRUE);
@@ -294,6 +364,7 @@ static int sim900_enable(struct ofono_modem *modem)
return -EINVAL;
g_at_chat_send(data->dlcs[SETUP_DLC], "ATE0", NULL, NULL, NULL, NULL);
+ g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CGMM", NULL,check_model, modem,
NULL);
/* For obtain correct sms service number */
g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CSCS=\"GSM\"", NULL,
@@ -353,18 +424,20 @@ static void sim900_post_sim(struct ofono_modem *modem)
DBG("%p", modem);
- ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
- ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
+ if (data->modem_type != SIM800) {
+ ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
+ ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
data->dlcs[SMS_DLC]);
- gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
- if (gprs == NULL)
- return;
+ gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
+ if (gprs == NULL)
+ return;
- gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
+ gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
"atmodem", data->dlcs[GPRS_DLC]);
- if (gc)
- ofono_gprs_add_context(gprs, gc);
+ if (gc)
+ ofono_gprs_add_context(gprs, gc);
+ }
}
static void sim900_post_online(struct ofono_modem *modem)
--
2.7.4
10:59 p.m.
Hi Clement,
On Tue, Nov 13, 2018 at 10:29 AM Clement Viel <vielclement(a)gmail.com> wrote:
---
plugins/sim900.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 82 insertions(+), 9 deletions(-)
diff --git a/plugins/sim900.c b/plugins/sim900.c
index a7728cd..bf7b9ec 100644
--- a/plugins/sim900.c
+++ b/plugins/sim900.c
@@ -24,6 +24,7 @@
#endif
#include <errno.h>
+#include <string.h>
#include <stdlib.h>
#include <glib.h>
#include <gatchat.h>
@@ -60,13 +61,77 @@ static char *dlc_prefixes[NUM_DLC] = { "Voice: ",
"Net: ", "SMS: ",
static const char *none_prefix[] = { NULL };
+typedef enum type {
+ SIM800,
+ SIM900,
+ SIM_UNKNOWN,
maybe SIMCOM_UNKNOWN ?
+} type_t;
+
struct sim900_data {
GIOChannel *device;
GAtMux *mux;
GAtChat * dlcs[NUM_DLC];
guint frame_size;
+ type_t modem_type;
};
+static void set_modem_type (const char *type, struct ofono_modem *modem)
+{
+ struct sim900_data *data = ofono_modem_get_data(modem);
+
+ if (strcmp(type, "sim800") == 0)
+ data->modem_type = SIM800;
+ else if (strcmp(type, "sim900") == 0)
+ data->modem_type = SIM900;
+ else
+ data->modem_type = SIM_UNKNOWN;
+ DBG("modem type is %d",data->modem_type);
+}
+
+static void mux_ready_notify(GAtResult *result, gpointer user_data)
+{
+ struct ofono_modem *modem = user_data;
+ struct sim900_data *data = ofono_modem_get_data(modem);
+ struct ofono_gprs *gprs = NULL;
+ struct ofono_gprs_context *gc;
+
+ ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
+ data->dlcs[SMS_DLC]);
+
+ gprs = ofono_gprs_create(modem, 0, "atmodem",
data->dlcs[GPRS_DLC]);
+ if (gprs == NULL)
+ return;
+
+ gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM,
+
"atmodem", data->dlcs[GPRS_DLC]);
+ if (gc)
+ ofono_gprs_add_context(gprs, gc);
+
+
+}
+
+static void check_model(gboolean ok, GAtResult *result, gpointer user_data)
+{
+ struct ofono_modem *modem = user_data;
+ GAtResultIter iter;
+ char const *model;
+
+ DBG("");
+
+ g_at_result_iter_init(&iter, result);
+
+ while (g_at_result_iter_next(&iter, NULL)) {
+ if (!g_at_result_iter_next_unquoted_string(&iter, &model))
+ continue;
+
+ DBG("setting type %s", model);
+ if (strstr(model, "SIM800"))
+ set_modem_type("sim800", modem);
+ else if (strstr(model, "SIM900"))
+ set_modem_type("sim900", modem);
+ }
+}
+
static int sim900_probe(struct ofono_modem *modem)
{
struct sim900_data *data;
@@ -78,7 +143,6 @@ static int sim900_probe(struct ofono_modem *modem)
return -ENOMEM;
ofono_modem_set_data(modem, data);
-
return 0;
}
@@ -111,6 +175,7 @@ static GAtChat *open_device(struct ofono_modem *modem,
GHashTable *options;
device = ofono_modem_get_string(modem, key);
+
if (device == NULL)
return NULL;
@@ -232,6 +297,11 @@ static void setup_internal_mux(struct ofono_modem *modem)
goto error;
}
}
+ if (data->modem_type == SIM800) {
+ for (i = 0; i<NUM_DLC; i++) {
+ g_at_chat_register(data->dlcs[i], "SMS Ready",
mux_ready_notify, FALSE, modem, NULL);
+ }
+ }
ofono_modem_set_powered(modem, TRUE);
@@ -294,6 +364,7 @@ static int sim900_enable(struct ofono_modem *modem)
return -EINVAL;
g_at_chat_send(data->dlcs[SETUP_DLC], "ATE0", NULL, NULL, NULL,
NULL);
+ g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CGMM", NULL,check_model,
modem, NULL);
/* For obtain correct sms service number */
g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CSCS=\"GSM\"",
NULL,
@@ -353,18 +424,20 @@ static void sim900_post_sim(struct ofono_modem *modem)
DBG("%p", modem);
- ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
- ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
+ if (data->modem_type != SIM800) {
+ ofono_phonebook_create(modem, 0, "atmodem",
data->dlcs[VOICE_DLC]);
+ ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
data->dlcs[SMS_DLC]);
- gprs = ofono_gprs_create(modem, 0, "atmodem",
data->dlcs[GPRS_DLC]);
- if (gprs == NULL)
- return;
+ gprs = ofono_gprs_create(modem, 0, "atmodem",
data->dlcs[GPRS_DLC]);
+ if (gprs == NULL)
+ return;
- gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
+ gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
"atmodem", data->dlcs[GPRS_DLC]);
- if (gc)
- ofono_gprs_add_context(gprs, gc);
+ if (gc)
+ ofono_gprs_add_context(gprs, gc);
+ }
}
static void sim900_post_online(struct ofono_modem *modem)
--
Regards,
Giacinto
2 a.m.
On Tue, Nov 13, 2018 at 12:59:47PM +0100, Giacinto Cifelli wrote:
Hi Clement,
On Tue, Nov 13, 2018 at 10:29 AM Clement Viel <vielclement(a)gmail.com> wrote:
>
> ---
> plugins/sim900.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 82 insertions(+), 9 deletions(-)
>
> diff --git a/plugins/sim900.c b/plugins/sim900.c
> index a7728cd..bf7b9ec 100644
> --- a/plugins/sim900.c
> +++ b/plugins/sim900.c
> @@ -24,6 +24,7 @@
> #endif
>
> #include <errno.h>
> +#include <string.h>
> #include <stdlib.h>
> #include <glib.h>
> #include <gatchat.h>
> @@ -60,13 +61,77 @@ static char *dlc_prefixes[NUM_DLC] = { "Voice: ",
"Net: ", "SMS: ",
>
> static const char *none_prefix[] = { NULL };
>
> +typedef enum type {
> + SIM800,
> + SIM900,
> + SIM_UNKNOWN,
maybe SIMCOM_UNKNOWN ?
Yes, I do agree, updating patchset.
> +} type_t;
> +
> struct sim900_data {
> GIOChannel *device;
> GAtMux *mux;
> GAtChat * dlcs[NUM_DLC];
> guint frame_size;
> + type_t modem_type;
> };
>
> +static void set_modem_type (const char *type, struct ofono_modem *modem)
> +{
> + struct sim900_data *data = ofono_modem_get_data(modem);
> +
> + if (strcmp(type, "sim800") == 0)
> + data->modem_type = SIM800;
> + else if (strcmp(type, "sim900") == 0)
> + data->modem_type = SIM900;
> + else
> + data->modem_type = SIM_UNKNOWN;
> + DBG("modem type is %d",data->modem_type);
> +}
> +
> +static void mux_ready_notify(GAtResult *result, gpointer user_data)
> +{
> + struct ofono_modem *modem = user_data;
> + struct sim900_data *data = ofono_modem_get_data(modem);
> + struct ofono_gprs *gprs = NULL;
> + struct ofono_gprs_context *gc;
> +
> + ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
> + data->dlcs[SMS_DLC]);
> +
> + gprs = ofono_gprs_create(modem, 0, "atmodem",
data->dlcs[GPRS_DLC]);
> + if (gprs == NULL)
> + return;
> +
> + gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM,
> +
"atmodem", data->dlcs[GPRS_DLC]);
> + if (gc)
> + ofono_gprs_add_context(gprs, gc);
> +
> +
> +}
> +
> +static void check_model(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> + struct ofono_modem *modem = user_data;
> + GAtResultIter iter;
> + char const *model;
> +
> + DBG("");
> +
> + g_at_result_iter_init(&iter, result);
> +
> + while (g_at_result_iter_next(&iter, NULL)) {
> + if (!g_at_result_iter_next_unquoted_string(&iter, &model))
> + continue;
> +
> + DBG("setting type %s", model);
> + if (strstr(model, "SIM800"))
> + set_modem_type("sim800", modem);
> + else if (strstr(model, "SIM900"))
> + set_modem_type("sim900", modem);
> + }
> +}
> +
> static int sim900_probe(struct ofono_modem *modem)
> {
> struct sim900_data *data;
> @@ -78,7 +143,6 @@ static int sim900_probe(struct ofono_modem *modem)
> return -ENOMEM;
>
> ofono_modem_set_data(modem, data);
> -
> return 0;
> }
>
> @@ -111,6 +175,7 @@ static GAtChat *open_device(struct ofono_modem *modem,
> GHashTable *options;
>
> device = ofono_modem_get_string(modem, key);
> +
> if (device == NULL)
> return NULL;
>
> @@ -232,6 +297,11 @@ static void setup_internal_mux(struct ofono_modem *modem)
> goto error;
> }
> }
> + if (data->modem_type == SIM800) {
> + for (i = 0; i<NUM_DLC; i++) {
> + g_at_chat_register(data->dlcs[i], "SMS Ready",
mux_ready_notify, FALSE, modem, NULL);
> + }
> + }
>
> ofono_modem_set_powered(modem, TRUE);
>
> @@ -294,6 +364,7 @@ static int sim900_enable(struct ofono_modem *modem)
> return -EINVAL;
>
> g_at_chat_send(data->dlcs[SETUP_DLC], "ATE0", NULL, NULL, NULL,
NULL);
> + g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CGMM",
NULL,check_model, modem, NULL);
>
> /* For obtain correct sms service number */
> g_at_chat_send(data->dlcs[SETUP_DLC],
"AT+CSCS=\"GSM\"", NULL,
> @@ -353,18 +424,20 @@ static void sim900_post_sim(struct ofono_modem *modem)
>
> DBG("%p", modem);
>
> - ofono_phonebook_create(modem, 0, "atmodem",
data->dlcs[VOICE_DLC]);
> - ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
> + if (data->modem_type != SIM800) {
> + ofono_phonebook_create(modem, 0, "atmodem",
data->dlcs[VOICE_DLC]);
> + ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
> data->dlcs[SMS_DLC]);
>
> - gprs = ofono_gprs_create(modem, 0, "atmodem",
data->dlcs[GPRS_DLC]);
> - if (gprs == NULL)
> - return;
> + gprs = ofono_gprs_create(modem, 0, "atmodem",
data->dlcs[GPRS_DLC]);
> + if (gprs == NULL)
> + return;
>
> - gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
> + gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
> "atmodem",
data->dlcs[GPRS_DLC]);
> - if (gc)
> - ofono_gprs_add_context(gprs, gc);
> + if (gc)
> + ofono_gprs_add_context(gprs, gc);
> + }
> }
>
> static void sim900_post_online(struct ofono_modem *modem)
> --
Regards,
Giacinto
Regards
Clement
2:04 a.m.
Hi Clement,
>> ---
>> plugins/sim900.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 82 insertions(+), 9 deletions(-)
>>
>> diff --git a/plugins/sim900.c b/plugins/sim900.c
>> index a7728cd..bf7b9ec 100644
>> --- a/plugins/sim900.c
>> +++ b/plugins/sim900.c
>> @@ -24,6 +24,7 @@
>> #endif
>>
>> #include <errno.h>
>> +#include <string.h>
>> #include <stdlib.h>
>> #include <glib.h>
>> #include <gatchat.h>
>> @@ -60,13 +61,77 @@ static char *dlc_prefixes[NUM_DLC] = { "Voice: ",
"Net: ", "SMS: ",
>>
>> static const char *none_prefix[] = { NULL };
>>
>> +typedef enum type {
>> + SIM800,
>> + SIM900,
>> + SIM_UNKNOWN,
>
> maybe SIMCOM_UNKNOWN ?
Yes, I do agree, updating patchset.
and no typedef please.
Regards
Marcel
2:11 a.m.
Hi Clement,
I already provided review for this patch. This submission addresses
none of my earlier comments...
On 13/11/2018 10:29, Clement Viel wrote:
---
plugins/sim900.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 82 insertions(+), 9 deletions(-)
diff --git a/plugins/sim900.c b/plugins/sim900.c
index a7728cd..bf7b9ec 100644
--- a/plugins/sim900.c
+++ b/plugins/sim900.c
@@ -24,6 +24,7 @@
#endif
#include <errno.h>
+#include <string.h>
#include <stdlib.h>
#include <glib.h>
#include <gatchat.h>
@@ -60,13 +61,77 @@ static char *dlc_prefixes[NUM_DLC] = { "Voice: ",
"Net: ", "SMS: ",
static const char *none_prefix[] = { NULL };
+typedef enum type {
+ SIM800,
+ SIM900,
+ SIM_UNKNOWN,
+} type_t;
+
There's no need to typedef an enumeration; type_t is not a great name,
anyway... How about: enum simcom_model? Furthermore, it makes sense
to have the 'unknown' model be value 0.
struct sim900_data {
GIOChannel *device;
GAtMux *mux;
GAtChat * dlcs[NUM_DLC];
guint frame_size;
+ type_t modem_type;
};
+static void set_modem_type (const char *type, struct ofono_modem *modem)
+{
+ struct sim900_data *data = ofono_modem_get_data(modem);
+
+ if (strcmp(type, "sim800") == 0)
+ data->modem_type = SIM800;
+ else if (strcmp(type, "sim900") == 0)
+ data->modem_type = SIM900;
+ else
+ data->modem_type = SIM_UNKNOWN;
+ DBG("modem type is %d",data->modem_type);
+}
+
See my previous review of this patch. This function is pointless.
+static void mux_ready_notify(GAtResult *result, gpointer user_data)
+{
+ struct ofono_modem *modem = user_data;
+ struct sim900_data *data = ofono_modem_get_data(modem);
+ struct ofono_gprs *gprs = NULL;
+ struct ofono_gprs_context *gc;
+
+ ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
+ data->dlcs[SMS_DLC]);
+
+ gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
+ if (gprs == NULL)
+ return;
+
+ gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM,
+ "atmodem", data->dlcs[GPRS_DLC]);
+ if (gc)
+ ofono_gprs_add_context(gprs, gc);
+
+
+}
+
+static void check_model(gboolean ok, GAtResult *result, gpointer user_data)
+{
+ struct ofono_modem *modem = user_data;
+ GAtResultIter iter;
+ char const *model;
+
+ DBG("");
+
+ g_at_result_iter_init(&iter, result);
+
+ while (g_at_result_iter_next(&iter, NULL)) {
+ if (!g_at_result_iter_next_unquoted_string(&iter, &model))
+ continue;
+
+ DBG("setting type %s", model);
+ if (strstr(model, "SIM800"))
strstr? The response begins with exactly this, as far as I can see.
+ set_modem_type("sim800", modem);
So you go from "SIM800" to "sim800" to an enumerated type... just
short
out the call to set_modem_type().
+ else if (strstr(model, "SIM900"))
+ set_modem_type("sim900", modem);
+ }
+}
+
static int sim900_probe(struct ofono_modem *modem)
{
struct sim900_data *data;
@@ -78,7 +143,6 @@ static int sim900_probe(struct ofono_modem *modem)
return -ENOMEM;
ofono_modem_set_data(modem, data);
-
Random white space change
return 0;
}
@@ -111,6 +175,7 @@ static GAtChat *open_device(struct ofono_modem *modem,
GHashTable *options;
device = ofono_modem_get_string(modem, key);
+
Random white space change.
if (device == NULL)
return NULL;
@@ -232,6 +297,11 @@ static void setup_internal_mux(struct ofono_modem *modem)
goto error;
}
}
+ if (data->modem_type == SIM800) {
+ for (i = 0; i<NUM_DLC; i++) {
+ g_at_chat_register(data->dlcs[i], "SMS Ready", mux_ready_notify, FALSE,
modem, NULL);
+ }
+ }
ofono_modem_set_powered(modem, TRUE);
@@ -294,6 +364,7 @@ static int sim900_enable(struct ofono_modem *modem)
return -EINVAL;
g_at_chat_send(data->dlcs[SETUP_DLC], "ATE0", NULL, NULL, NULL, NULL);
+ g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CGMM", NULL,check_model, modem,
NULL);
/* For obtain correct sms service number */
g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CSCS=\"GSM\"", NULL,
@@ -353,18 +424,20 @@ static void sim900_post_sim(struct ofono_modem *modem)
DBG("%p", modem);
- ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
- ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
+ if (data->modem_type != SIM800) {
Again, this is SIM900, specifically. Make the check for that. Or:
if (data->modem_type == SIM800)
return;
I think that's what you're doing here, effecively.
+ ofono_phonebook_create(modem, 0, "atmodem",
data->dlcs[VOICE_DLC]);
+ ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
data->dlcs[SMS_DLC]);
- gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
- if (gprs == NULL)
- return;
+ gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
+ if (gprs == NULL)
+ return;
- gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
+ gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
"atmodem", data->dlcs[GPRS_DLC]);
- if (gc)
- ofono_gprs_add_context(gprs, gc);
+ if (gc)
+ ofono_gprs_add_context(gprs, gc);
+ }
}
static void sim900_post_online(struct ofono_modem *modem)
/Jonas
2:28 a.m.
On Tue, Nov 13, 2018 at 04:11:07PM +0100, Jonas Bonn wrote:
Hi Jonas,
Hi Clement,
I already provided review for this patch. This submission addresses none of
my earlier comments...
On 13/11/2018 10:29, Clement Viel wrote:
>---
> plugins/sim900.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 82 insertions(+), 9 deletions(-)
>
>diff --git a/plugins/sim900.c b/plugins/sim900.c
>index a7728cd..bf7b9ec 100644
>--- a/plugins/sim900.c
>+++ b/plugins/sim900.c
>@@ -24,6 +24,7 @@
> #endif
> #include <errno.h>
>+#include <string.h>
> #include <stdlib.h>
> #include <glib.h>
> #include <gatchat.h>
>@@ -60,13 +61,77 @@ static char *dlc_prefixes[NUM_DLC] = { "Voice: ",
"Net: ", "SMS: ",
> static const char *none_prefix[] = { NULL };
>+typedef enum type {
>+ SIM800,
>+ SIM900,
>+ SIM_UNKNOWN,
>+} type_t;
>+
There's no need to typedef an enumeration; type_t is not a great name,
anyway... How about: enum simcom_model? Furthermore, it makes sense to
have the 'unknown' model be value 0.
Yeah you're right, the next patchset will fix that.
> struct sim900_data {
> GIOChannel *device;
> GAtMux *mux;
> GAtChat * dlcs[NUM_DLC];
> guint frame_size;
>+ type_t modem_type;
> };
>+static void set_modem_type (const char *type, struct ofono_modem *modem)
>+{
>+ struct sim900_data *data = ofono_modem_get_data(modem);
>+
>+ if (strcmp(type, "sim800") == 0)
>+ data->modem_type = SIM800;
>+ else if (strcmp(type, "sim900") == 0)
>+ data->modem_type = SIM900;
>+ else
>+ data->modem_type = SIM_UNKNOWN;
>+ DBG("modem type is %d",data->modem_type);
>+}
>+
See my previous review of this patch. This function is pointless.
I prefer adding a function than adding 10 lines to antoher. To my mind it's clearer.
>+static void mux_ready_notify(GAtResult *result, gpointer
user_data)
>+{
>+ struct ofono_modem *modem = user_data;
>+ struct sim900_data *data = ofono_modem_get_data(modem);
>+ struct ofono_gprs *gprs = NULL;
>+ struct ofono_gprs_context *gc;
>+
>+ ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
>+ data->dlcs[SMS_DLC]);
>+
>+ gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
>+ if (gprs == NULL)
>+ return;
>+
>+ gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM,
>+ "atmodem", data->dlcs[GPRS_DLC]);
>+ if (gc)
>+ ofono_gprs_add_context(gprs, gc);
>+
>+
>+}
>+
>+static void check_model(gboolean ok, GAtResult *result, gpointer user_data)
>+{
>+ struct ofono_modem *modem = user_data;
>+ GAtResultIter iter;
>+ char const *model;
>+
>+ DBG("");
>+
>+ g_at_result_iter_init(&iter, result);
>+
>+ while (g_at_result_iter_next(&iter, NULL)) {
>+ if (!g_at_result_iter_next_unquoted_string(&iter, &model))
>+ continue;
>+
>+ DBG("setting type %s", model);
>+ if (strstr(model, "SIM800"))
strstr? The response begins with exactly this, as far as I can see.
Nope, CGMM does not reply exactly "SIM800" or "SIM900". Because there
is no need to differentiate revision of modems for now, I prefer using strstr.
>+ set_modem_type("sim800", modem);
So you go from "SIM800" to "sim800" to an enumerated type... just
short out
the call to set_modem_type().
I find it simplier to work with enumerated than with strings.
>+ else if (strstr(model, "SIM900"))
>+ set_modem_type("sim900", modem);
>+ }
>+}
>+
> static int sim900_probe(struct ofono_modem *modem)
> {
> struct sim900_data *data;
>@@ -78,7 +143,6 @@ static int sim900_probe(struct ofono_modem *modem)
> return -ENOMEM;
> ofono_modem_set_data(modem, data);
>-
Random white space change
Good catch !
> return 0;
> }
>@@ -111,6 +175,7 @@ static GAtChat *open_device(struct ofono_modem *modem,
> GHashTable *options;
> device = ofono_modem_get_string(modem, key);
>+
Random white space change.
> if (device == NULL)
> return NULL;
>@@ -232,6 +297,11 @@ static void setup_internal_mux(struct ofono_modem *modem)
> goto error;
> }
> }
>+ if (data->modem_type == SIM800) {
>+ for (i = 0; i<NUM_DLC; i++) {
>+ g_at_chat_register(data->dlcs[i], "SMS Ready", mux_ready_notify,
FALSE, modem, NULL);
>+ }
>+ }
> ofono_modem_set_powered(modem, TRUE);
>@@ -294,6 +364,7 @@ static int sim900_enable(struct ofono_modem *modem)
> return -EINVAL;
> g_at_chat_send(data->dlcs[SETUP_DLC], "ATE0", NULL, NULL, NULL,
NULL);
>+ g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CGMM", NULL,check_model,
modem, NULL);
> /* For obtain correct sms service number */
> g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CSCS=\"GSM\"",
NULL,
>@@ -353,18 +424,20 @@ static void sim900_post_sim(struct ofono_modem *modem)
> DBG("%p", modem);
>- ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
>- ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
>+ if (data->modem_type != SIM800) {
Again, this is SIM900, specifically. Make the check for that. Or:
if (data->modem_type == SIM800)
return;
I think that's what you're doing here, effecively.
Good catch ! Because this check allows a SIMCOM_UNKNOWN modem to execute this code.
I'll check for SIM900.
>+ ofono_phonebook_create(modem, 0, "atmodem",
data->dlcs[VOICE_DLC]);
>+ ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
> data->dlcs[SMS_DLC]);
>- gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
>- if (gprs == NULL)
>- return;
>+ gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
>+ if (gprs == NULL)
>+ return;
>- gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
>+ gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
> "atmodem", data->dlcs[GPRS_DLC]);
>- if (gc)
>- ofono_gprs_add_context(gprs, gc);
>+ if (gc)
>+ ofono_gprs_add_context(gprs, gc);
>+ }
> }
> static void sim900_post_online(struct ofono_modem *modem)
>
/Jonas
1405
days inactive
1405
days old
5 comments
4 participants
participants (4)
-
Clement Viel -
Giacinto Cifelli -
Jonas Bonn -
Marcel Holtmann