Re: [PATCH 3/5] gemalto: gprs: support automatic context activation
Hi Denis, > > > Are you sure you don't want to wait until swwan_cb to return success to > > > the > > > core? AT^SWWAN can still fail...? > > > > AT^SWWAN command is yet another way to activate a PDP context. AT commands > > spec for this modem is a bit vague about SWWAN details, but according to > > other materials from Gemalto as well as my experiments, this command > > activates internal DHCP server, so DHCP client can be started for modem > > USB ethernet interfaces. Based on my observations, SWWAN command does > > not return until DHCP flow is completed. > > Ugh. I'd 'window.throw(modem)'... Well, suggested revision works fairly well in conjunction with ConnMan. So I would give it a chance ) > > > > So the idea is to send SWWAN command to modem and make it possible to > > start DHCP flow right away. I assume that I need both things: mark > > interface for DHCP and signal success to the core. Callback swwan_cb is > > So the problem with this is that you've now blocked the app/modem port until > that DHCP negotiation happens. Maybe it does, maybe it doesn't. It is less > than ideal to depend on some external component; there are frequently > situations where people would be running without ConnMan for example. > > > supposed to handle the case when SWWAN command fails: mark context as > > deactivated and let oFono proceed with further connection attempts. > > > > Another thing to consider is to just run dhcp yourself. ell has had a > DHCPv4 client for a while now. So you could just run l_dhcp_client to > obtain the address and signal it to the core, leaving the app port in a > known state... > > Or better yet, don't use SWWAN if you can help it... I guess that can be done as well. As I mentioned previously, SWWAN is just one possible option. I assume that another option is to use CGDCONT/CGACT and then to retrieve IP settings from modem. There were earlier patches on the mail list for PLS8x modem, IIRC at least one of them suggested both CGACT and SWWAN. I think I will take a look and update those patches as soon as I am done with this SWWAN part. Though at the moment I have no idea how to make configurable gprs driver selection in gemalto plugin... > > > > Sure, I can add a comment. What whould be better: to add a comment in > > driver or to write a more detailed commit message ? > > Given how unusual this behavior is, I'd add a comment directly in the code. Ok, I will do both in v2: clarify commit message and add a comment into the driver. Regards, Sergey ___ ofono mailing list -- ofono@ofono.org To unsubscribe send an email to ofono-le...@ofono.org
Re: [PATCH 3/5] gemalto: gprs: support automatic context activation
Hi Sergey, Are you sure you don't want to wait until swwan_cb to return success to the core? AT^SWWAN can still fail...? AT^SWWAN command is yet another way to activate a PDP context. AT commands spec for this modem is a bit vague about SWWAN details, but according to other materials from Gemalto as well as my experiments, this command activates internal DHCP server, so DHCP client can be started for modem USB ethernet interfaces. Based on my observations, SWWAN command does not return until DHCP flow is completed. Ugh. I'd 'window.throw(modem)'... So the idea is to send SWWAN command to modem and make it possible to start DHCP flow right away. I assume that I need both things: mark interface for DHCP and signal success to the core. Callback swwan_cb is So the problem with this is that you've now blocked the app/modem port until that DHCP negotiation happens. Maybe it does, maybe it doesn't. It is less than ideal to depend on some external component; there are frequently situations where people would be running without ConnMan for example. supposed to handle the case when SWWAN command fails: mark context as deactivated and let oFono proceed with further connection attempts. Another thing to consider is to just run dhcp yourself. ell has had a DHCPv4 client for a while now. So you could just run l_dhcp_client to obtain the address and signal it to the core, leaving the app port in a known state... Or better yet, don't use SWWAN if you can help it... Sure, I can add a comment. What whould be better: to add a comment in driver or to write a more detailed commit message ? Given how unusual this behavior is, I'd add a comment directly in the code. Regards, -Denis ___ ofono mailing list -- ofono@ofono.org To unsubscribe send an email to ofono-le...@ofono.org
Re: [PATCH 3/5] gemalto: gprs: support automatic context activation
Hello Denis,
> > Implement read_settings function to get configuration for automatic
> > contexts. Fix the issue uncovered by added support for automatic
> > context activation: do not use AT+CGACT for the contexts handled
> > by AT^SWWAN. As per modem specs, CGACT context can not be reused
> > for SWWAN. Though that worked for some reason when automatic
> > context was reactivated without proper deactivation.
> > ---
> > drivers/gemaltomodem/gprs-context.c | 110 +++-
> > 1 file changed, 59 insertions(+), 51 deletions(-)
> > +static void swwan_cb(gboolean ok, GAtResult *result, gpointer user_data)
> > +{
> > + struct ofono_gprs_context *gc = user_data;
> > + struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> > + struct ofono_error error;
> > DBG("ok %d", ok);
> > if (!ok) {
> > - struct ofono_error error;
> > -
> > + ofono_error("Unable to activate context");
> > + ofono_gprs_context_deactivated(gc, gcd->active_context);
> > gcd->active_context = 0;
>
> This seems a bit strange. Are you signaling success to the core too early
Correct, this is intended behavior. See my further comments regarding
SWWAN command and its processing.
> > -
> > decode_at_error(&error, g_at_result_final_response(result));
> > gcd->cb(&error, gcd->cb_data);
> > -
> > return;
> > }
> > -
> > - snprintf(buf, sizeof(buf), "AT^SWWAN=1,%u", gcd->active_context);
> > - g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL);
> > -
> > - modem = ofono_gprs_context_get_modem(gc);
> > - interface = ofono_modem_get_string(modem, "NetworkInterface");
> > - ofono_gprs_context_set_interface(gc, interface);
> > -
> > - /* Use DHCP */
> > - ofono_gprs_context_set_ipv4_address(gc, NULL, 0);
> > -
> > - CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
> > }
> > static void cgdcont_enable_cb(gboolean ok, GAtResult *result,
> > - gpointer user_data)
> > + gpointer user_data)
>
> nit: This is superfluous and also see doc/coding-style.txt item M4
Fixed in v2.
> > - snprintf(buf, sizeof(buf), "AT+CGACT=1,%u", gcd->active_context);
> > -
> > - if (g_at_chat_send(gcd->chat, buf, none_prefix,
> > - cgact_enable_cb, gc, NULL) == 0)
> > - goto error;
> > + snprintf(buf, sizeof(buf), "AT^SWWAN=1,%u", gcd->active_context);
> > + if (g_at_chat_send(gcd->chat, buf, none_prefix, swwan_cb, gc, NULL)) {
>
> Note, this returns > 0 when the command has been queued, not that it has
> been sent or replied to yet...
>
> > + set_gprs_context_interface(gc);
> > + /* Use DHCP */
> > + ofono_gprs_context_set_ipv4_address(gc, NULL, 0);
>
> If these modems can only do DHCP, then might be cleaner to move the 'Use
> DHCP' bits into set_gprs_context_interface. And maybe rename it to make
> things clearer, if needed.
Good point, fixed in v2.
>
> > - return;
> > + CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
>
> Are you sure you don't want to wait until swwan_cb to return success to the
> core? AT^SWWAN can still fail...?
AT^SWWAN command is yet another way to activate a PDP context. AT commands
spec for this modem is a bit vague about SWWAN details, but according to
other materials from Gemalto as well as my experiments, this command
activates internal DHCP server, so DHCP client can be started for modem
USB ethernet interfaces. Based on my observations, SWWAN command does
not return until DHCP flow is completed.
So the idea is to send SWWAN command to modem and make it possible to
start DHCP flow right away. I assume that I need both things: mark
interface for DHCP and signal success to the core. Callback swwan_cb is
supposed to handle the case when SWWAN command fails: mark context as
deactivated and let oFono proceed with further connection attempts.
> > + return;
> > + }
> > -error:
> > CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
> > }
>
>
>
> > @@ -185,17 +170,39 @@ static void gemalto_gprs_deactivate_primary(struct
> > ofono_gprs_context *gc,
> > gcd->cb = cb;
> > gcd->cb_data = data;
> > - snprintf(buf, sizeof(buf), "AT+CGACT=0,%u", cid);
> > + snprintf(buf, sizeof(buf), "AT^SWWAN=0,%u", gcd->active_context);
> > if (g_at_chat_send(gcd->chat, buf, none_prefix,
> > - cgact_disable_cb, gc, NULL) == 0)
> > - goto error;
> > -
> > - return;
> > + deactivate_cb, gc, NULL))
> > + return;
> > -error:
> > CALLBACK_WITH_FAILURE(cb, data);
> > +}
> > +
> > +static void gemalto_gprs_read_settings(struct ofono_gprs_context *gc,
> > + unsigned int cid,
> > + ofono_gprs_context_cb_t cb, void *data)
> > +{
> > + struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
> > + char
Re: [PATCH 3/5] gemalto: gprs: support automatic context activation
Hi Sergey,
On 12/21/20 2:01 PM, Sergey Matyukevich wrote:
From: Sergey Matyukevich
Implement read_settings function to get configuration for automatic
contexts. Fix the issue uncovered by added support for automatic
context activation: do not use AT+CGACT for the contexts handled
by AT^SWWAN. As per modem specs, CGACT context can not be reused
for SWWAN. Though that worked for some reason when automatic
context was reactivated without proper deactivation.
---
drivers/gemaltomodem/gprs-context.c | 110 +++-
1 file changed, 59 insertions(+), 51 deletions(-)
diff --git a/drivers/gemaltomodem/gprs-context.c
b/drivers/gemaltomodem/gprs-context.c
index 322a5f98..680f01ab 100644
--- a/drivers/gemaltomodem/gprs-context.c
+++ b/drivers/gemaltomodem/gprs-context.c
@@ -50,70 +50,61 @@ struct gprs_context_data {
void *cb_data;
};
-static void cgact_enable_cb(gboolean ok, GAtResult *result,
- gpointer user_data)
+static void set_gprs_context_interface(struct ofono_gprs_context *gc)
{
- struct ofono_gprs_context *gc = user_data;
- struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
struct ofono_modem *modem;
const char *interface;
- char buf[64];
+
+ modem = ofono_gprs_context_get_modem(gc);
+ interface = ofono_modem_get_string(modem, "NetworkInterface");
+ ofono_gprs_context_set_interface(gc, interface);
+}
+
+static void swwan_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+ struct ofono_gprs_context *gc = user_data;
+ struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+ struct ofono_error error;
DBG("ok %d", ok);
if (!ok) {
- struct ofono_error error;
-
+ ofono_error("Unable to activate context");
+ ofono_gprs_context_deactivated(gc, gcd->active_context);
gcd->active_context = 0;
This seems a bit strange. Are you signaling success to the core too early
-
decode_at_error(&error, g_at_result_final_response(result));
gcd->cb(&error, gcd->cb_data);
-
return;
}
-
- snprintf(buf, sizeof(buf), "AT^SWWAN=1,%u", gcd->active_context);
- g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL);
-
- modem = ofono_gprs_context_get_modem(gc);
- interface = ofono_modem_get_string(modem, "NetworkInterface");
- ofono_gprs_context_set_interface(gc, interface);
-
- /* Use DHCP */
- ofono_gprs_context_set_ipv4_address(gc, NULL, 0);
-
- CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
}
static void cgdcont_enable_cb(gboolean ok, GAtResult *result,
- gpointer user_data)
+ gpointer user_data)
nit: This is superfluous and also see doc/coding-style.txt item M4
{
struct ofono_gprs_context *gc = user_data;
struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+ struct ofono_error error;
char buf[64];
DBG("ok %d", ok);
if (!ok) {
- struct ofono_error error;
-
gcd->active_context = 0;
-
decode_at_error(&error, g_at_result_final_response(result));
gcd->cb(&error, gcd->cb_data);
-
return;
}
- snprintf(buf, sizeof(buf), "AT+CGACT=1,%u", gcd->active_context);
-
- if (g_at_chat_send(gcd->chat, buf, none_prefix,
- cgact_enable_cb, gc, NULL) == 0)
- goto error;
+ snprintf(buf, sizeof(buf), "AT^SWWAN=1,%u", gcd->active_context);
+ if (g_at_chat_send(gcd->chat, buf, none_prefix, swwan_cb, gc, NULL)) {
Note, this returns > 0 when the command has been queued, not that it has been
sent or replied to yet...
+ set_gprs_context_interface(gc);
+ /* Use DHCP */
+ ofono_gprs_context_set_ipv4_address(gc, NULL, 0);
If these modems can only do DHCP, then might be cleaner to move the 'Use DHCP'
bits into set_gprs_context_interface. And maybe rename it to make things
clearer, if needed.
- return;
+ CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
Are you sure you don't want to wait until swwan_cb to return success to the
core? AT^SWWAN can still fail...?
+ return;
+ }
-error:
CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
}
@@ -185,17 +170,39 @@ static void gemalto_gprs_deactivate_primary(struct
ofono_gprs_context *gc,
gcd->cb = cb;
gcd->cb_data = data;
- snprintf(buf, sizeof(buf), "AT+CGACT=0,%u", cid);
+ snprintf(buf, sizeof(buf), "AT^SWWAN=0,%u", gcd->active_context);
if (g_at_chat_send(gcd->chat, buf, none_prefix,
- cgact_disable_cb, gc, NULL) == 0)
- goto error;
-
- return;
+ deactivate_cb, gc, NULL))
+
[PATCH 3/5] gemalto: gprs: support automatic context activation
From: Sergey Matyukevich
Implement read_settings function to get configuration for automatic
contexts. Fix the issue uncovered by added support for automatic
context activation: do not use AT+CGACT for the contexts handled
by AT^SWWAN. As per modem specs, CGACT context can not be reused
for SWWAN. Though that worked for some reason when automatic
context was reactivated without proper deactivation.
---
drivers/gemaltomodem/gprs-context.c | 110 +++-
1 file changed, 59 insertions(+), 51 deletions(-)
diff --git a/drivers/gemaltomodem/gprs-context.c
b/drivers/gemaltomodem/gprs-context.c
index 322a5f98..680f01ab 100644
--- a/drivers/gemaltomodem/gprs-context.c
+++ b/drivers/gemaltomodem/gprs-context.c
@@ -50,70 +50,61 @@ struct gprs_context_data {
void *cb_data;
};
-static void cgact_enable_cb(gboolean ok, GAtResult *result,
- gpointer user_data)
+static void set_gprs_context_interface(struct ofono_gprs_context *gc)
{
- struct ofono_gprs_context *gc = user_data;
- struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
struct ofono_modem *modem;
const char *interface;
- char buf[64];
+
+ modem = ofono_gprs_context_get_modem(gc);
+ interface = ofono_modem_get_string(modem, "NetworkInterface");
+ ofono_gprs_context_set_interface(gc, interface);
+}
+
+static void swwan_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+ struct ofono_gprs_context *gc = user_data;
+ struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+ struct ofono_error error;
DBG("ok %d", ok);
if (!ok) {
- struct ofono_error error;
-
+ ofono_error("Unable to activate context");
+ ofono_gprs_context_deactivated(gc, gcd->active_context);
gcd->active_context = 0;
-
decode_at_error(&error, g_at_result_final_response(result));
gcd->cb(&error, gcd->cb_data);
-
return;
}
-
- snprintf(buf, sizeof(buf), "AT^SWWAN=1,%u", gcd->active_context);
- g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL);
-
- modem = ofono_gprs_context_get_modem(gc);
- interface = ofono_modem_get_string(modem, "NetworkInterface");
- ofono_gprs_context_set_interface(gc, interface);
-
- /* Use DHCP */
- ofono_gprs_context_set_ipv4_address(gc, NULL, 0);
-
- CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
}
static void cgdcont_enable_cb(gboolean ok, GAtResult *result,
- gpointer user_data)
+ gpointer user_data)
{
struct ofono_gprs_context *gc = user_data;
struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+ struct ofono_error error;
char buf[64];
DBG("ok %d", ok);
if (!ok) {
- struct ofono_error error;
-
gcd->active_context = 0;
-
decode_at_error(&error, g_at_result_final_response(result));
gcd->cb(&error, gcd->cb_data);
-
return;
}
- snprintf(buf, sizeof(buf), "AT+CGACT=1,%u", gcd->active_context);
-
- if (g_at_chat_send(gcd->chat, buf, none_prefix,
- cgact_enable_cb, gc, NULL) == 0)
- goto error;
+ snprintf(buf, sizeof(buf), "AT^SWWAN=1,%u", gcd->active_context);
+ if (g_at_chat_send(gcd->chat, buf, none_prefix, swwan_cb, gc, NULL)) {
+ set_gprs_context_interface(gc);
+ /* Use DHCP */
+ ofono_gprs_context_set_ipv4_address(gc, NULL, 0);
- return;
+ CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
+ return;
+ }
-error:
CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
}
@@ -141,34 +132,28 @@ static void gemalto_gprs_activate_primary(struct
ofono_gprs_context *gc,
snprintf(buf + len, sizeof(buf) - len - 3, ",\"%s\"", ctx->apn);
if (g_at_chat_send(gcd->chat, buf, none_prefix,
- cgdcont_enable_cb, gc, NULL) == 0)
- goto error;
-
- return;
+ cgdcont_enable_cb, gc, NULL))
+ return;
error:
CALLBACK_WITH_FAILURE(cb, data);
}
-static void cgact_disable_cb(gboolean ok, GAtResult *result,
- gpointer user_data)
+static void deactivate_cb(gboolean ok, GAtResult *result,
+ gpointer user_data)
{
struct ofono_gprs_context *gc = user_data;
struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
- char buf[64];
DBG("ok %d", ok);
+ gcd->active_context = 0;
+
if (!ok) {
CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
return;
}
- snprintf(buf, sizeof(buf), "AT^SWWAN=0,%u", gcd->active_context);
- g_at_chat_s
