Re: [PATCH 1/3] sim800: add sim800 support.

[Clement Viel <vielclement@gmail.com>]

[-- 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