[PATCH 1/3] sim800: adding support for SimCom SIM800 modem

Discussion:

Clement Viel

2018-09-24 22:43:28 UTC

From: clem <***@gmail.com>

---
Makefile.am | 4 +
plugins/sim800.c | 463 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 467 insertions(+)
create mode 100644 plugins/sim800.c

diff --git a/Makefile.am b/Makefile.am
index 6dee4ce..f4d03b6 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -496,6 +496,10 @@ builtin_sources += plugins/speedupcdma.c
builtin_modules += samsung
builtin_sources += plugins/samsung.c

+builtin_modules += sim800
+builtin_sources += plugins/sim800.c
+
+
builtin_modules += sim900
builtin_sources += plugins/sim900.c

diff --git a/plugins/sim800.c b/plugins/sim800.c
new file mode 100644
index 0000000..c9285c1
--- /dev/null
+++ b/plugins/sim800.c
@@ -0,0 +1,463 @@
+/*
+ *
+ * oFono - Open Source Telephony
+ *
+ * Copyright (C) 2008-2011 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <errno.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <glib.h>
+#include <gatchat.h>
+#include <gattty.h>
+#include <gatmux.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/plugin.h>
+#include <ofono/modem.h>
+#include <ofono/devinfo.h>
+#include <ofono/netreg.h>
+#include <ofono/sim.h>
+#include <ofono/sms.h>
+#include <ofono/ussd.h>
+#include <ofono/gprs.h>
+#include <ofono/gprs-context.h>
+#include <ofono/phonebook.h>
+#include <ofono/history.h>
+#include <ofono/log.h>
+#include <ofono/voicecall.h>
+#include <ofono/call-volume.h>
+#include <drivers/atmodem/vendor.h>
+
+#define NUM_DLC 5
+
+#define VOICE_DLC 0
+#define NETREG_DLC 1
+#define SMS_DLC 2
+#define GPRS_DLC 3
+#define SETUP_DLC 4
+
+static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "SMS: ",
+ "GPRS: " , "Setup: "};
+
+static const char *none_prefix[] = { NULL };
+
+struct sim800_data {
+ GIOChannel *device;
+ GAtMux *mux;
+ GAtChat * dlcs[NUM_DLC];
+ guint frame_size;
+ int mux_not_supported;
+};
+
+
+static void mux_ready_notify(GAtResult *result, gpointer user_data)
+{
+ struct ofono_modem *modem = user_data;
+ struct sim800_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);
+ }
+ notified = 1;
+
+}
+
+
+static int sim800_probe(struct ofono_modem *modem)
+{
+ struct sim800_data *data;
+
+ DBG("%p", modem);
+
+ data = g_try_new0(struct sim800_data, 1);
+ if (data == NULL)
+ return -ENOMEM;
+
+ ofono_modem_set_data(modem, data);
+
+ return 0;
+}
+
+static void sim800_remove(struct ofono_modem *modem)
+{
+ struct sim800_data *data = ofono_modem_get_data(modem);
+
+ DBG("%p", modem);
+
+ ofono_modem_set_data(modem, NULL);
+
+ g_free(data);
+}
+
+static void sim800_debug(const char *str, void *user_data)
+{
+ const char *prefix = user_data;
+
+ ofono_info("%s%s", prefix, str);
+}
+
+static GAtChat *open_device(struct ofono_modem *modem,
+ const char *key, char *debug)
+{
+ struct sim800_data *data = ofono_modem_get_data(modem);
+ const char *device;
+ GAtSyntax *syntax;
+ GIOChannel *channel;
+ GAtChat *chat;
+ GHashTable *options;
+
+ device = ofono_modem_get_string(modem, key);
+ if (device == NULL) {
+ DBG("couldn't get string %s",key);
+ return NULL;
+ }
+
+ DBG("%s %s", key, device);
+
+ options = g_hash_table_new(g_str_hash, g_str_equal);
+ if (options == NULL) {
+ DBG("options is null");
+ return NULL;
+ }
+
+ g_hash_table_insert(options, "Baud", "115200");
+ g_hash_table_insert(options, "Parity", "none");
+ g_hash_table_insert(options, "StopBits", "1");
+ g_hash_table_insert(options, "DataBits", "8");
+ g_hash_table_insert(options, "XonXoff", "off");
+ g_hash_table_insert(options, "Local", "off");
+ g_hash_table_insert(options, "RtsCts", "off");
+ g_hash_table_insert(options, "Read", "on");
+
+ channel = g_at_tty_open(device, options);
+ g_hash_table_destroy(options);
+
+ if (channel == NULL){
+ DBG("serial channel couldn't be opened");
+ return NULL;
+ }
+
+ data->device = channel;
+ syntax = g_at_syntax_new_gsm_permissive();
+ chat = g_at_chat_new(channel, syntax);
+ g_at_syntax_unref(syntax);
+
+ if (chat == NULL) {
+ g_io_channel_unref(data->device);
+ data->device = NULL;
+ DBG("at channel couldn't be opened");
+
+ return NULL;
+ }
+
+ if (getenv("OFONO_AT_DEBUG"))
+ g_at_chat_set_debug(chat, sim800_debug, debug);
+
+ return chat;
+}
+
+static GAtChat *create_chat(GIOChannel *channel, struct ofono_modem *modem,
+ char *debug)
+{
+ GAtSyntax *syntax;
+ GAtChat *chat;
+
+ if (channel == NULL)
+ return NULL;
+
+ syntax = g_at_syntax_new_gsmv1();
+ chat = g_at_chat_new(channel, syntax);
+ g_at_syntax_unref(syntax);
+ g_io_channel_unref(channel);
+
+ if (chat == NULL)
+ return NULL;
+
+ if (getenv("OFONO_AT_DEBUG")) {
+ DBG("AT DEBUG enabled");
+ g_at_chat_set_debug(chat, sim800_debug, debug);
+ }
+
+ return chat;
+}
+
+static void shutdown_device(struct sim800_data *data)
+{
+ int i;
+
+ DBG("");
+
+ for (i = 0; i < NUM_DLC; i++) {
+ if (data->dlcs[i] == NULL)
+ continue;
+
+ g_at_chat_unref(data->dlcs[i]);
+ data->dlcs[i] = NULL;
+ }
+
+ if (data->mux) {
+ g_at_mux_shutdown(data->mux);
+ g_at_mux_unref(data->mux);
+ data->mux = NULL;
+ }
+
+ g_io_channel_unref(data->device);
+ data->device = NULL;
+}
+
+static void setup_internal_mux(struct ofono_modem *modem)
+{
+ struct sim800_data *data = ofono_modem_get_data(modem);
+ int i;
+
+ DBG("");
+
+ data->frame_size = 128;
+
+ data->mux = g_at_mux_new_gsm0710_basic(data->device,
+ data->frame_size);
+ if (data->mux == NULL)
+ goto error;
+
+ if (getenv("OFONO_MUX_DEBUG"))
+ g_at_mux_set_debug(data->mux, sim800_debug, "MUX: ");
+
+ if (!g_at_mux_start(data->mux)) {
+ g_at_mux_shutdown(data->mux);
+ g_at_mux_unref(data->mux);
+ goto error;
+ }
+
+ for (i = 0; i < NUM_DLC; i++) {
+ GIOChannel *channel = g_at_mux_create_channel(data->mux);
+
+ data->dlcs[i] = create_chat(channel, modem, dlc_prefixes[i]);
+ if (data->dlcs[i] == NULL) {
+ ofono_error("Failed to create channel");
+ goto error;
+ }
+ }
+ 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);
+ return;
+
+error:
+ shutdown_device(data);
+ ofono_modem_set_powered(modem, FALSE);
+}
+
+static void mux_setup_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+ struct ofono_modem *modem = user_data;
+ struct sim800_data *data = ofono_modem_get_data(modem);
+
+ DBG("");
+
+ g_at_chat_unref(data->dlcs[SETUP_DLC]);
+ data->dlcs[SETUP_DLC] = NULL;
+
+ if (!ok)
+ goto error;
+
+ data->mux_not_supported = 0;
+ setup_internal_mux(modem);
+ return;
+
+error:
+ DBG("If you are on a SIM800H modem with firmware version is \
+ 1308B02SIM800H32_BT or lower, CMUX command is not supported thus \
+ preventing GPRS, Voice and SMS managers \
+ to be executed simultaneously.");
+ /*
+ * If your use of SIM800 does not need simultaneous use of Voice, SMS
+ * and GPRS, you can ignore the error, set mux_not_supported=1 and comment
+ * setup_internal_mux function, refactor this code to use only one DLC.
+ * Else, you must contact SIMCOM to get a firmware update.
+ */
+ shutdown_device(data);
+ ofono_modem_set_powered(modem, FALSE);
+}
+
+static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
+{
+ struct ofono_modem *modem = user_data;
+ struct sim800_data *data = ofono_modem_get_data(modem);
+
+ DBG("");
+
+ if (!ok) {
+ g_at_chat_unref(data->dlcs[SETUP_DLC]);
+ data->dlcs[SETUP_DLC] = NULL;
+ ofono_modem_set_powered(modem, FALSE);
+ return;
+ }
+
+ g_at_chat_send(data->dlcs[SETUP_DLC],"AT+CMUX=0,0,5,128,10,3,30,10,2", NULL,mux_setup_cb, modem, NULL);
+
+}
+
+static int sim800_enable(struct ofono_modem *modem)
+{
+ struct sim800_data *data = ofono_modem_get_data(modem);
+
+ DBG("%p", modem);
+
+ data->dlcs[SETUP_DLC] = open_device(modem, "Device", "Setup: ");
+ if (data->dlcs[SETUP_DLC] == NULL){
+ DBG("couldn't open device");
+ return -EINVAL;
+ }
+
+ g_at_chat_send(data->dlcs[SETUP_DLC], "ATE0", NULL, NULL, NULL, NULL);
+ g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CFUN=1", none_prefix,
+ cfun_enable, modem, NULL);
+
+ return -EINPROGRESS;
+}
+
+static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
+{
+ struct ofono_modem *modem = user_data;
+ struct sim800_data *data = ofono_modem_get_data(modem);
+
+ DBG("");
+
+ shutdown_device(data);
+
+ if (ok)
+ ofono_modem_set_powered(modem, FALSE);
+}
+
+static int sim800_disable(struct ofono_modem *modem)
+{
+ struct sim800_data *data = ofono_modem_get_data(modem);
+
+ DBG("%p", modem);
+
+ g_at_chat_send(data->dlcs[SETUP_DLC], "AT+CFUN=4", none_prefix,
+ cfun_disable, modem, NULL);
+
+ return -EINPROGRESS;
+}
+
+static void sim800_pre_sim(struct ofono_modem *modem)
+{
+ struct sim800_data *data = ofono_modem_get_data(modem);
+ struct ofono_sim *sim;
+
+ DBG("%p %x", modem, data);
+
+ ofono_devinfo_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
+ DBG("devinfo created");
+ sim = ofono_sim_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
+ data->dlcs[VOICE_DLC]);
+ DBG("sim created created");
+
+ if (sim)
+ ofono_sim_inserted_notify(sim, TRUE);
+
+ DBG("pre sim finished");
+}
+
+static void sim800_post_sim(struct ofono_modem *modem)
+{
+ struct sim800_data *data = ofono_modem_get_data(modem);
+ struct ofono_gprs *gprs = NULL;
+ struct ofono_gprs_context *gc;
+
+ DBG("%p", modem);
+
+ /* If CMUX command is not supported, we can go through
+ * the normal way but with only one DLC.*/
+
+ if (data->mux_not_supported){
+
+ 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 sim800_post_online(struct ofono_modem *modem)
+{
+ struct sim800_data *data = ofono_modem_get_data(modem);
+
+ DBG("%p", modem);
+
+ ofono_phonebook_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
+ ofono_netreg_create(modem, OFONO_VENDOR_SIMCOM,
+ "atmodem", data->dlcs[NETREG_DLC]);
+ ofono_ussd_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
+ ofono_voicecall_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
+ ofono_call_volume_create(modem, 0, "atmodem", data->dlcs[VOICE_DLC]);
+}
+
+static struct ofono_modem_driver sim800_driver = {
+ .name = "sim800",
+ .probe = sim800_probe,
+ .remove = sim800_remove,
+ .enable = sim800_enable,
+ .disable = sim800_disable,
+ .pre_sim = sim800_pre_sim,
+ .post_sim = sim800_post_sim,
+ .post_online = sim800_post_online,
+};
+
+static int sim800_init(void)
+{
+ return ofono_modem_driver_register(&sim800_driver);
+}
+
+static void sim800_exit(void)
+{
+ ofono_modem_driver_unregister(&sim800_driver);
+}
+
+OFONO_PLUGIN_DEFINE(sim800, "SIM800 modem driver", VERSION,
+ OFONO_PLUGIN_PRIORITY_DEFAULT, sim800_init, sim800_exit)

--
2.7.4

Clement Viel

2018-09-24 22:43:29 UTC

Permalink

From: clem <***@gmail.com>

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

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 02d049e..3f019b8 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1300,6 +1300,7 @@ static struct {
{ "cinterion", setup_serial_modem },
{ "nokiacdma", setup_serial_modem },
{ "sim900", setup_serial_modem },
+ { "sim800", setup_serial_modem },
{ "wavecom", setup_wavecom },
{ "tc65", setup_tc65 },
{ "ehs6", setup_ehs6 },

--
2.7.4

Clement Viel

2018-09-24 22:43:30 UTC

Permalink

From: clem <***@gmail.com>

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

diff --git a/AUTHORS b/AUTHORS
index 52f46e9..c5fa295 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -137,3 +137,4 @@ Varun Gargi <***@intel.com>
Florent Beillonnet <***@gmail.com>
Martin Hundebøll <***@geanix.com>
Julien Tournier <***@gmail.com>
+Clement Viel <***@gmail.com>
diff --git a/doc/sim800-modem.txt b/doc/sim800-modem.txt
new file mode 100644
index 0000000..94bf573
--- /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=="gsmtty3", ENV{OFONO_DRIVER}="sim800"

--
2.7.4

Denis Kenzior

2018-09-25 16:48:39 UTC

Permalink

Hi Clement,
Your author information is still broken for the actual commits. Please
fix that.

Post by Clement Viel
---
Makefile.am | 4 +
plugins/sim800.c | 463 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 467 insertions(+)
create mode 100644 plugins/sim800.c
diff --git a/Makefile.am b/Makefile.am
index 6dee4ce..f4d03b6 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -496,6 +496,10 @@ builtin_sources += plugins/speedupcdma.c
builtin_modules += samsung
builtin_sources += plugins/samsung.c
+builtin_modules += sim800
+builtin_sources += plugins/sim800.c
+
+

One empty line please

Post by Clement Viel
builtin_modules += sim900
builtin_sources += plugins/sim900.c
diff --git a/plugins/sim800.c b/plugins/sim800.c
new file mode 100644
index 0000000..c9285c1
--- /dev/null
+++ b/plugins/sim800.c
@@ -0,0 +1,463 @@
+/*
+ *
+ * oFono - Open Source Telephony
+ *
+ * Copyright (C) 2008-2011 Intel Corporation. All rights reserved.

Not updating the copyright?

Post by Clement Viel
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <errno.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <glib.h>
+#include <gatchat.h>
+#include <gattty.h>
+#include <gatmux.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/plugin.h>
+#include <ofono/modem.h>
+#include <ofono/devinfo.h>
+#include <ofono/netreg.h>
+#include <ofono/sim.h>
+#include <ofono/sms.h>
+#include <ofono/ussd.h>
+#include <ofono/gprs.h>
+#include <ofono/gprs-context.h>
+#include <ofono/phonebook.h>
+#include <ofono/history.h>
+#include <ofono/log.h>
+#include <ofono/voicecall.h>
+#include <ofono/call-volume.h>
+#include <drivers/atmodem/vendor.h>
+
+#define NUM_DLC 5
+
+#define VOICE_DLC 0
+#define NETREG_DLC 1
+#define SMS_DLC 2
+#define GPRS_DLC 3
+#define SETUP_DLC 4
+
+static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "SMS: ",
+ "GPRS: " , "Setup: "};
+
+static const char *none_prefix[] = { NULL };
+
+struct sim800_data {
+ GIOChannel *device;
+ GAtMux *mux;
+ GAtChat * dlcs[NUM_DLC];
+ guint frame_size;
+ int mux_not_supported;

bool ?

Post by Clement Viel
+};
+
+

No double empty lines please

Post by Clement Viel
+static void mux_ready_notify(GAtResult *result, gpointer user_data)
+{
+ struct ofono_modem *modem = user_data;
+ struct sim800_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]);
+
+

Wrong indentation & coding style...

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

<snip>

Post by Clement Viel
+static void mux_setup_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+ struct ofono_modem *modem = user_data;
+ struct sim800_data *data = ofono_modem_get_data(modem);
+
+ DBG("");
+
+ g_at_chat_unref(data->dlcs[SETUP_DLC]);
+ data->dlcs[SETUP_DLC] = NULL;
+
+ if (!ok)
+ goto error;
+
+ data->mux_not_supported = 0;

What does this variable do?

Post by Clement Viel
+ setup_internal_mux(modem);
+ return;
+
+ DBG("If you are on a SIM800H modem with firmware version is \
+ 1308B02SIM800H32_BT or lower, CMUX command is not supported thus \
+ preventing GPRS, Voice and SMS managers \
+ to be executed simultaneously.");
+ /*
+ * If your use of SIM800 does not need simultaneous use of Voice, SMS
+ * and GPRS, you can ignore the error, set mux_not_supported=1 and comment
+ * setup_internal_mux function, refactor this code to use only one DLC.
+ * Else, you must contact SIMCOM to get a firmware update.

I would drop all these hacks and document the required firmware version
in doc/sim800-modem.txt.

Post by Clement Viel
+ */
+ shutdown_device(data);
+ ofono_modem_set_powered(modem, FALSE);
+}
+
+static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
+{
+ struct ofono_modem *modem = user_data;
+ struct sim800_data *data = ofono_modem_get_data(modem);
+
+ DBG("");
+
+ if (!ok) {
+ g_at_chat_unref(data->dlcs[SETUP_DLC]);
+ data->dlcs[SETUP_DLC] = NULL;
+ ofono_modem_set_powered(modem, FALSE);
+ return;
+ }
+
+ g_at_chat_send(data->dlcs[SETUP_DLC],"AT+CMUX=0,0,5,128,10,3,30,10,2", NULL,mux_setup_cb, modem, NULL);

Not our coding style.

Post by Clement Viel
+
+}
+

<snip>

I ran a quick diff between this and sim900 driver and there's really not
enough differences to warrant a completely separate plugin. Can't we
just simply query the model version and act accordingly?

Regards,
-Denis

Clement Viel

2018-09-25 18:08:24 UTC

Permalink

Hi Denis,

Post by Denis Kenzior
Hi Clement,
Your author information is still broken for the actual commits. Please fix
that.

One empty line please

Not updating the copyright?

bool ?

Post by Clement Viel
+};
+
+

No double empty lines please

Wrong indentation & coding style...

<snip>

What does this variable do?

I would drop all these hacks and document the required firmware version in
doc/sim800-modem.txt.

Not our coding style.

Post by Clement Viel
+
+}
+

