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

[Clement Viel]

---
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;
+
+}
+
+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);
+ }
+
+}
+
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);
+
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);
+ }
+ }

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);
+ }
}

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;
}

static void sim900_exit(void)

[Clement Viel]

---
AUTHORS | 1 +
doc/sim800-modem.txt | 11 +++++++++++
2 files changed, 12 insertions(+)
create mode 100644 doc/sim800-modem.txt

diff --git a/AUTHORS b/AUTHORS
index 2d360e6..a8362c8 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -138,3 +138,4 @@ Florent Beillonnet <***@gmail.com>
Martin Hundebøll <***@geanix.com>
Julien Tournier <***@gmail.com>
Nandini Rebello <***@intel.com>
+Clement Viel <***@gmail.com>
diff --git a/doc/sim800-modem.txt b/doc/sim800-modem.txt
new file mode 100644
index 0000000..7220cb8
--- /dev/null
+++ b/doc/sim800-modem.txt
@@ -0,0 +1,11 @@
+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=="ttyS2", ENV{OFONO_DRIVER}="sim800"
+
+Sim800 driver code is merged with sim900's. So, to add modifications to sim800 driver, sim900.c file must be edited
+accordingly.
+To differentiate the two modems, developpers must use the "modem_type" field of sim900_data structure.

[Denis Kenzior]

Hi Clement,

Post by Clement Viel
---
AUTHORS | 1 +

Please don't include AUTHORS changes. One of the maintainers will take
care of it.

Post by Clement Viel
doc/sim800-modem.txt | 11 +++++++++++
2 files changed, 12 insertions(+)
create mode 100644 doc/sim800-modem.txt
diff --git a/AUTHORS b/AUTHORS
index 2d360e6..a8362c8 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -138,3 +138,4 @@ Florent Beillonnet <***@gmail.com>
diff --git a/doc/sim800-modem.txt b/doc/sim800-modem.txt
new file mode 100644
index 0000000..7220cb8
--- /dev/null
+++ b/doc/sim800-modem.txt
@@ -0,0 +1,11 @@
+SIM800 modem usage
+===================
+
+To enable SIM800 module support you need to put the following
+
+KERNEL=="ttyS2", ENV{OFONO_DRIVER}="sim800"
+
+Sim800 driver code is merged with sim900's. So, to add modifications to sim800 driver, sim900.c file must be edited
+accordingly.
+To differentiate the two modems, developpers must use the "modem_type" field of sim900_data

typo

structure.
Also, we can't have people hand-editing driver code to distinguish
between one device and another. You've already went to the trouble of
creating a separate sim800 driver (which I do not think is the right
approach at all), why not take advantage of that fact with an
appropriate probe() function or something?

Anyway, I still think you should be querying +CGMM or ATI or whatever to
figure out the modem model automatically and not rely on the user to set
this up.

Regards,
-Denis

[Clement Viel]

Hi Denis,

Post by Denis Kenzior
Hi Clement,

Post by Clement Viel
---
AUTHORS | 1 +

Please don't include AUTHORS changes. One of the maintainers will take care
of it.

Post by Clement Viel
doc/sim800-modem.txt | 11 +++++++++++
2 files changed, 12 insertions(+)
create mode 100644 doc/sim800-modem.txt
diff --git a/AUTHORS b/AUTHORS
index 2d360e6..a8362c8 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -138,3 +138,4 @@ Florent Beillonnet <***@gmail.com>
diff --git a/doc/sim800-modem.txt b/doc/sim800-modem.txt
new file mode 100644
index 0000000..7220cb8
--- /dev/null
+++ b/doc/sim800-modem.txt
@@ -0,0 +1,11 @@
+SIM800 modem usage
+===================
+
+To enable SIM800 module support you need to put the following
+
+KERNEL=="ttyS2", ENV{OFONO_DRIVER}="sim800"
+
+Sim800 driver code is merged with sim900's. So, to add modifications to sim800 driver, sim900.c file must be edited
+accordingly.
+To differentiate the two modems, developpers must use the "modem_type"
field of sim900_data

typo
structure.
Also, we can't have people hand-editing driver code to distinguish between
one device and another. You've already went to the trouble of creating a
separate sim800 driver (which I do not think is the right approach at all),
why not take advantage of that fact with an appropriate probe() function or
something?

These indications were targeting developpers to warn them about the fact that sim800 and sim900 drivers are merged.
I'll remove these lines

Post by Denis Kenzior
Anyway, I still think you should be querying +CGMM or ATI or whatever to
figure out the modem model automatically and not rely on the user to set
this up.

Yep this will be done.

Post by Denis Kenzior
Regards,
-Denis

Regards
Clem

[Clement Viel]

