Skip to contentSkip to conversation

sim800: add support for sim800 modem.

PATCH In review Clement Viel 3 participants 8 messages Nov 7, 2018

lore
Share & tools
lore

Clement Viel

author
#0 lore raw

PATCH 1/3 sim800: add support for sim800 modem.

---
 plugins/sim900.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 98 insertions(+), 10 deletions(-)
plugins/sim900.c +98 -10
index a7728cd..c12576d 100644
@@ -24,6 +24,7 @@
 #endif
 
 #include <errno.h>
+#include <string.h>
 #include <stdlib.h>
 #include <glib.h>
 #include <gatchat.h>

@@ -60,13 +61,73 @@ 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;
+
+	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;
+
+		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 +139,6 @@ static int sim900_probe(struct ofono_modem *modem)
 		return -ENOMEM;
 
 	ofono_modem_set_data(modem, data);
-
 	return 0;
 }
 

@@ -111,6 +171,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 +293,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 +360,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 +420,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)

@@ -391,9 +460,28 @@ 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);
+	if (!ret)
+		return ret;
+	ret = ofono_modem_driver_register(&sim900_driver);
+	if (!ret)
+		return ret;
+	return ret;
 }
 
 static void sim900_exit(void)
Signature · 1 line
2.7.4
author
#1 lore raw

PATCH 2/3 sim800: add documentation.

---
 doc/sim800-modem.txt | 7 +++++++
 1 file changed, 7 insertions(+)
 create mode 100644 doc/sim800-modem.txt
doc/sim800-modem.txt new file +7 -0
new file mode 100644
index 0000000..a299e97
@@ -0,0 +1,7 @@
+SIM800 modem usage
+===================
+
+To enable SIM800 module support you need to put the following
+udev rule into appropriate file in /{etc,lib}/udev/rules.d:
+
+KERNEL=="ttyS0", ENV{OFONO_DRIVER}="sim800"
Signature · 1 line
2.7.4
#2 lore raw

Re: [PATCH 2/3] sim800: add documentation.

Hi Clement,

Quoted 12 lines · Clement Viel
---
  doc/sim800-modem.txt | 7 +++++++
  1 file changed, 7 insertions(+)

create mode 100644 doc/sim800-modem.txt

doc/sim800-modem.txt new file +0 -0
new file mode 100644
index 0000000..a299e97

doc/sim800-modem.txt line 7

Quoted 7 lines
doc/sim800-modem.txt new file +7 -0
@@ -0,0 +1,7 @@
+SIM800 modem usage
+===================
+
+To enable SIM800 module support you need to put the following
+udev rule into appropriate file in /{etc,lib}/udev/rules.d:
+
+KERNEL=="ttyS0", ENV{OFONO_DRIVER}="sim800"

Confused by this. Isn't the sim900 the serial modem and sim800 the USB model? The above rule is for a strictly "serial" modem.

/Jonas

#3 lore raw

Re: [PATCH 2/3] sim800: add documentation.

Quoted 2 lines

Confused by this. Isn't the sim900 the serial modem and sim800 the USB model? The above rule is for a strictly "serial" modem.

Nope, the SIM800 is a serial modem - it's, as far as I can tell, the next generation after SIM900 or something along those lines. It also has a USB port, IIRC, but I never had a chance to use that.

-Arsenijs

07.11.2018, 15:50, "Jonas Bonn" <[email protected]>:

Quoted 30 lines

Hi Clement,

Quoted 5 lines · Clement Viel

 ---
   doc/sim800-modem.txt | 7 +++++++

   1 file changed, 7 insertions(+)

   create mode 100644 doc/sim800-modem.txt

Quoted 13 lines

 diff --git a/doc/sim800-modem.txt b/doc/sim800-modem.txt new file mode 100644
 index 0000000..a299e97
 --- /dev/null
 +++ b/doc/sim800-modem.txt
 @@ -0,0 +1,7 @@
 +SIM800 modem usage
 +===================
 +
 +To enable SIM800 module support you need to put the following +udev rule into appropriate file in /{etc,lib}/udev/rules.d:
 +
 +KERNEL=="ttyS0", ENV{OFONO_DRIVER}="sim800"

Confused by this. Isn't the sim900 the serial modem and sim800 the USB model? The above rule is for a strictly "serial" modem.

