Re: [RFC PATCH 4/4] plugins: gemalto: fix incomplete at-chat shutdown
Hello Denis, > > Function gemalto_modem_ready attempts to restart AT chat data->app > > after incomplete shutdown. As a result, new AT chat does not work > > as expected loosing AT commands. > > > > Signed-off-by: Sergey Matyukevich > > --- > > plugins/gemalto.c | 8 > > 1 file changed, 8 insertions(+) > > Patch 2, 3 look good to me. > > For this one, what is it trying to do actually? > Thanks for review. I will re-send second revision of the patches 1-3 with minor changes according to review comments. Concerning the 4th patch, I thought the problem was in the incomplete cleanup on data->app re-open in gemalto_modem_ready function. However this is not the case. Sometimes, in particular on cold start, modem is not really ready even after it sends ^SYSSTART. As a result, subsequent AT+CFUN=4 command never completes. I have been experimenting with the same approach as in ublox plugin with much better results. So I will complete my experiments and send new fix in a separate patch. Regards, Sergey ___ ofono mailing list -- ofono@ofono.org To unsubscribe send an email to ofono-le...@ofono.org
Re: [RFC PATCH 4/4] plugins: gemalto: fix incomplete at-chat shutdown
Hi Sergey,
On 8/15/20 4:43 PM, Sergey Matyukevich wrote:
Function gemalto_modem_ready attempts to restart AT chat data->app
after incomplete shutdown. As a result, new AT chat does not work
as expected loosing AT commands.
Signed-off-by: Sergey Matyukevich
---
plugins/gemalto.c | 8
1 file changed, 8 insertions(+)
Patch 2, 3 look good to me.
For this one, what is it trying to do actually?
diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 238c7cc4..321c8c1b 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -222,6 +222,8 @@ static void sim_state_cb(gboolean present, gpointer
user_data)
struct ofono_modem *modem = user_data;
struct gemalto_data *data = ofono_modem_get_data(modem);
+ DBG("");
+
at_util_sim_state_query_free(data->sim_state_query);
data->sim_state_query = NULL;
@@ -241,6 +243,8 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
struct ofono_modem *modem = user_data;
struct gemalto_data *data = ofono_modem_get_data(modem);
+ DBG("");
+
if (!ok) {
g_at_chat_unref(data->app);
data->app = NULL;
@@ -451,6 +455,8 @@ static void gemalto_modem_ready(GAtResult *result, gpointer
user_data)
data->modem_ready_id = 0;
data->trial_cmd_id = 0;
+ g_at_chat_cancel_all(data->app);
+ g_at_chat_unregister_all(data->app);
These get called automatically on unref below, assuming this is the last (it
should be) reference.
g_at_chat_unref(data->app);
Instead of this dance of opening / closing the chat in order to force-cancel the
'AT' command queued in gemalto_enable(), maybe there should be something in
GAtChat itself to handle this case. Either a 'force cancel the top of the queue
because I know the modem is broken' or maybe some form of 'send this on the port
until it responds properly'. Alternatively, you could simply use GAtIO and a
timer to write stuff to the port until it responds and only create the chat at
that time.
There's already something a bit similar in the form of
g_at_chat_set_wakeup_command(). It was used on the old Neo Freerunner modem
which would go to 'sleep' after several seconds of inactivity. So the
subsequent AT command would get eaten. You basically had to poke it with an
empty command, have it timeout / return OK and then you could go on with
submitting AT commands again. But I don't think it is a good fit for this
particular case.
data->app = open_device(app);
@@ -466,6 +472,8 @@ 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);
+ DBG("");
+
g_at_chat_unregister(data->app, data->modem_ready_id);
data->modem_ready_id = 0;
Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org
Re: [RFC PATCH 1/4] drivers: gemalto: add gprs-context driver
Hi Sergey,
On 8/15/20 4:43 PM, Sergey Matyukevich wrote:
Some gemalto modems provide USB ethernet interfaces for data path.
Implement gprs-context driver for such modems to send data via
USB ethernet rather than fallback to PPP.
Signed-off-by: Sergey Matyukevich
---
Makefile.am | 3 +-
drivers/gemaltomodem/gemaltomodem.c | 4 +-
drivers/gemaltomodem/gemaltomodem.h | 3 +
drivers/gemaltomodem/gprs-context.c | 280
4 files changed, 288 insertions(+), 2 deletions(-)
create mode 100644 drivers/gemaltomodem/gprs-context.c
This looks fine to me. My only 2 nitpicks are:
1. We do not use S-o-B, so feel free to drop those in the future or I will just
drop them when applying.
+
+static int gemalto_gprs_context_probe(struct ofono_gprs_context *gc,
+ unsigned int vendor, void *data)
+{
+ GAtChat *chat = data;
+ struct gprs_context_data *gcd;
+
+ DBG("");
+
+ gcd = g_try_new0(struct gprs_context_data, 1);
+ if (gcd == NULL)
+ return -ENOMEM;
+
2. We now prefer to use g_new0 for small structure allocations as these can't
really fail on Linux userspace in practice. Keeps the code shorter as well.
+ gcd->chat = g_at_chat_clone(chat);
+
Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org
Re: [RFC PATCH 4/4] plugins: gemalto: fix incomplete at-chat shutdown
> Function gemalto_modem_ready attempts to restart AT chat data->app > after incomplete shutdown. As a result, new AT chat does not work > as expected loosing AT commands. > > Signed-off-by: Sergey Matyukevich > --- > plugins/gemalto.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/plugins/gemalto.c b/plugins/gemalto.c > index 238c7cc4..321c8c1b 100644 > --- a/plugins/gemalto.c > +++ b/plugins/gemalto.c ... > @@ -451,6 +455,8 @@ static void gemalto_modem_ready(GAtResult *result, > gpointer user_data) > data->modem_ready_id = 0; > data->trial_cmd_id = 0; > > + g_at_chat_cancel_all(data->app); > + g_at_chat_unregister_all(data->app); > g_at_chat_unref(data->app); > > data->app = open_device(app); Please disregard this patch in the series. This change does not resolve the root cause. It looks like when gemalto_modem_ready is called on ^SYSSTART, occasionally modem is not ready to process AT commands. As a result, the upcoming gemalto_initialize function fails due to timed out AT commands. It turns out that the approach implemented in ublox plugin (repeated probe AT command) provides more reliable results, in particular in power-off tests. I will do more testing and update this patch in the next revision of the patch set. Regards, Sergey ___ ofono mailing list -- ofono@ofono.org To unsubscribe send an email to ofono-le...@ofono.org
Re: [PATCH v4] gprs: Quectel EC21 does not understand AT+CPSB
Hi Lars, On 8/17/20 2:58 AM, poesc...@lemonage.de wrote: From: Lars Poeschel The Quectel EC21 modem does not understand the AT+CPSB command, and we did not find a suitable replacement in the Quectel_EC25_AT_Commands_Manual_V1.3.pdf AT+CPSB gives an error on this modem, so we just skip it. --- drivers/atmodem/gprs.c | 2 ++ 1 file changed, 2 insertions(+) Applied, thanks. Regards, -Denis ___ ofono mailing list -- ofono@ofono.org To unsubscribe send an email to ofono-le...@ofono.org
[PATCH v4] gprs: Quectel EC21 does not understand AT+CPSB
From: Lars Poeschel The Quectel EC21 modem does not understand the AT+CPSB command, and we did not find a suitable replacement in the Quectel_EC25_AT_Commands_Manual_V1.3.pdf AT+CPSB gives an error on this modem, so we just skip it. --- drivers/atmodem/gprs.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/atmodem/gprs.c b/drivers/atmodem/gprs.c index b637f733..d829e2e6 100644 --- a/drivers/atmodem/gprs.c +++ b/drivers/atmodem/gprs.c @@ -624,6 +624,8 @@ static void gprs_initialized(gboolean ok, GAtResult *result, gpointer user_data) g_at_chat_send(gd->chat, "AT#PSNT=1", none_prefix, NULL, NULL, NULL); break; + case OFONO_VENDOR_QUECTEL_EC2X: + break; default: g_at_chat_register(gd->chat, "+CPSB:", cpsb_notify, FALSE, gprs, NULL); -- 2.27.0 ___ ofono mailing list -- ofono@ofono.org To unsubscribe send an email to ofono-le...@ofono.org
Re: [PATCH v3] gprs: Quectel EC21 does not understand AT+CPSB
On Thu, Aug 13, 2020 at 10:03:12AM -0500, Denis Kenzior wrote: > > @@ -624,6 +667,12 @@ static void gprs_initialized(gboolean ok, GAtResult > > *result, gpointer user_data) > > g_at_chat_send(gd->chat, "AT#PSNT=1", none_prefix, > > NULL, NULL, NULL); > > break; > > + case OFONO_VENDOR_QUECTEL_EC2X: > > + g_at_chat_register(gd->chat, "+QIND", > > + quectel_qind_notify, FALSE, gprs, NULL); > > + /* The "QIND: \"act\", ..." URC is activated in > > +* network-registration.c */ > > You're using the QIND in network-registration.c to notify CREG technology. > And now you're trying to use this here in place of CPSB. This seems fishy > to me. Looking at the Quectel AT commands manual, I'd just guess they do not > support this bearer concept at all... Until here, you could tell me way more than the official quectel support. Thank you for this! I will post a new version of the patch that will just skip the AT+CPSB command. Thanks a lot! Lars ___ ofono mailing list -- ofono@ofono.org To unsubscribe send an email to ofono-le...@ofono.org
