Hi Denis,
Thanks for the quick review !
On Thu, Nov 01, 2018 at 09:53:58AM -0500, Denis Kenzior wrote:
> Hi Clement,
>
> On 10/15/2018 12:27 PM, Clement Viel wrote:
> >---
> > plugins/sim900.c | 89
> > ++++++++++++++++++++++++++++++++++++++++++++++++++------
> > 1 file changed, 80 insertions(+), 9 deletions(-)
> >
> >diff --git a/plugins/sim900.c b/plugins/sim900.c
> >index a7728cd..9f3f334 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,59 @@ 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 (struct ofono_modem *modem)
> >+{
> >+ struct sim900_data *data = ofono_modem_get_data(modem);
> >+ const char *path = ofono_modem_get_path(modem);
> >+
> >+ if (strstr(path, "sim800"))
> >+ data->modem_type = SIM800;
> >+ else if (strstr(path, "sim900"))
> >+ data->modem_type = SIM900;
> >+ else
> >+ data->modem_type = SIM_UNKNOWN;
> >+
>
> You can't really rely on the modem path for this. Either query the model
> directly or set the type based on the driver selected.
>
> And no empty lines at the end of the function please
I'll go with the CGMM way.
>
> >+}
> >+
> >+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;
> >+ static int notified = 0;
> >+
> >+ if (!notified) {
> >+ 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);
> >+ }
> >+
>
> The indentation and stule is all wrong here.
>
> >+}
> >+
> > static int sim900_probe(struct ofono_modem *modem)
> > {
> > struct sim900_data *data;
> >@@ -79,6 +126,7 @@ static int sim900_probe(struct ofono_modem *modem)
> > ofono_modem_set_data(modem, data);
> >+ set_modem_type(modem);
> > return 0;
> > }
> >@@ -111,6 +159,7 @@ static GAtChat *open_device(struct ofono_modem *modem,
> > GHashTable *options;
> > device = ofono_modem_get_string(modem, key);
> >+
>
> This change is spurious and should not be part of this commit
>
> > if (device == NULL)
> > return NULL;
> >@@ -232,6 +281,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);
> >+ }
> >+ }
>
> Why do you want this notification on all ports? Isn't one enough?
These URC's are sent on all channels so I am catching them all to ensure all
channels are now ready.
> > ofono_modem_set_powered(modem, TRUE);
> >@@ -353,18 +407,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);
> >+ }
>
> Som SIM800 doesn't get any of these atoms?
SIM800 get these atoms by antoher way : the function setup_internal_mux
registers the callback for SMS_READY URC. This is the callback that will call
these atoms.
The Sim800 must respect this order when using these atoms else all the
interfaces won't be up even if the AT commands returned OK.
>
> > }
> > static void sim900_post_online(struct ofono_modem *modem)
> >@@ -391,9 +447,24 @@ static struct ofono_modem_driver sim900_driver = {
> > .post_online = sim900_post_online,
> > };
> >+
> >+static struct ofono_modem_driver sim800_driver = {
> >+ .name = "sim800",
> >+ .probe = sim900_probe,
> >+ .remove = sim900_remove,
> >+ .enable = sim900_enable,
> >+ .disable = sim900_disable,
> >+ .pre_sim = sim900_pre_sim,
> >+ .post_sim = sim900_post_sim,
> >+ .post_online = sim900_post_online,
> >+};
> >+
> > static int sim900_init(void)
> > {
> >- return ofono_modem_driver_register(&sim900_driver);
> >+ int ret = 0;
> >+ ret = ofono_modem_driver_register(&sim800_driver);
> >+ ret += ofono_modem_driver_register(&sim900_driver);
> >+ return ret;
>
> This doesn't really work this way. The init function should either register
> all drivers successfully or fail.
>
> > }
> > static void sim900_exit(void)
> >
>
> Regards,
> -Denis
I'll add those remarks on the new patchset ( and i hope this one will be the
one)!
Regards
Clem
_______________________________________________
ofono mailing list
[email protected]
https://lists.ofono.org/mailman/listinfo/ofono