/Jonas
_______________________________________________
ofono mailing list
ofono(a)ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

author
#4 lore raw

PATCH 3/3 sim800: add udev detection.

In reply to Clement Viel
---
 plugins/udevng.c | 92 +++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 68 insertions(+), 24 deletions(-)
plugins/udevng.c +68 -24
index ff6e1fc..ee6f9e8 100644
@@ -711,8 +711,56 @@ static gboolean setup_telitqmi(struct modem_info *modem)
 	return TRUE;
 }
 
-/* TODO: Not used as we have no simcom driver */
-static gboolean setup_simcom(struct modem_info *modem)
+
+static gboolean setup_sim800(struct modem_info *modem)
+{
+	const char *mdm = NULL, *aux = NULL, *gps = NULL, *diag = NULL;
+	GSList *list;
+
+	DBG("%s", modem->syspath);
+
+	for (list = modem->devices; list; list = list->next) {
+		struct device_info *info = list->data;
+
+		DBG("%s %s %s %s", info->devnode, info->interface,
+						info->number, info->label);
+
+		if (g_strcmp0(info->label, "aux") == 0) {
+			DBG("setting aux as info->devnode");
+			aux = info->devnode;
+			if (mdm != NULL)
+				break;
+		} else if (g_strcmp0(info->label, "modem") == 0) {
+			mdm = info->devnode;
+			if (aux != NULL)
+				break;
+		} else if (g_strcmp0(info->interface, "255/0/0") == 0) {
+			if (g_strcmp0(info->number, "00") == 0)
+				mdm = info->devnode;
+			else if (g_strcmp0(info->number, "01") == 0)
+				gps = info->devnode;
+			else if (g_strcmp0(info->number, "02") == 0)
+				aux = info->devnode;
+			else if (g_strcmp0(info->number, "03") == 0)
+				mdm = info->devnode;
+		}
+	}
+	DBG("modem=%s aux=%s gps=%s diag=%s", mdm, aux, gps, diag);
+
+	if (mdm == NULL) {
+		DBG("modem did not set up");
+		return FALSE;
+	}
+
+	ofono_modem_set_string(modem->modem, "Device", mdm);
+	ofono_modem_set_string(modem->modem, "Modem", mdm);
+
+	return TRUE;
+}
+
+
+
+static gboolean setup_sim900(struct modem_info *modem)
 {
 	const char *mdm = NULL, *aux = NULL, *gps = NULL, *diag = NULL;
 	GSList *list;

@@ -962,6 +1010,8 @@ static gboolean setup_mbim(struct modem_info *modem)
 	ofono_modem_set_string(modem->modem, "Device", ctl);
 	ofono_modem_set_string(modem->modem, "NetworkInterface", net);
 	ofono_modem_set_string(modem->modem, "DescriptorFile", descriptors);
+	ofono_modem_set_string(modem->modem, "Vendor", modem->vendor);
+	ofono_modem_set_string(modem->modem, "Model", modem->model);
 
 	return TRUE;
 }

@@ -1193,22 +1243,12 @@ static gboolean setup_xmm7xxx(struct modem_info *modem)
 				info->interface, info->number, info->label,
 				info->sysattr, info->subsystem);
 
-		if (g_strcmp0(modem->model,"095a") == 0) {
-			if (g_strcmp0(info->subsystem, "tty") == 0) {
-				if (g_strcmp0(info->number, "00") == 0)
-					mdm = info->devnode;
-			} else if (g_strcmp0(info->subsystem, "net") == 0) {
-				if (g_strcmp0(info->number, "06") == 0)
-					net = info->devnode;
-			}
-		} else {
-			if (g_strcmp0(info->subsystem, "tty") == 0) {
-				if (g_strcmp0(info->number, "02") == 0)
-					mdm = info->devnode;
-			} else if (g_strcmp0(info->subsystem, "net") == 0) {
-				if (g_strcmp0(info->number, "00") == 0)
-					net = info->devnode;
-			}
+		if (g_strcmp0(info->subsystem, "tty") == 0) {
+			if (g_strcmp0(info->number, "02") == 0)
+				mdm = info->devnode;
+		} else if (g_strcmp0(info->subsystem, "net") == 0) {
+			if (g_strcmp0(info->number, "00") == 0)
+				net = info->devnode;
 		}
 	}
 