---
plugins/udevng.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 3c39e68..202519e 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1300,6 +1300,7 @@ static struct {
{ "calypso", setup_serial_modem },
{ "cinterion", setup_serial_modem },
{ "nokiacdma", setup_serial_modem },
+ { "sim800", setup_serial_modem },
{ "sim900", setup_serial_modem },
{ "wavecom", setup_wavecom },
{ "tc65", setup_tc65 },

[Clément VIEL]

Hi all,

I sent these patches 2 weeks ago and did not receive any feedback.
It's not urgent at all, I was just wandering whether they'll be
reviewed so I can move on :-)

Regards

Post by Clement Viel
---
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;
+
+}
+
+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);
+ }
+
+}
+
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);
+
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);
+ }
+ }
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);
+ }
}
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;
}
static void sim900_exit(void)
--
2.7.4

[Denis Kenzior]

Hi Clement,

Post by Clement Viel
---
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

Post by Clement Viel
+}
+
+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.

Post by Clement Viel
+}
+
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

Post by Clement Viel
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?

Post by Clement Viel
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?

Post by Clement Viel
}
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.

Post by Clement Viel
}
static void sim900_exit(void)

Regards,
-Denis

[Clement Viel]

Hi Denis,

Thanks for the quick review !

Post by Denis Kenzior
Hi Clement,

Post by Clement Viel
---
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.

Post by Denis Kenzior

Post by Clement Viel
+}
+
+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.

Post by Clement Viel
+}
+
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

Post by Clement Viel
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.

Post by Denis Kenzior

Post by Clement Viel
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.

Post by Denis Kenzior

Post by Clement Viel
}
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.

Post by Clement Viel
}
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

[Clement Viel]

---
plugins/sim900.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 98 insertions(+), 10 deletions(-)

diff --git a/plugins/sim900.c b/plugins/sim900.c
index a7728cd..c12576d 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,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)

[Clement Viel]

---
doc/sim800-modem.txt | 7 +++++++
1 file changed, 7 insertions(+)
create mode 100644 doc/sim800-modem.txt

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"

[Jonas Bonn]

Hi Clement,

Post by Clement Viel
---
doc/sim800-modem.txt | 7 +++++++
1 file changed, 7 insertions(+)
create mode 100644 doc/sim800-modem.txt
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
+
+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

[Pičugins Arsenijs]

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

Hi Clement,

 ---
   doc/sim800-modem.txt | 7 +++++++
   1 file changed, 7 insertions(+)
   create mode 100644 doc/sim800-modem.txt
 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
 +SIM800 modem usage
 +===================
 +
 +To enable SIM800 module support you need to put the following
 +
 +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
https://lists.ofono.org/mailman/listinfo/ofono

[Clement Viel]

---
plugins/udevng.c | 92 +++++++++++++++++++++++++++++++++++++++++---------------
1 file changed, 68 insertions(+), 24 deletions(-)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index ff6e1fc..ee6f9e8 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -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;

[Jonas Bonn]

Hi Clement,

Post by Clement Viel
---
plugins/udevng.c | 92 +++++++++++++++++++++++++++++++++++++++++---------------
1 file changed, 68 insertions(+), 24 deletions(-)
diff --git a/plugins/udevng.c b/plugins/udevng.c
index ff6e1fc..ee6f9e8 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -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);
+

gps is unused.

Post by Clement Viel
+ if (mdm == NULL) {
+ DBG("modem did not set up");
+ return FALSE;
+ }
+
+ ofono_modem_set_string(modem->modem, "Device", mdm);

You probably mean 'aux' here.

Post by Clement Viel
+ 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);

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

Post by Clement Viel
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;
}
}

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

Post by Clement Viel
@@ -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.

Post by Clement Viel
{ "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.

Post by Clement Viel
{ "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" },

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

Post by Clement Viel
{ "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");
+ }
}

No.

Post by Clement Viel
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.
/Jonas

[Clement Viel]

Post by Denis Kenzior
Hi Clement,

Post by Clement Viel
---
plugins/udevng.c | 92 +++++++++++++++++++++++++++++++++++++++++---------------
1 file changed, 68 insertions(+), 24 deletions(-)
diff --git a/plugins/udevng.c b/plugins/udevng.c
index ff6e1fc..ee6f9e8 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -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);
+

gps is unused.

Post by Clement Viel
+ if (mdm == NULL) {
+ DBG("modem did not set up");
+ return FALSE;
+ }
+
+ ofono_modem_set_string(modem->modem, "Device", mdm);

You probably mean 'aux' here.

Post by Clement Viel
+ 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);

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

Post by Clement Viel
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;
}
}

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

Post by Clement Viel
@@ -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.

Post by Clement Viel
{ "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.

Post by Clement Viel
{ "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" },

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

Post by Clement Viel
{ "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");
+ }
}

No.

Post by Clement Viel
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.
/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.

[Jonas Bonn]

Hi Clement,

Post by Clement Viel
---
plugins/sim900.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 98 insertions(+), 10 deletions(-)
diff --git a/plugins/sim900.c b/plugins/sim900.c
index a7728cd..c12576d 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,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.

Post by Clement Viel
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);
+}
+

This function is unnecessary... see below.

Post by Clement Viel
+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);

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.

