2:46 a.m.
From: Richard Röjfors <richard(a)puffinpack.se>
There is an issue if an context gets auto activated early,
then provisioning might not have run yet for instance,
so a "new" context is created, which might be duplicated
by a provisioning context later.
So ignore the activated contexts until gprs is ready,
then it calls the driver to list active contexts.
---
include/gprs.h | 7 +++++++
src/gprs.c | 24 ++++++++++++++++++++++++
2 files changed, 31 insertions(+)
diff --git a/include/gprs.h b/include/gprs.h
index 988d6102..38e3c508 100644
--- a/include/gprs.h
+++ b/include/gprs.h
@@ -36,6 +36,10 @@ typedef void (*ofono_gprs_status_cb_t)(const struct ofono_error
*error,
typedef void (*ofono_gprs_cb_t)(const struct ofono_error *error, void *data);
+typedef void (*ofono_gprs_active_context_cb_t)(const struct ofono_error *error,
+ int cid, const char *apn,
+ void *data);
+
struct ofono_gprs_driver {
const char *name;
int (*probe)(struct ofono_gprs *gprs, unsigned int vendor,
@@ -45,6 +49,9 @@ struct ofono_gprs_driver {
ofono_gprs_cb_t cb, void *data);
void (*attached_status)(struct ofono_gprs *gprs,
ofono_gprs_status_cb_t cb, void *data);
+ void (*get_active_auto_context)(struct ofono_gprs *gprs,
+ ofono_gprs_active_context_cb_t cb,
+ void *data);
};
enum gprs_suspend_cause {
diff --git a/src/gprs.c b/src/gprs.c
index 9cb69d14..05b413c5 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -1959,6 +1959,11 @@ void ofono_gprs_cid_activated(struct ofono_gprs *gprs, unsigned
int cid,
DBG("");
+ if (!__ofono_atom_get_registered(gprs->atom)) {
+ DBG("cid %u activated before atom registered", cid);
+ return;
+ }
+
if (l_uintset_contains(gprs->used_cids, cid)) {
DBG("cid %u already activated", cid);
return;
@@ -3338,11 +3343,24 @@ remove:
storage_sync(imsi, SETTINGS_STORE, gprs->settings);
}
+static void gprs_get_active_context_callback(const struct ofono_error *error,
+ int cid, const char *apn,
+ void *data)
+{
+ struct ofono_gprs *gprs = data;
+
+ DBG("error = %d", error->type);
+
+ if (error->type == OFONO_ERROR_TYPE_NO_ERROR && apn != NULL && cid
> 0)
+ ofono_gprs_cid_activated(gprs, cid, apn);
+}
+
static void ofono_gprs_finish_register(struct ofono_gprs *gprs)
{
DBusConnection *conn = ofono_dbus_get_connection();
struct ofono_modem *modem = __ofono_atom_get_modem(gprs->atom);
const char *path = __ofono_atom_get_path(gprs->atom);
+ const struct ofono_gprs_driver *drv = gprs->driver;
if (gprs->contexts == NULL) /* Automatic provisioning failed */
add_context(gprs, NULL, OFONO_GPRS_CONTEXT_TYPE_INTERNET);
@@ -3366,6 +3384,12 @@ static void ofono_gprs_finish_register(struct ofono_gprs *gprs)
netreg_watch, gprs, NULL);
__ofono_atom_register(gprs->atom, gprs_unregister);
+
+ /* Find any context activated during init */
+ if (drv->get_active_auto_context)
+ drv->get_active_auto_context(gprs,
+ gprs_get_active_context_callback,
+ gprs);
}
static void spn_read_cb(const char *spn, const char *dc, void *data)
--
2.20.1
1:10 a.m.
Hi Richard,
On 8/11/19 11:46 AM, richard.rojfors(a)gmail.com wrote:
From: Richard Röjfors <richard(a)puffinpack.se>
There is an issue if an context gets auto activated early,
then provisioning might not have run yet for instance,
so a "new" context is created, which might be duplicated
by a provisioning context later.
So ignore the activated contexts until gprs is ready,
then it calls the driver to list active contexts.
---
include/gprs.h | 7 +++++++
src/gprs.c | 24 ++++++++++++++++++++++++
2 files changed, 31 insertions(+)
diff --git a/include/gprs.h b/include/gprs.h
index 988d6102..38e3c508 100644
--- a/include/gprs.h
+++ b/include/gprs.h
@@ -36,6 +36,10 @@ typedef void (*ofono_gprs_status_cb_t)(const struct ofono_error
*error,
typedef void (*ofono_gprs_cb_t)(const struct ofono_error *error, void *data);
+typedef void (*ofono_gprs_active_context_cb_t)(const struct ofono_error *error,
+ int cid, const char *apn,
+ void *data);
This assumes just one is active? That doesn't have to be the case...
+
struct ofono_gprs_driver {
const char *name;
int (*probe)(struct ofono_gprs *gprs, unsigned int vendor,
@@ -45,6 +49,9 @@ struct ofono_gprs_driver {
ofono_gprs_cb_t cb, void *data);
void (*attached_status)(struct ofono_gprs *gprs,
ofono_gprs_status_cb_t cb, void *data);
+ void (*get_active_auto_context)(struct ofono_gprs *gprs,
+ ofono_gprs_active_context_cb_t cb,
+ void *data);
list_active_contexts? Also, since there can in theory be multiple ones
of these, I'd do something like:
typedef void (*ofono_gprs_active_context_cb_t)(int cid, const char *apn,
void *data);
void (*list_active_contexts)(struct ofono_gprs *gprs,
ofono_gprs_active_context_cb_t active_cb,
ofono_gprs_cb_t finished_cb);
};
The include/ portion should be a separate commit.
enum gprs_suspend_cause {
diff --git a/src/gprs.c b/src/gprs.c
index 9cb69d14..05b413c5 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -1959,6 +1959,11 @@ void ofono_gprs_cid_activated(struct ofono_gprs *gprs, unsigned
int cid,
DBG("");
+ if (!__ofono_atom_get_registered(gprs->atom)) {
+ DBG("cid %u activated before atom registered", cid);
+ return;
+ }
+
if (l_uintset_contains(gprs->used_cids, cid)) {
DBG("cid %u already activated", cid);
return;
@@ -3338,11 +3343,24 @@ remove:
storage_sync(imsi, SETTINGS_STORE, gprs->settings);
}
+static void gprs_get_active_context_callback(const struct ofono_error *error,
+ int cid, const char *apn,
+ void *data)
+{
+ struct ofono_gprs *gprs = data;
+
+ DBG("error = %d", error->type);
+
+ if (error->type == OFONO_ERROR_TYPE_NO_ERROR && apn != NULL && cid
> 0)
+ ofono_gprs_cid_activated(gprs, cid, apn);
+}
+
static void ofono_gprs_finish_register(struct ofono_gprs *gprs)
{
DBusConnection *conn = ofono_dbus_get_connection();
struct ofono_modem *modem = __ofono_atom_get_modem(gprs->atom);
const char *path = __ofono_atom_get_path(gprs->atom);
+ const struct ofono_gprs_driver *drv = gprs->driver;
if (gprs->contexts == NULL) /* Automatic provisioning failed */
add_context(gprs, NULL, OFONO_GPRS_CONTEXT_TYPE_INTERNET);
@@ -3366,6 +3384,12 @@ static void ofono_gprs_finish_register(struct ofono_gprs *gprs)
netreg_watch, gprs, NULL);
__ofono_atom_register(gprs->atom, gprs_unregister);
+
+ /* Find any context activated during init */
+ if (drv->get_active_auto_context)
+ drv->get_active_auto_context(gprs,
+ gprs_get_active_context_callback,
+ gprs);
}
static void spn_read_cb(const char *spn, const char *dc, void *data)
Regards,
-Denis
1:27 a.m.
Hi Richard,
> + void (*get_active_auto_context)(struct ofono_gprs *gprs,
> + ofono_gprs_active_context_cb_t cb,
> + void *data);
list_active_contexts? Also, since there can in theory be multiple ones
of these, I'd do something like:
typedef void (*ofono_gprs_active_context_cb_t)(int cid, const char *apn,
void *data);
void (*list_active_contexts)(struct ofono_gprs *gprs,
ofono_gprs_active_context_cb_t active_cb,
ofono_gprs_cb_t finished_cb);
As an alternative to the above you can simply do:
void (*list_active_contexts)(struct ofono_gprs *gprs,
ofono_gprs_cb_t cb);
And direct the driver to issue N ofono_gprs_context_activated calls
followed by cb. Where N is the number of active contexts.
Regards,
-Denis
4:43 a.m.
Hi Denis,
Den mån 12 aug. 2019 kl 19:28 skrev Denis Kenzior <denkenz(a)gmail.com>:
Hi Richard,
>> + void (*get_active_auto_context)(struct ofono_gprs *gprs,
>> + ofono_gprs_active_context_cb_t cb,
>> + void *data);
>
> list_active_contexts? Also, since there can in theory be multiple ones
> of these, I'd do something like:
>
> typedef void (*ofono_gprs_active_context_cb_t)(int cid, const char *apn,
> void *data);
>
> void (*list_active_contexts)(struct ofono_gprs *gprs,
> ofono_gprs_active_context_cb_t active_cb,
> ofono_gprs_cb_t finished_cb);
>
As an alternative to the above you can simply do:
void (*list_active_contexts)(struct ofono_gprs *gprs,
ofono_gprs_cb_t cb);
And direct the driver to issue N ofono_gprs_context_activated calls
followed by cb. Where N is the number of active contexts.
Makes more sense, then there is no need for a "strange" call with -1 and
NULL.
Regards,
-Denis
--Richard
4:41 a.m.
Hi Denis,
Den mån 12 aug. 2019 kl 19:10 skrev Denis Kenzior <denkenz(a)gmail.com>:
Hi Richard,
On 8/11/19 11:46 AM, richard.rojfors(a)gmail.com wrote:
> From: Richard Röjfors <richard(a)puffinpack.se>
>
> There is an issue if an context gets auto activated early,
> then provisioning might not have run yet for instance,
> so a "new" context is created, which might be duplicated
> by a provisioning context later.
> So ignore the activated contexts until gprs is ready,
> then it calls the driver to list active contexts.
> ---
> include/gprs.h | 7 +++++++
> src/gprs.c | 24 ++++++++++++++++++++++++
> 2 files changed, 31 insertions(+)
>
> diff --git a/include/gprs.h b/include/gprs.h
> index 988d6102..38e3c508 100644
> --- a/include/gprs.h
> +++ b/include/gprs.h
> @@ -36,6 +36,10 @@ typedef void (*ofono_gprs_status_cb_t)(const struct
ofono_error *error,
>
> typedef void (*ofono_gprs_cb_t)(const struct ofono_error *error, void
*data);
>
> +typedef void (*ofono_gprs_active_context_cb_t)(const struct ofono_error
*error,
> + int cid, const char *apn,
> + void *data);
This assumes just one is active? That doesn't have to be the case...
Well I was thinking of that. But can more that one get auto activated?
But I agree its more generic to support more than one. Then it could
be used in other cases too.
> +
> struct ofono_gprs_driver {
> const char *name;
> int (*probe)(struct ofono_gprs *gprs, unsigned int vendor,
> @@ -45,6 +49,9 @@ struct ofono_gprs_driver {
> ofono_gprs_cb_t cb, void *data);
> void (*attached_status)(struct ofono_gprs *gprs,
> ofono_gprs_status_cb_t cb, void
*data);
> + void (*get_active_auto_context)(struct ofono_gprs *gprs,
> + ofono_gprs_active_context_cb_t cb,
> + void *data);
list_active_contexts? Also, since there can in theory be multiple ones
of these, I'd do something like:
typedef void (*ofono_gprs_active_context_cb_t)(int cid, const char *apn,
void *data);
void (*list_active_contexts)(struct ofono_gprs *gprs,
ofono_gprs_active_context_cb_t active_cb,
ofono_gprs_cb_t finished_cb);
Nice solution, I assume we call the callback multiple times.
And if none is found
we still call it once with -1 and null, right?
> };
>
The include/ portion should be a separate commit.
> enum gprs_suspend_cause {
> diff --git a/src/gprs.c b/src/gprs.c
> index 9cb69d14..05b413c5 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -1959,6 +1959,11 @@ void ofono_gprs_cid_activated(struct ofono_gprs
*gprs, unsigned int cid,
>
> DBG("");
>
> + if (!__ofono_atom_get_registered(gprs->atom)) {
> + DBG("cid %u activated before atom registered", cid);
> + return;
> + }
> +
> if (l_uintset_contains(gprs->used_cids, cid)) {
> DBG("cid %u already activated", cid);
> return;
> @@ -3338,11 +3343,24 @@ remove:
> storage_sync(imsi, SETTINGS_STORE, gprs->settings);
> }
>
> +static void gprs_get_active_context_callback(const struct ofono_error
*error,
> + int cid, const char *apn,
> + void *data)
> +{
> + struct ofono_gprs *gprs = data;
> +
> + DBG("error = %d", error->type);
> +
> + if (error->type == OFONO_ERROR_TYPE_NO_ERROR && apn != NULL
&& cid
> 0)
> + ofono_gprs_cid_activated(gprs, cid, apn);
> +}
> +
> static void ofono_gprs_finish_register(struct ofono_gprs *gprs)
> {
> DBusConnection *conn = ofono_dbus_get_connection();
> struct ofono_modem *modem = __ofono_atom_get_modem(gprs->atom);
> const char *path = __ofono_atom_get_path(gprs->atom);
> + const struct ofono_gprs_driver *drv = gprs->driver;
>
> if (gprs->contexts == NULL) /* Automatic provisioning failed */
> add_context(gprs, NULL, OFONO_GPRS_CONTEXT_TYPE_INTERNET);
> @@ -3366,6 +3384,12 @@ static void ofono_gprs_finish_register(struct
ofono_gprs *gprs)
> netreg_watch, gprs, NULL);
>
> __ofono_atom_register(gprs->atom, gprs_unregister);
> +
> + /* Find any context activated during init */
> + if (drv->get_active_auto_context)
> + drv->get_active_auto_context(gprs,
> +
gprs_get_active_context_callback,
> + gprs);
> }
>
> static void spn_read_cb(const char *spn, const char *dc, void *data)
>
Regards,
-Denis
5:58 a.m.
Hi Richard,
list_active_contexts? Also, since there can in theory be
multiple ones
of these, I'd do something like:
typedef void (*ofono_gprs_active_context_cb_t)(int cid, const char *apn,
void *data);
void (*list_active_contexts)(struct ofono_gprs *gprs,
ofono_gprs_active_context_cb_t active_cb,
ofono_gprs_cb_t finished_cb);
Nice solution, I assume we call the callback multiple times.
And if none is found we still call it once with -1 and null, right?
Well, that is what the finished_cb is for. But I guess you liked the
other solution better. They're equivalent in the end.
Regards,
-Denis
2:46 a.m.
New subject: [PATCH 2/2] atmodem: Implement the get_active_auto_context callback
From: Richard Röjfors <richard(a)puffinpack.se>
The callback calls cgact and cgdcont to get information
regarding any auto activated context.
---
drivers/atmodem/gprs.c | 115 +++++++++++++++++++++++++++++++----------
1 file changed, 88 insertions(+), 27 deletions(-)
diff --git a/drivers/atmodem/gprs.c b/drivers/atmodem/gprs.c
index 10108281..f08dc00c 100644
--- a/drivers/atmodem/gprs.c
+++ b/drivers/atmodem/gprs.c
@@ -146,27 +146,11 @@ static void at_gprs_registration_status(struct ofono_gprs *gprs,
CALLBACK_WITH_FAILURE(cb, -1, data);
}
-static void at_cgdcont_read_cb(gboolean ok, GAtResult *result,
- gpointer user_data)
+static const char *at_cgdcont_parse(GAtResult *result, int cid)
{
- struct ofono_gprs *gprs = user_data;
- struct gprs_data *gd = ofono_gprs_get_data(gprs);
- int activated_cid = gd->last_auto_context_id;
const char *apn = NULL;
GAtResultIter iter;
- DBG("ok %d", ok);
-
- if (!ok) {
- ofono_warn("Can't read CGDCONT contexts.");
- return;
- }
-
- if (gd->last_auto_context_id == -1) {
- DBG("Context got deactivated while calling CGDCONT");
- return;
- }
-
g_at_result_iter_init(&iter, result);
while (g_at_result_iter_next(&iter, "+CGDCONT:")) {
@@ -175,7 +159,7 @@ static void at_cgdcont_read_cb(gboolean ok, GAtResult *result,
if (!g_at_result_iter_next_number(&iter, &read_cid))
break;
- if (read_cid != activated_cid)
+ if (read_cid != cid)
continue;
/* ignore protocol */
@@ -186,6 +170,31 @@ static void at_cgdcont_read_cb(gboolean ok, GAtResult *result,
break;
}
+ return apn;
+}
+
+static void at_cgdcont_read_cb(gboolean ok, GAtResult *result,
+ gpointer user_data)
+{
+ struct ofono_gprs *gprs = user_data;
+ struct gprs_data *gd = ofono_gprs_get_data(gprs);
+ int activated_cid = gd->last_auto_context_id;
+ const char *apn = NULL;
+
+ DBG("ok %d", ok);
+
+ if (!ok) {
+ ofono_warn("Can't read CGDCONT contexts.");
+ return;
+ }
+
+ if (activated_cid == -1) {
+ DBG("Context got deactivated while calling CGDCONT");
+ return;
+ }
+
+ apn = at_cgdcont_parse(result, activated_cid);
+
if (apn)
ofono_gprs_cid_activated(gprs, activated_cid, apn);
else
@@ -193,16 +202,46 @@ static void at_cgdcont_read_cb(gboolean ok, GAtResult *result,
activated_cid);
}
+static void at_cgdcont_act_read_cb(gboolean ok, GAtResult *result,
+ gpointer user_data)
+{
+ struct cb_data *cbd = user_data;
+ struct gprs_data *gd = cbd->user;
+ ofono_gprs_active_context_cb_t cb = cbd->cb;
+ struct ofono_error error;
+ int activated_cid = gd->last_auto_context_id;
+ const char *apn = NULL;
+
+ decode_at_error(&error, g_at_result_final_response(result));
+
+ if (!ok) {
+ ofono_warn("Can't read CGDCONT contexts.");
+
+ activated_cid = -1;
+ } else if (activated_cid != -1) {
+ apn = at_cgdcont_parse(result, activated_cid);
+ }
+
+ cb(&error, activated_cid, apn, cbd->data);
+}
+
static void at_cgact_cb(gboolean ok, GAtResult *result, gpointer user_data)
{
- struct ofono_gprs *gprs = user_data;
- struct gprs_data *gd = ofono_gprs_get_data(gprs);
+ struct cb_data *cbd = user_data;
+ struct gprs_data *gd = cbd->user;
+ ofono_gprs_active_context_cb_t cb = cbd->cb;
+ struct ofono_error error;
GAtResultIter iter;
- DBG("ok %d", ok);
+ decode_at_error(&error, g_at_result_final_response(result));
if (!ok) {
ofono_warn("Can't read CGACT contexts.");
+
+ cb(&error, -1, NULL, cbd->data);
+
+ g_free(cbd);
+
return;
}
@@ -222,17 +261,42 @@ static void at_cgact_cb(gboolean ok, GAtResult *result, gpointer
user_data)
continue;
/* Flag this as auto context as it was obviously active */
- if (gd->last_auto_context_id == 0)
+ if (gd->last_auto_context_id == -1)
gd->last_auto_context_id = read_cid;
if (read_cid != gd->last_auto_context_id)
continue;
g_at_chat_send(gd->chat, "AT+CGDCONT?", cgdcont_prefix,
- at_cgdcont_read_cb, gprs, NULL);
+ at_cgdcont_act_read_cb, cbd, g_free);
break;
}
+
+ if (gd->last_auto_context_id == -1) {
+ /* No error, but no auto activated contexts... */
+ cb(&error, -1, NULL, cbd->data);
+
+ g_free(cbd);
+ }
+}
+
+static void at_gprs_get_active_auto_context(struct ofono_gprs *gprs,
+ ofono_gprs_active_context_cb_t cb,
+ void *data)
+{
+ struct gprs_data *gd = ofono_gprs_get_data(gprs);
+ struct cb_data *cbd = cb_data_new(cb, data);
+
+ cbd->user = gd;
+
+ if (g_at_chat_send(gd->chat, "AT+CGACT?", cgact_prefix,
+ at_cgact_cb, cbd, NULL))
+ return;
+
+ g_free(cbd);
+
+ CALLBACK_WITH_FAILURE(cb, -1, NULL, data);
}
static void cgreg_notify(GAtResult *result, gpointer user_data)
@@ -538,10 +602,6 @@ static void gprs_initialized(gboolean ok, GAtResult *result, gpointer
user_data)
break;
}
- /* Check if there is any already activated contexts at init */
- g_at_chat_send(gd->chat, "AT+CGACT?", cgact_prefix,
- at_cgact_cb, gprs, NULL);
-
ofono_gprs_register(gprs);
}
@@ -705,6 +765,7 @@ static const struct ofono_gprs_driver driver = {
.remove = at_gprs_remove,
.set_attached = at_gprs_set_attached,
.attached_status = at_gprs_registration_status,
+ .get_active_auto_context= at_gprs_get_active_auto_context,
};
void at_gprs_init(void)
--
2.20.1
1133
days inactive
1134
days old
6 comments
3 participants
participants (3)
-
Denis Kenzior -
Richard Röjfors -
richard.rojfors＠gmail.com