@@ -1290,7 +1330,8 @@ static struct {
 	{ "nokia",	setup_nokia	},
 	{ "telit",	setup_telit,	"device/interface"	},
 	{ "telitqmi",	setup_telitqmi	},
-	{ "simcom",	setup_simcom	},
+	{ "sim800",	setup_sim800	},
+	{ "sim900",	setup_sim900	},
 	{ "sim7100",	setup_sim7100	},
 	{ "zte",	setup_zte	},
 	{ "icera",	setup_icera	},

@@ -1308,7 +1349,9 @@ static struct {
 	{ "calypso",	setup_serial_modem	},
 	{ "cinterion",	setup_serial_modem	},
 	{ "nokiacdma",	setup_serial_modem	},
+	{ "sim800",	setup_serial_modem	},
 	{ "sim900",	setup_serial_modem	},
+	{ "sim800",	setup_serial_modem	},
 	{ "wavecom",	setup_wavecom		},
 	{ "tc65",	setup_tc65		},
 	{ "ehs6",	setup_ehs6		},

@@ -1471,7 +1514,7 @@ static void add_serial_device(struct udev_device *dev)
 	const char *subsystem;
 	struct udev_device* mdev;
 	const char* driver;
-
+	DBG("adding %s interface", udev_device_get_devpath(dev));
 	mdev = get_serial_modem_device(dev);
 	if (!mdev) {
 		DBG("Device is missing required OFONO_DRIVER property");

@@ -1663,7 +1706,8 @@ static struct {
 	{ "alcatel",	"option",	"1bbb", "0017"	},
 	{ "novatel",	"option",	"1410"		},
 	{ "zte",	"option",	"19d2"		},
-	{ "simcom",	"option",	"05c6", "9000"	},
+	{ "sim800",	"option",	"05c6", "9000"	},
+	{ "sim900",	"option",	"05c6", "9000"	},
 	{ "sim7100",	"option",	"1e0e", "9001"	},
 	{ "telit",	"usbserial",	"1bc7"		},
 	{ "telit",	"option",	"1bc7"		},

@@ -1693,7 +1737,6 @@ static struct {
 	{ "xmm7xxx",	"cdc_ncm",	"8087"		},
 	{ }
 };
-
 static void check_usb_device(struct udev_device *device)
 {
 	struct udev_device *usb_device;

@@ -1722,9 +1765,10 @@ static void check_usb_device(struct udev_device *device)
 			udev_device_get_parent_with_subsystem_devtype(
 				device, "usb", "usb_interface");
 
-		if (usb_interface)
+		if (usb_interface) {
 			driver = udev_device_get_property_value(
 					usb_interface, "OFONO_DRIVER");
+		}
 	}
 
 	if (driver == NULL) {

@@ -1801,7 +1845,7 @@ static gboolean create_modem(gpointer key, gpointer value, gpointer user_data)
 	struct modem_info *modem = value;
 	const char *syspath = key;
 	unsigned int i;
-
+	DBG("");
 	if (modem->modem != NULL)
 		return FALSE;
Signature · 1 line
2.7.4
#5 lore raw

Re: [PATCH 3/3] sim800: add udev detection.

Hi Clement,

Quoted 40 lines · Clement Viel
---
  plugins/udevng.c | 92 +++++++++++++++++++++++++++++++++++++++++---------------
  1 file changed, 68 insertions(+), 24 deletions(-)
plugins/udevng.c +26 -2
index ff6e1fc..ee6f9e8 100644
@@ -711,8 +711,56 @@ static gboolean setup_telitqmi(struct modem_info *modem)
  	return TRUE;
  }
 
-/* TODO: Not used as we have no simcom driver */
-static gboolean setup_simcom(struct modem_info *modem)
+
+static gboolean setup_sim800(struct modem_info *modem)
+{
+	const char *mdm = NULL, *aux = NULL, *gps = NULL, *diag = NULL;
+	GSList *list;
+
+	DBG("%s", modem->syspath);
+
+	for (list = modem->devices; list; list = list->next) {
+		struct device_info *info = list->data;
+
+		DBG("%s %s %s %s", info->devnode, info->interface,
+						info->number, info->label);
+
+		if (g_strcmp0(info->label, "aux") == 0) {
+			DBG("setting aux as info->devnode");
+			aux = info->devnode;
+			if (mdm != NULL)
+				break;
+		} else if (g_strcmp0(info->label, "modem") == 0) {
+			mdm = info->devnode;
+			if (aux != NULL)
+				break;
+		} else if (g_strcmp0(info->interface, "255/0/0") == 0) {
+			if (g_strcmp0(info->number, "00") == 0)
+				mdm = info->devnode;

plugins/udevng.c static gboolean setup_telitqmi(struct modem_info *modem) · line 749

Quoted 10 lines
plugins/udevng.c +10 -0
+			else if (g_strcmp0(info->number, "01") == 0)
+				gps = info->devnode;
+			else if (g_strcmp0(info->number, "02") == 0)
+				aux = info->devnode;
+			else if (g_strcmp0(info->number, "03") == 0)
+				mdm = info->devnode;
+		}
+	}
+	DBG("modem=%s aux=%s gps=%s diag=%s", mdm, aux, gps, diag);
+

gps is unused.

plugins/udevng.c static gboolean setup_telitqmi(struct modem_info *modem) · line 755

Quoted 6 lines
plugins/udevng.c +6 -0
+	if (mdm == NULL) {
+		DBG("modem did not set up");
+		return FALSE;
+	}
+
+	ofono_modem_set_string(modem->modem, "Device", mdm);

You probably mean 'aux' here.

Quoted 12 lines
plugins/udevng.c +8 -0
+	ofono_modem_set_string(modem->modem, "Modem", mdm);
+
+	return TRUE;
+}
+
+
+
+static gboolean setup_sim900(struct modem_info *modem)
  {
  	const char *mdm = NULL, *aux = NULL, *gps = NULL, *diag = NULL;
  	GSList *list;

plugins/udevng.c static gboolean setup_mbim(struct modem_info *modem) · line 1014

Quoted 5 lines
plugins/udevng.c +2 -0
@@ -962,6 +1010,8 @@ static gboolean setup_mbim(struct modem_info *modem)
  	ofono_modem_set_string(modem->modem, "Device", ctl);
  	ofono_modem_set_string(modem->modem, "NetworkInterface", net);
  	ofono_modem_set_string(modem->modem, "DescriptorFile", descriptors);
+	ofono_modem_set_string(modem->modem, "Vendor", modem->vendor);
+	ofono_modem_set_string(modem->modem, "Model", modem->model);

I don't see that these strings are being used anywhere in the modem driver.

Quoted 22 lines
plugins/udevng.c +0 -15
  	return TRUE;
  }

@@ -1193,22 +1243,12 @@ static gboolean setup_xmm7xxx(struct modem_info *modem)
  				info->interface, info->number, info->label,
  				info->sysattr, info->subsystem);
 
-		if (g_strcmp0(modem->model,"095a") == 0) {
-			if (g_strcmp0(info->subsystem, "tty") == 0) {
-				if (g_strcmp0(info->number, "00") == 0)
-					mdm = info->devnode;
-			} else if (g_strcmp0(info->subsystem, "net") == 0) {
-				if (g_strcmp0(info->number, "06") == 0)
-					net = info->devnode;
-			}
-		} else {
-			if (g_strcmp0(info->subsystem, "tty") == 0) {
-				if (g_strcmp0(info->number, "02") == 0)
-					mdm = info->devnode;
-			} else if (g_strcmp0(info->subsystem, "net") == 0) {
-				if (g_strcmp0(info->number, "00") == 0)
-					net = info->devnode;

plugins/udevng.c static gboolean setup_xmm7xxx(struct modem_info *modem) · line 1254

Quoted 10 lines
plugins/udevng.c +6 -1
-			}
+		if (g_strcmp0(info->subsystem, "tty") == 0) {
+			if (g_strcmp0(info->number, "02") == 0)
+				mdm = info->devnode;
+		} else if (g_strcmp0(info->subsystem, "net") == 0) {
+			if (g_strcmp0(info->number, "00") == 0)
+				net = info->devnode;
  		}
  	}

This change doesn't look like it belongs to this patch.

plugins/udevng.c static struct { · line 1334

Quoted 7 lines
plugins/udevng.c +2 -1
@@ -1290,7 +1330,8 @@ static struct {
  	{ "nokia",	setup_nokia	},
  	{ "telit",	setup_telit,	"device/interface"	},
  	{ "telitqmi",	setup_telitqmi	},
-	{ "simcom",	setup_simcom	},
+	{ "sim800",	setup_sim800	},
+	{ "sim900",	setup_sim900	},

Is the sim900 not a serial modem? These setup routines are really only for USB devices.

plugins/udevng.c static struct { · line 1354

Quoted 10 lines
plugins/udevng.c +2 -0
  	{ "sim7100",	setup_sim7100	},
  	{ "zte",	setup_zte	},
  	{ "icera",	setup_icera	},

@@ -1308,7 +1349,9 @@ static struct {
  	{ "calypso",	setup_serial_modem	},
  	{ "cinterion",	setup_serial_modem	},
  	{ "nokiacdma",	setup_serial_modem	},
+	{ "sim800",	setup_serial_modem	},
  	{ "sim900",	setup_serial_modem	},
+	{ "sim800",	setup_serial_modem	},

You've added the same line twice. Furthermore, the sim800 is a USB modem (???) so it should not be set up in this way.

Quoted 13 lines
plugins/udevng.c +1 -1
  	{ "wavecom",	setup_wavecom		},
  	{ "tc65",	setup_tc65		},
  	{ "ehs6",	setup_ehs6		},

@@ -1471,7 +1514,7 @@ static void add_serial_device(struct udev_device *dev)
  	const char *subsystem;
  	struct udev_device* mdev;
  	const char* driver;
-
+	DBG("adding %s interface", udev_device_get_devpath(dev));
  	mdev = get_serial_modem_device(dev);
  	if (!mdev) {
  		DBG("Device is missing required OFONO_DRIVER property");

plugins/udevng.c static struct { · line 1710

Quoted 6 lines
plugins/udevng.c +2 -1
@@ -1663,7 +1706,8 @@ static struct {
  	{ "alcatel",	"option",	"1bbb", "0017"	},
  	{ "novatel",	"option",	"1410"		},
  	{ "zte",	"option",	"19d2"		},
-	{ "simcom",	"option",	"05c6", "9000"	},
+	{ "sim800",	"option",	"05c6", "9000"	}
+	{ "sim900",	"option",	"05c6", "9000"	},

The sim900 is a serial modem... there is no vendor id nor product id.

Quoted 12 lines
plugins/udevng.c +0 -1
  	{ "sim7100",	"option",	"1e0e", "9001"	},
  	{ "telit",	"usbserial",	"1bc7"		},
  	{ "telit",	"option",	"1bc7"		},

@@ -1693,7 +1737,6 @@ static struct {
  	{ "xmm7xxx",	"cdc_ncm",	"8087"		},
  	{ }
  };
-
  static void check_usb_device(struct udev_device *device)
  {
  	struct udev_device *usb_device;

plugins/udevng.c static void check_usb_device(struct udev_device *device) · line 1772

Quoted 9 lines
plugins/udevng.c +2 -1
@@ -1722,9 +1765,10 @@ static void check_usb_device(struct udev_device *device)
  			udev_device_get_parent_with_subsystem_devtype(
  				device, "usb", "usb_interface");
 
-		if (usb_interface)
+		if (usb_interface) {
  			driver = udev_device_get_property_value(
  					usb_interface, "OFONO_DRIVER");
+		}
  	}

No.

plugins/udevng.c static gboolean create_modem(gpointer key, gpointer value, gpointer user_data) · line 1851

Quoted 11 lines
plugins/udevng.c +1 -1
  	if (driver == NULL) {

@@ -1801,7 +1845,7 @@ static gboolean create_modem(gpointer key, gpointer value, gpointer user_data)
  	struct modem_info *modem = value;
  	const char *syspath = key;
  	unsigned int i;
-
+	DBG("");
  	if (modem->modem != NULL)
  		return FALSE;

Doesn't belong to this patch, if at all.

Quoted 1 line

/Jonas

author
#6 lore raw

Re: [PATCH 3/3] sim800: add udev detection.

Quoted 204 lines · Jonas Bonn

Hi Clement,

Quoted 39 lines · Clement Viel
---
 plugins/udevng.c | 92 +++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 68 insertions(+), 24 deletions(-)
plugins/udevng.c +26 -2
index ff6e1fc..ee6f9e8 100644
@@ -711,8 +711,56 @@ static gboolean setup_telitqmi(struct modem_info *modem)
 	return TRUE;
 }
-/* TODO: Not used as we have no simcom driver */
-static gboolean setup_simcom(struct modem_info *modem)
+
+static gboolean setup_sim800(struct modem_info *modem)
+{
+	const char *mdm = NULL, *aux = NULL, *gps = NULL, *diag = NULL;
+	GSList *list;
+
+	DBG("%s", modem->syspath);
+
+	for (list = modem->devices; list; list = list->next) {
+		struct device_info *info = list->data;
+
+		DBG("%s %s %s %s", info->devnode, info->interface,
+						info->number, info->label);
+
+		if (g_strcmp0(info->label, "aux") == 0) {
+			DBG("setting aux as info->devnode");
+			aux = info->devnode;
+			if (mdm != NULL)
+				break;
+		} else if (g_strcmp0(info->label, "modem") == 0) {
+			mdm = info->devnode;
+			if (aux != NULL)
+				break;
+		} else if (g_strcmp0(info->interface, "255/0/0") == 0) {
+			if (g_strcmp0(info->number, "00") == 0)
+				mdm = info->devnode;

plugins/udevng.c static gboolean setup_telitqmi(struct modem_info *modem) · line 748

Quoted 10 lines
plugins/udevng.c +10 -0
+			else if (g_strcmp0(info->number, "01") == 0)
+				gps = info->devnode;
+			else if (g_strcmp0(info->number, "02") == 0)
+				aux = info->devnode;
+			else if (g_strcmp0(info->number, "03") == 0)
+				mdm = info->devnode;
+		}
+	}
+	DBG("modem=%s aux=%s gps=%s diag=%s", mdm, aux, gps, diag);
+

gps is unused.

plugins/udevng.c static gboolean setup_telitqmi(struct modem_info *modem) · line 755

Quoted 6 lines
plugins/udevng.c +6 -0
+	if (mdm == NULL) {
+		DBG("modem did not set up");
+		return FALSE;
+	}
+
+	ofono_modem_set_string(modem->modem, "Device", mdm);

You probably mean 'aux' here.

Quoted 12 lines
plugins/udevng.c +8 -0
+	ofono_modem_set_string(modem->modem, "Modem", mdm);
+
+	return TRUE;
+}
+
+
+
+static gboolean setup_sim900(struct modem_info *modem)
 {
 	const char *mdm = NULL, *aux = NULL, *gps = NULL, *diag = NULL;
 	GSList *list;

plugins/udevng.c static gboolean setup_mbim(struct modem_info *modem) · line 1014

Quoted 5 lines
plugins/udevng.c +2 -0
@@ -962,6 +1010,8 @@ static gboolean setup_mbim(struct modem_info *modem)
 	ofono_modem_set_string(modem->modem, "Device", ctl);
 	ofono_modem_set_string(modem->modem, "NetworkInterface", net);
 	ofono_modem_set_string(modem->modem, "DescriptorFile", descriptors);
+	ofono_modem_set_string(modem->modem, "Vendor", modem->vendor);
+	ofono_modem_set_string(modem->modem, "Model", modem->model);

I don't see that these strings are being used anywhere in the modem driver.

Quoted 19 lines
plugins/udevng.c +0 -14
 	return TRUE;
 }

@@ -1193,22 +1243,12 @@ static gboolean setup_xmm7xxx(struct modem_info *modem)
 				info->interface, info->number, info->label,
 				info->sysattr, info->subsystem);
-		if (g_strcmp0(modem->model,"095a") == 0) {
-			if (g_strcmp0(info->subsystem, "tty") == 0) {
-				if (g_strcmp0(info->number, "00") == 0)
-					mdm = info->devnode;
-			} else if (g_strcmp0(info->subsystem, "net") == 0) {
-				if (g_strcmp0(info->number, "06") == 0)
-					net = info->devnode;
-			}
-		} else {
-			if (g_strcmp0(info->subsystem, "tty") == 0) {
-				if (g_strcmp0(info->number, "02") == 0)
-					mdm = info->devnode;
-			} else if (g_strcmp0(info->subsystem, "net") == 0) {
-				if (g_strcmp0(info->number, "00") == 0)

plugins/udevng.c static gboolean setup_xmm7xxx(struct modem_info *modem) · line 1252

Quoted 10 lines
plugins/udevng.c +6 -2
-					net = info->devnode;
-			}
+		if (g_strcmp0(info->subsystem, "tty") == 0) {
+			if (g_strcmp0(info->number, "02") == 0)
+				mdm = info->devnode;
+		} else if (g_strcmp0(info->subsystem, "net") == 0) {
+			if (g_strcmp0(info->number, "00") == 0)
+				net = info->devnode;
 		}
 	}

This change doesn't look like it belongs to this patch.

plugins/udevng.c static struct { · line 1334

Quoted 7 lines
plugins/udevng.c +2 -1
@@ -1290,7 +1330,8 @@ static struct {
 	{ "nokia",	setup_nokia	},
 	{ "telit",	setup_telit,	"device/interface"	},
 	{ "telitqmi",	setup_telitqmi	},
-	{ "simcom",	setup_simcom	},
+	{ "sim800",	setup_sim800	},
+	{ "sim900",	setup_sim900	},

Is the sim900 not a serial modem? These setup routines are really only for USB devices.

plugins/udevng.c static struct { · line 1354

Quoted 10 lines
plugins/udevng.c +2 -0
 	{ "sim7100",	setup_sim7100	},
 	{ "zte",	setup_zte	},
 	{ "icera",	setup_icera	},

@@ -1308,7 +1349,9 @@ static struct {
 	{ "calypso",	setup_serial_modem	},
 	{ "cinterion",	setup_serial_modem	},
 	{ "nokiacdma",	setup_serial_modem	},
+	{ "sim800",	setup_serial_modem	},
 	{ "sim900",	setup_serial_modem	},
+	{ "sim800",	setup_serial_modem	},

You've added the same line twice. Furthermore, the sim800 is a USB modem (???) so it should not be set up in this way.

Quoted 13 lines
plugins/udevng.c +1 -1
 	{ "wavecom",	setup_wavecom		},
 	{ "tc65",	setup_tc65		},
 	{ "ehs6",	setup_ehs6		},

@@ -1471,7 +1514,7 @@ static void add_serial_device(struct udev_device *dev)
 	const char *subsystem;
 	struct udev_device* mdev;
 	const char* driver;
-
+	DBG("adding %s interface", udev_device_get_devpath(dev));
 	mdev = get_serial_modem_device(dev);
 	if (!mdev) {
 		DBG("Device is missing required OFONO_DRIVER property");

plugins/udevng.c static struct { · line 1710

Quoted 6 lines
plugins/udevng.c +2 -1
@@ -1663,7 +1706,8 @@ static struct {
 	{ "alcatel",	"option",	"1bbb", "0017"	},
 	{ "novatel",	"option",	"1410"		},
 	{ "zte",	"option",	"19d2"		},
-	{ "simcom",	"option",	"05c6", "9000"	},
+	{ "sim800",	"option",	"05c6", "9000"	}
+	{ "sim900",	"option",	"05c6", "9000"	},

The sim900 is a serial modem... there is no vendor id nor product id.

Quoted 12 lines
plugins/udevng.c +0 -1
 	{ "sim7100",	"option",	"1e0e", "9001"	},
 	{ "telit",	"usbserial",	"1bc7"		},
 	{ "telit",	"option",	"1bc7"		},

@@ -1693,7 +1737,6 @@ static struct {
 	{ "xmm7xxx",	"cdc_ncm",	"8087"		},
 	{ }
 };
-
 static void check_usb_device(struct udev_device *device)
 {
 	struct udev_device *usb_device;

plugins/udevng.c static void check_usb_device(struct udev_device *device) · line 1771

Quoted 8 lines
plugins/udevng.c +2 -1
@@ -1722,9 +1765,10 @@ static void check_usb_device(struct udev_device *device)
 			udev_device_get_parent_with_subsystem_devtype(
 				device, "usb", "usb_interface");
-		if (usb_interface)
+		if (usb_interface) {
 			driver = udev_device_get_property_value(
 					usb_interface, "OFONO_DRIVER");
+		}
 	}

No.

plugins/udevng.c static gboolean create_modem(gpointer key, gpointer value, gpointer user_data) · line 1850

Quoted 9 lines
plugins/udevng.c +1 -1
 	if (driver == NULL) {

@@ -1801,7 +1845,7 @@ static gboolean create_modem(gpointer key, gpointer value, gpointer user_data)
 	struct modem_info *modem = value;
 	const char *syspath = key;
 	unsigned int i;
-
+	DBG("");
 	if (modem->modem != NULL)
 		return FALSE;

Doesn't belong to this patch, if at all.

Quoted 1 line
Quoted 1 line

/Jonas

Hi Jonas,

Thanks for the review, this patch is all wrong because it's not the good one...litlle bit tired, i'm sorry.

#7 lore raw
In reply to Clement Viel

Hi Clement,

Quoted 17 lines · Clement Viel
---
  plugins/sim900.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++------
  1 file changed, 98 insertions(+), 10 deletions(-)
plugins/sim900.c +1 -0
index a7728cd..c12576d 100644
@@ -24,6 +24,7 @@
  #endif
 
  #include <errno.h>
+#include <string.h>
  #include <stdlib.h>
  #include <glib.h>
  #include <gatchat.h>

plugins/sim900.c static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "SMS: ", · line 69

Quoted 9 lines
plugins/sim900.c +6 -0
@@ -60,13 +61,73 @@ static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "SMS: ",
 
  static const char *none_prefix[] = { NULL };
 
+typedef enum type {
+	SIM800,
+	SIM900,
+	SIM_UNKNOWN,
+} type_t;
+

type_t is not a great name... furthermore, I don't think ofono wants typedef'd enums. Just use: enum simcom_model {...} or similar.

Quoted 11 lines
plugins/sim900.c +4 -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);

plugins/sim900.c static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "SMS: ", · line 90

Quoted 10 lines
plugins/sim900.c +10 -0
+
+	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);
+}
+

This function is unnecessary... see below.

Quoted 27 lines
plugins/sim900.c +27 -0
+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;
+

plugins/sim900.c static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "SMS: ", · line 127

Quoted 10 lines
plugins/sim900.c +10 -0
+	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;
+
+		if (strstr(model, "SIM800"))
+			set_modem_type("sim800", modem);
+		else if (strstr(model, "SIM900"))
+			set_modem_type("sim900", modem);

Just set the type directly here... no need to pass an arbitrary string through a secondary function to accomplish that.

Furthermore, the result is exactly "SIM800" or "SIM900", right? No need for strstr, use strcmp.

Quoted 43 lines
plugins/sim900.c +10 -1
+	}
+}
+
  static int sim900_probe(struct ofono_modem *modem)
  {
  	struct sim900_data *data;

@@ -78,7 +139,6 @@ static int sim900_probe(struct ofono_modem *modem)
  		return -ENOMEM;
 
  	ofono_modem_set_data(modem, data);
-
  	return 0;
  }
 

@@ -111,6 +171,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 +293,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 +360,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,

plugins/sim900.c static void sim900_post_sim(struct ofono_modem *modem) · line 423

Quoted 6 lines
plugins/sim900.c +1 -2
@@ -353,18 +420,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) {

How about "== SIM900" because that's what this is for.

Quoted 27 lines
plugins/sim900.c +10 -6
+		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)

@@ -391,9 +460,28 @@ static struct ofono_modem_driver sim900_driver = {
  	.post_online	= sim900_post_online,
  };
 
+

plugins/sim900.c static struct ofono_modem_driver sim900_driver = { · line 473

Quoted 10 lines
plugins/sim900.c +10 -0
+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,
+};

This is identical to the sim900_driver.

Your driver is already taking into account the modem model and adjusting functionality accordingly. You don't need to specify a new driver for the sim800 where the only thing that changes is the name.

Quoted 6 lines
plugins/sim900.c +2 -1
+
  static int sim900_init(void)
  {
-	return ofono_modem_driver_register(&sim900_driver);
+	int ret = 0;

plugins/sim900.c static struct ofono_modem_driver sim900_driver = { · line 487

Quoted 10 lines
plugins/sim900.c +7 -0
+	ret = ofono_modem_driver_register(&sim800_driver);
+	if (!ret)
+		return ret;
+	ret = ofono_modem_driver_register(&sim900_driver);
+	if (!ret)
+		return ret;
+	return ret;
  }
 
  static void sim900_exit(void)

/Jonas