Post by Clement Viel
+ }
+}
+
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) {

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

Post by Clement Viel
+ 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,
+};

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.

Post by Clement Viel
+
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)

/Jonas

[Clement Viel]

---
plugins/udevng.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index ff6e1fc..d0a849d 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1308,6 +1308,7 @@ static struct {
{ "calypso", setup_serial_modem },
{ "cinterion", setup_serial_modem },
{ "nokiacdma", setup_serial_modem },
+ { "sim800", setup_serial_modem },
{ "sim900", setup_serial_modem },
{ "wavecom", setup_wavecom },
{ "tc65", setup_tc65 },

[Jonas Bonn]

Post by Clement Viel
---
plugins/udevng.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/plugins/udevng.c b/plugins/udevng.c
index ff6e1fc..d0a849d 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1308,6 +1308,7 @@ static struct {
{ "calypso", setup_serial_modem },
{ "cinterion", setup_serial_modem },
{ "nokiacdma", setup_serial_modem },
+ { "sim800", setup_serial_modem },

Right... that's much simpler. And given that you are extending the
sim900 driver to handle the sim800 model, you don't need the above line
either.

/Jonas

Post by Clement Viel
{ "sim900", setup_serial_modem },
{ "wavecom", setup_wavecom },
{ "tc65", setup_tc65 },

[Clement Viel]

Post by Clement Viel
---
plugins/udevng.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/plugins/udevng.c b/plugins/udevng.c
index ff6e1fc..d0a849d 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1308,6 +1308,7 @@ static struct {
{ "calypso", setup_serial_modem },
{ "cinterion", setup_serial_modem },
{ "nokiacdma", setup_serial_modem },
+ { "sim800", setup_serial_modem },

Right... that's much simpler. And given that you are extending the sim900
driver to handle the sim800 model, you don't need the above line either.
/Jonas

Post by Clement Viel
{ "sim900", setup_serial_modem },
{ "wavecom", setup_wavecom },
{ "tc65", setup_tc65 },

I think I must keep this line because the udev rule added by the user will specify that modem is "OFONO_DRIVER=sim800" so udevng.c must know what to do with that.

[Denis Kenzior]

Hi Clement,

+ { "sim800", setup_serial_modem }, > I think I must keep this line because the udev rule added by the user

will specify that modem is "OFONO_DRIVER=sim800" so udevng.c must know
what to do with that.
This is kind of weird that we have a driver line for a modem driver that
doesn't exist. We never do things this way. Why don't you just modify
the driver instructions to have OFONO_DRIVER=sim900 instead?

Also, we could simply rename the sim900 driver into sim_x00 or
simcom_serial or something too. That could make things a bit more clear.

Regards,
-Denis

[Clement Viel]

On Thu, Nov 08, 2018 at 10:59:30AM -0600, Denis Kenzior wrote:

Hi Denis,

Post by Denis Kenzior
Hi Clement,

+ { "sim800", setup_serial_modem }, > I think I must keep this line
because the udev rule added by the user

will specify that modem is "OFONO_DRIVER=sim800" so udevng.c must know what
to do with that.
This is kind of weird that we have a driver line for a modem driver that
doesn't exist. We never do things this way. Why don't you just modify the
driver instructions to have OFONO_DRIVER=sim900 instead?

I agree with you, I was thinking the sim800 must be mentioned to avoid confusing
doc/sim900.txt to mention that sim800 is supported by this driver. Then, keep in
the users.

Post by Denis Kenzior
Also, we could simply rename the sim900 driver into sim_x00 or simcom_serial
or something too. That could make things a bit more clear.

I think we can keep that in mind for further development.

Post by Denis Kenzior
Regards,
-Denis

Just tell me whether I should add mention of sim800 in sim900.txt or rename sim900.txt
in simcom.txt to mention the two drivers. Then I'll upload the patchset with doc in it.

Regards
Clement

[Denis Kenzior]

Hi Clement,

Post by Clement Viel
Just tell me whether I should add mention of sim800 in sim900.txt or rename sim900.txt
in simcom.txt to mention the two drivers. Then I'll upload the patchset with doc in it.

I don't feel strongly either way, but if I had to pick, I say lets
rename sim900.txt into simcom.txt and maintain all driver instructions
there.

Regards,
-Denis

[Clement Viel]

Post by Denis Kenzior
Hi Clement,

+ { "sim800", setup_serial_modem }, > I think I must keep this line
because the udev rule added by the user

will specify that modem is "OFONO_DRIVER=sim800" so udevng.c must know what
to do with that.
This is kind of weird that we have a driver line for a modem driver that
doesn't exist. We never do things this way. Why don't you just modify the
driver instructions to have OFONO_DRIVER=sim900 instead?
Also, we could simply rename the sim900 driver into sim_x00 or simcom_serial
or something too. That could make things a bit more clear.
Regards,
-Denis

Moreover if we go for the OFONO_DRIVER=sim900 solution, plugins/udevng.c will stay
the same.