* [PATCH 1/3] sim800: add sim800 support.
@ 2018-11-13 9:29 Clement Viel
2018-11-13 11:59 ` Giacinto Cifelli
2018-11-13 15:11 ` Jonas Bonn
0 siblings, 2 replies; 11+ messages in thread
From: Clement Viel @ 2018-11-13 9:29 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 4466 bytes --]
---
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 1/3] sim800: add sim800 support.
2018-11-13 9:29 [PATCH 1/3] sim800: add sim800 support Clement Viel
@ 2018-11-13 11:59 ` Giacinto Cifelli
2018-11-13 15:00 ` Clement Viel
2018-11-13 15:11 ` Jonas Bonn
1 sibling, 1 reply; 11+ messages in thread
From: Giacinto Cifelli @ 2018-11-13 11:59 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 5808 bytes --]
Hi Clement,
On Tue, Nov 13, 2018 at 10:29 AM Clement Viel <vielclement@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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/3] sim800: add sim800 support.
2018-11-13 11:59 ` Giacinto Cifelli
@ 2018-11-13 15:00 ` Clement Viel
2018-11-13 15:04 ` Marcel Holtmann
0 siblings, 1 reply; 11+ messages in thread
From: Clement Viel @ 2018-11-13 15:00 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 6276 bytes --]
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@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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/3] sim800: add sim800 support.
2018-11-13 15:00 ` Clement Viel
@ 2018-11-13 15:04 ` Marcel Holtmann
0 siblings, 0 replies; 11+ messages in thread
From: Marcel Holtmann @ 2018-11-13 15:04 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 851 bytes --]
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] sim800: add sim800 support.
2018-11-13 9:29 [PATCH 1/3] sim800: add sim800 support Clement Viel
2018-11-13 11:59 ` Giacinto Cifelli
@ 2018-11-13 15:11 ` Jonas Bonn
2018-11-13 15:28 ` Clement Viel
1 sibling, 1 reply; 11+ messages in thread
From: Jonas Bonn @ 2018-11-13 15:11 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 5696 bytes --]
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/3] sim800: add sim800 support.
2018-11-13 15:11 ` Jonas Bonn
@ 2018-11-13 15:28 ` Clement Viel
0 siblings, 0 replies; 11+ messages in thread
From: Clement Viel @ 2018-11-13 15:28 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 6384 bytes --]
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] sim800: add sim800 support.
@ 2018-11-13 15:33 Clement Viel
2018-11-13 16:53 ` Jonas Bonn
0 siblings, 1 reply; 11+ messages in thread
From: Clement Viel @ 2018-11-13 15:33 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 4460 bytes --]
---
plugins/sim900.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 82 insertions(+), 9 deletions(-)
diff --git a/plugins/sim900.c b/plugins/sim900.c
index a7728cd..f280e5e 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 };
+enum type {
+ SIM800,
+ SIM900,
+ SIMCOM_UNKNOWN,
+};
+
struct sim900_data {
GIOChannel *device;
GAtMux *mux;
GAtChat * dlcs[NUM_DLC];
guint frame_size;
+ enum type 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 = SIMCOM_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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 1/3] sim800: add sim800 support.
2018-11-13 15:33 Clement Viel
@ 2018-11-13 16:53 ` Jonas Bonn
2018-11-13 17:11 ` Clement Viel
0 siblings, 1 reply; 11+ messages in thread
From: Jonas Bonn @ 2018-11-13 16:53 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 5463 bytes --]
Hi,
On 13/11/2018 16:33, 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..f280e5e 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 };
>
> +enum type {
> + SIM800,
> + SIM900,
> + SIMCOM_UNKNOWN,
> +};
> +
Makes more sense to have UNKNOWN be the first entry in the enumeration.
> struct sim900_data {
> GIOChannel *device;
> GAtMux *mux;
> GAtChat * dlcs[NUM_DLC];
> guint frame_size;
> + enum type 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 = SIMCOM_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;
The documentation doesn't indicate that the model type is given as an
arbitrary string at a random location in the response. It's pretty
specific about the SIM800/SIM900 token appearing as the first element of
the response. Is the documentation not correct on this point?
> +
> + DBG("setting type %s", model);
> + if (strstr(model, "SIM800"))
> + set_modem_type("sim800", modem);
Instead of doing this dance with strings, just directly set the type:
data->modem_type = SIM800;
> + 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);
> -
Whitespace.
> return 0;
> }
>
> @@ -111,6 +175,7 @@ static GAtChat *open_device(struct ofono_modem *modem,
> GHashTable *options;
>
> device = ofono_modem_get_string(modem, key);
> +
Ditto.
> 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);
> + }
All this is duplicated in mux_ready_notify... maybe move it a function
of its own...?
> }
>
> static void sim900_post_online(struct ofono_modem *modem)
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/3] sim800: add sim800 support.
2018-11-13 16:53 ` Jonas Bonn
@ 2018-11-13 17:11 ` Clement Viel
0 siblings, 0 replies; 11+ messages in thread
From: Clement Viel @ 2018-11-13 17:11 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 5966 bytes --]
On Tue, Nov 13, 2018 at 05:53:56PM +0100, Jonas Bonn wrote:
> Hi,
>
> On 13/11/2018 16:33, 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..f280e5e 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 };
> >+enum type {
> >+ SIM800,
> >+ SIM900,
> >+ SIMCOM_UNKNOWN,
> >+};
> >+
>
> Makes more sense to have UNKNOWN be the first entry in the enumeration.
You're right !
>
> > struct sim900_data {
> > GIOChannel *device;
> > GAtMux *mux;
> > GAtChat * dlcs[NUM_DLC];
> > guint frame_size;
> >+ enum type 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 = SIMCOM_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;
>
> The documentation doesn't indicate that the model type is given as an
> arbitrary string at a random location in the response. It's pretty specific
> about the SIM800/SIM900 token appearing as the first element of the
> response. Is the documentation not correct on this point?
>
> >+
> >+ DBG("setting type %s", model);
> >+ if (strstr(model, "SIM800"))
> >+ set_modem_type("sim800", modem);
>
> Instead of doing this dance with strings, just directly set the type:
>
> data->modem_type = SIM800;
>
>
Right again.
> >+ 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);
> >-
>
> Whitespace.
>
> > return 0;
> > }
> >@@ -111,6 +175,7 @@ static GAtChat *open_device(struct ofono_modem *modem,
> > GHashTable *options;
> > device = ofono_modem_get_string(modem, key);
> >+
>
> Ditto.
>
> > 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);
> >+ }
>
> All this is duplicated in mux_ready_notify... maybe move it a function of
> its own...?
this is duplicated because the ofono atoms cannot be created at the same point for the two modems :
for sim800, we must wait for an URC stating that channels have been created whereas sim900 is much straightforward.
That's why i've splitted : one in post_sim (sim900) another in mux_ready_notify (sim800).
>
> > }
> > static void sim900_post_online(struct ofono_modem *modem)
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] sim800: add sim800 support
@ 2018-11-13 17:19 Clement Viel
2018-11-14 20:34 ` Denis Kenzior
0 siblings, 1 reply; 11+ messages in thread
From: Clement Viel @ 2018-11-13 17:19 UTC (permalink / raw)
To: ofono
[-- Attachment #1: Type: text/plain, Size: 4012 bytes --]
---
plugins/sim900.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 69 insertions(+), 8 deletions(-)
diff --git a/plugins/sim900.c b/plugins/sim900.c
index a7728cd..ace02a5 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,64 @@ static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "SMS: ",
static const char *none_prefix[] = { NULL };
+enum type {
+ SIMCOM_UNKNOWN,
+ SIM800,
+ SIM900,
+};
+
struct sim900_data {
GIOChannel *device;
GAtMux *mux;
GAtChat * dlcs[NUM_DLC];
guint frame_size;
+ enum type 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;
+ struct sim900_data *data = ofono_modem_get_data(modem);
+ 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"))
+ data->modem_type = SIM800;
+ else if (strstr(model, "SIM900"))
+ data->modem_type = SIM800;
+ else
+ data->modem_type = SIMCOM_UNKNOWN;
+ }
+}
+
static int sim900_probe(struct ofono_modem *modem)
{
struct sim900_data *data;
@@ -111,6 +163,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 +285,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 +352,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 +412,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 == 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)
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-11-14 20:34 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-13 9:29 [PATCH 1/3] sim800: add sim800 support Clement Viel
2018-11-13 11:59 ` Giacinto Cifelli
2018-11-13 15:00 ` Clement Viel
2018-11-13 15:04 ` Marcel Holtmann
2018-11-13 15:11 ` Jonas Bonn
2018-11-13 15:28 ` Clement Viel
-- strict thread matches above, loose matches on Subject: below --
2018-11-13 15:33 Clement Viel
2018-11-13 16:53 ` Jonas Bonn
2018-11-13 17:11 ` Clement Viel
2018-11-13 17:19 Clement Viel
2018-11-14 20:34 ` Denis Kenzior
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.