<snip>
I ran a quick diff between this and sim900 driver and there's really not
enough differences to warrant a completely separate plugin. Can't we just
simply query the model version and act accordingly?

My opinion is that the sim800 driver is much to be needed as Simcom considers the sim800 as the new sim900.
I "fear" that merging the two drivers will end up in a big file having a lot of "if...else" for every different feature
between those two drivers. Whereas having the two separate for some time will allow to address the use of sim800's new features
and support legacy sim900.

But as you are the maintainer, I think the decision is yours. Tell me and i'll modify my patches accordingly then preventing too much noisy commits :)

Post by Denis Kenzior
Regards,
-Denis

Regards
Clement

Denis Kenzior

2018-09-25 18:41:23 UTC

Permalink

Hi Clement,

Post by Clement Viel

Post by Denis Kenzior
I ran a quick diff between this and sim900 driver and there's really not
enough differences to warrant a completely separate plugin. Can't we just
simply query the model version and act accordingly?

It might not be as bad as you think. Besides, if we determine at some
future date, that sim800 and sim900 are different enough to warrant
different drivers, then we have the option of splitting them out into
separate drivers at that time.

Post by Clement Viel
But as you are the maintainer, I think the decision is yours. Tell me and i'll modify my patches accordingly then preventing too much noisy commits :)

My feeling is that we should merge these. The only difference I see
right now is a VENDOR flag tweak and an SMS Ready notification handling.
These don't amount to much and could be easily handled by the existing
driver.

Regards,
-Denis