Hi Denis,
On 15/03/2018 18:11, Denis Kenzior wrote:
No // please, use /* */. This is C code ;)
Fixed. I've just learned it was not ANSI C :).
You leak options here in case channel == NULL. The typical pattern is
to call g_hash_table_destroy(options) right after g_at_tty_open. See
other plugins for details.
Fixed
@@ -118,6 +138,7 @@ static GAtChat *open_device(const char *device)
chat = g_at_chat_new(channel, syntax);
g_at_syntax_unref(syntax);
g_io_channel_unref(channel);
+ g_hash_table_destroy(options);
if (chat == NULL)
return NULL;
@@ -300,29 +321,33 @@ static int
gemalto_hardware_monitor_enable(struct ofono_modem *modem)
return 0;
}
-static int gemalto_enable(struct ofono_modem *modem)
+static void gemalto_initialize(struct ofono_modem *modem)
{
struct gemalto_data *data = ofono_modem_get_data(modem);
const char *app, *mdm;
- DBG("%p", modem);
+ /* Close devices */
+ g_at_chat_unref(data->mdm);
+ g_at_chat_unref(data->app);
+
Why would you close both devices? It is unnecessary, especially in
the case that the modem is already up and responding to commands.
Because it seems that opening the device while it is not ready leads the
driver to a weird state.
When the modem becomes ready, I am not able to send any AT command to
the APP device.
Here is the output:
ofonod[12826]: src/modem.c:get_modem_property() modem 0x72b750 property
Application
ofonod[12826]: src/modem.c:get_modem_property() modem 0x72b750 property
Modem
ofonod[12826]: plugins/gemalto.c:open_device() Opening device /dev/ttyACM1
ofonod[12826]: plugins/gemalto.c:open_device() Opening device /dev/ttyACM0
ofonod[12826]: plugins/gemalto.c:gemalto_modem_ready()
ofonod[12826]: plugins/gemalto.c:gemalto_initialize()
ofonod[12826]: src/modem.c:get_modem_property() modem 0x72b750 property
Application
ofonod[12826]: src/modem.c:get_modem_property() modem 0x72b750 property
Modem
ofonod[12826]: plugins/gemalto.c:gemalto_hardware_monitor_enable()
ofonod[12826]: Mdm> ATE0\r
ofonod[12826]: Mdm< ATE0\r\r\nOK\r\n
ofonod[12826]: Mdm> AT&C0\r
ofonod[12826]: Mdm< \r\n+CME ERROR: operation not supported\r\n
ofonod[12826]: src/modem.c:set_powered_timeout() modem: 0x72b750
When it is re-opened, it works fine.
+ DBG("");
app = ofono_modem_get_string(modem, "Application");
mdm = ofono_modem_get_string(modem, "Modem");
if (app == NULL || mdm == NULL)
- return -EINVAL;
+ return;
/* Open devices */
data->app = open_device(app);
if (data->app == NULL)
- return -EINVAL;
+ return;
data->mdm = open_device(mdm);
if (data->mdm == NULL) {
g_at_chat_unref(data->app);
data->app = NULL;
- return -EINVAL;
+ return;
}
if (getenv("OFONO_AT_DEBUG")) {
@@ -340,6 +365,67 @@ static int gemalto_enable(struct ofono_modem
*modem)
cfun_enable, modem, NULL);
gemalto_hardware_monitor_enable(modem);
+}
+
+static void gemalto_modem_ready(GAtResult *result, gpointer user_data)
+{
+ struct ofono_modem *modem = user_data;
+ struct gemalto_data *data = ofono_modem_get_data(modem);
+
+ DBG("");
+
+ g_at_chat_unregister(data->app, data->modem_ready_id);
+ g_at_chat_cancel(data->app, data->trial_cmd_id);
+
+ gemalto_initialize(modem);
+}
+
+static void gemalto_at_cb(gboolean ok, GAtResult *result, gpointer
user_data)
+{
+ struct ofono_modem *modem = user_data;
+ struct gemalto_data *data = ofono_modem_get_data(modem);
+
+ g_at_chat_unregister(data->app, data->modem_ready_id);
+ gemalto_initialize(modem);
+}
+
+static gboolean gemalto_at_timeout(gpointer user_data)
+{
+ struct ofono_modem *modem = user_data;
+ struct gemalto_data *data = ofono_modem_get_data(modem);
+
+ data->trial_cmd_id = g_at_chat_send(data->app, "AT+CFUN?",
none_prefix, gemalto_at_cb, modem, NULL);
What does this do?
Some gemalto's modems have their USB devices mounted before they are
ready to handle AT command.
At modem's or oFono's startup, the modem is either ready or not. In
gemalto_enable, we register to the "^SYSSTART" URC and send the "AT"
command.
- If the AT command returns, then the modem is ready so we unregister
the "^SYSSTART" URC and start initializing the modem.
- If the modem isn't ready, the AT command will never return so we have
to wait for the "^SYSSTART" URC. When it is received, we cancel the AT
command because it will never get any answer and start initializing the
modem.
I've no cleaner way to do that.
+
+ return FALSE;
+}
Where is the timeout started?
Seems like I forgot to clean some parts of the code ;). There is no timeout.
We use the Linux Kernel coding style with house rules documented in
doc/coding-style.txt. No lines over 80 characters please.
Fixed
Why do you open both ports only to close them in gemalto_initialize?
One port here would be sufficient.
You are right, I've changed the code such that it opens only app, then
try the modem on it and reopen it
only if the modem wasn't ready.
You might as well perform this assignment at variable declaration.
Fixed
+
+ if(!strncmp(model, GEMALTO_MODEL_ALS3, 4)) {
+ ofono_lte_create(modem, "atmodem", data->app);
+ }
Why strncmp? No need for {} and there should be a space between if
and '('
Because strncmp is more secured than strcmp. But if you prefer strcmp or
anything else, I'll change it.
Also, you most likely need the LTE atom in post_sim.
You also now have to little arguments to ofono_lte_create. Please git
pull --rebase first.
Fixed
I'm waiting for your answers before reposting the reviewed patch.
Thanks for the patch review.
Best Regards,
Gabriel
_______________________________________________
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono
