[PATCH 0/3] Emulator CGDCONT

[PATCH 0/3] Emulator CGDCONT

[Olivier Guiter]

[-- Attachment #1: Type: text/plain, Size: 563 bytes --]

This first set of patches proposes an partial implementation 
of AT+CGDCONT command (for the AT Emulator), as specified in the 
dialup-command-set.txt doc file. The current code handles only
list parameters and list existing contexts.

Olivier Guiter (3):
  gprs.c: add emulator CGDCONT handler
  idmap.c: add get size function
  gprs.c: add list contexts for emulator

 src/gprs.c  |   86 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 src/idmap.c |    5 +++
 src/idmap.h |    1 +
 3 files changed, 91 insertions(+), 1 deletions(-)

[PATCH 1/3] gprs.c: add emulator CGDCONT handler

[Olivier Guiter]

[-- Attachment #1: Type: text/plain, Size: 3481 bytes --]

---
 src/gprs.c |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 63 insertions(+), 1 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index 00f6d6d..04432c3 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -90,6 +90,7 @@ struct ofono_gprs {
 	struct ofono_netreg *netreg;
 	unsigned int netreg_watch;
 	unsigned int status_watch;
+	unsigned int dun_watch;
 	GKeyFile *settings;
 	char *imsi;
 	DBusMessage *pending;
@@ -2294,7 +2295,7 @@ struct ofono_modem *ofono_gprs_context_get_modem(struct ofono_gprs_context *gc)
 }
 
 void ofono_gprs_context_set_type(struct ofono_gprs_context *gc,
-                                        enum ofono_gprs_context_type type)
+					enum ofono_gprs_context_type type)
 {
 	DBG("type %d", type);
 
@@ -2862,12 +2863,63 @@ static void provision_contexts(struct ofono_gprs *gprs, const char *mcc,
 	__ofono_gprs_provision_free_settings(settings, count);
 }
 
+/* Process the usual AT+CGDCONT command
+ */
+static void cgdcont_cb(struct ofono_emulator *em,
+			struct ofono_emulator_request *req, void *userdata)
+{
+	struct ofono_error result;
+	char buf[256];
+
+	result.error = 0;
+
+	switch (ofono_emulator_request_get_type(req)) {
+	case OFONO_EMULATOR_REQUEST_TYPE_SUPPORT:
+		/* TODO: check additionnal parameters */
+		snprintf(buf, 255, "+CGDCONT: (1-2),\"IP\",,,(0-2),(0,1,2,3,4)");
+		ofono_emulator_send_info(em, buf, FALSE);
+		result.type = OFONO_ERROR_TYPE_NO_ERROR;
+		ofono_emulator_send_final(em, &result);
+		break;
+
+	case OFONO_EMULATOR_REQUEST_TYPE_COMMAND_ONLY:
+		result.type = OFONO_ERROR_TYPE_FAILURE;
+		ofono_emulator_send_final(em, &result);
+		break;
+
+	case OFONO_EMULATOR_REQUEST_TYPE_QUERY:
+		result.type = OFONO_ERROR_TYPE_FAILURE;
+		ofono_emulator_send_final(em, &result);
+		break;
+	case OFONO_EMULATOR_REQUEST_TYPE_SET:
+		result.type = OFONO_ERROR_TYPE_FAILURE;
+		ofono_emulator_send_final(em, &result);
+		break;
+	default:
+		result.type = OFONO_ERROR_TYPE_FAILURE;
+		ofono_emulator_send_final(em, &result);
+	};
+}
+
+static void gprs_dun_watch(struct ofono_atom *atom,
+			enum ofono_atom_watch_condition cond, void *data)
+{
+	struct ofono_emulator *em = __ofono_atom_get_data(atom);
+
+	if (cond == OFONO_ATOM_WATCH_CONDITION_UNREGISTERED)
+		return;
+
+	ofono_emulator_add_handler(em, "+CGDCONT", cgdcont_cb, data, NULL);
+}
+
+
 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);
 	struct ofono_atom *netreg_atom;
+	struct ofono_atom *dun_atom;
 
 	if (gprs->contexts == NULL) /* Automatic provisioning failed */
 		add_context(gprs, NULL, OFONO_GPRS_CONTEXT_TYPE_INTERNET);
@@ -2896,6 +2948,16 @@ static void ofono_gprs_finish_register(struct ofono_gprs *gprs)
 		netreg_watch(netreg_atom,
 				OFONO_ATOM_WATCH_CONDITION_REGISTERED, gprs);
 
+	gprs->dun_watch = __ofono_modem_add_atom_watch(modem,
+						OFONO_ATOM_TYPE_EMULATOR_DUN,
+						gprs_dun_watch, gprs, NULL);
+
+	dun_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_EMULATOR_DUN);
+
+	if (dun_atom && __ofono_atom_get_registered(dun_atom))
+		gprs_dun_watch(dun_atom, OFONO_ATOM_WATCH_CONDITION_REGISTERED,
+				gprs);
+
 	__ofono_atom_register(gprs->atom, gprs_unregister);
 }
 
-- 
1.7.1

[PATCH 2/3] idmap.c: add get size function

[Olivier Guiter]

[-- Attachment #1: Type: text/plain, Size: 842 bytes --]

---
 src/idmap.c |    5 +++++
 src/idmap.h |    1 +
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/src/idmap.c b/src/idmap.c
index 6d46e8a..967adbd 100644
--- a/src/idmap.c
+++ b/src/idmap.c
@@ -200,3 +200,8 @@ unsigned int idmap_get_max(struct idmap *idmap)
 {
 	return idmap->max;
 }
+
+unsigned int idmap_get_size(struct idmap *idmap)
+{
+	return idmap->size;
+}
diff --git a/src/idmap.h b/src/idmap.h
index d6141c2..6fc29fd 100644
--- a/src/idmap.h
+++ b/src/idmap.h
@@ -30,3 +30,4 @@ unsigned int idmap_alloc_next(struct idmap *idmap, unsigned int last);
 struct idmap *idmap_new_from_range(unsigned int min, unsigned int max);
 unsigned int idmap_get_min(struct idmap *idmap);
 unsigned int idmap_get_max(struct idmap *idmap);
+unsigned int idmap_get_size(struct idmap *idmap);
-- 
1.7.1

[PATCH 3/3] gprs.c: add list contexts for emulator

[Olivier Guiter]

[-- Attachment #1: Type: text/plain, Size: 2002 bytes --]

---
 src/gprs.c |   26 ++++++++++++++++++++++++--
 1 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index 04432c3..06d52f3 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -2863,11 +2863,31 @@ static void provision_contexts(struct ofono_gprs *gprs, const char *mcc,
 	__ofono_gprs_provision_free_settings(settings, count);
 }
 
+static void ofono_gprs_list_contexts(struct ofono_emulator *em, void *userdata)
+{
+	struct ofono_gprs *gprs = userdata;
+	GSList *l;
+	char buf[256];
+
+	struct pri_context *ctx;
+
+	for (l = gprs->contexts; l; l = l->next) {
+		ctx = l->data;
+
+		snprintf(buf, 255, "+CGDCONT: %d,\"%s\",\"%s\"",
+				ctx->id, gprs_proto_to_string(ctx->context.proto),
+				ctx->context.apn);
+		ofono_emulator_send_info(em, buf, FALSE);
+	}
+}
+
 /* Process the usual AT+CGDCONT command
  */
 static void cgdcont_cb(struct ofono_emulator *em,
 			struct ofono_emulator_request *req, void *userdata)
 {
+	struct ofono_gprs *gprs = userdata;
+	struct idmap *cid = gprs->cid_map;
 	struct ofono_error result;
 	char buf[256];
 
@@ -2876,7 +2896,8 @@ static void cgdcont_cb(struct ofono_emulator *em,
 	switch (ofono_emulator_request_get_type(req)) {
 	case OFONO_EMULATOR_REQUEST_TYPE_SUPPORT:
 		/* TODO: check additionnal parameters */
-		snprintf(buf, 255, "+CGDCONT: (1-2),\"IP\",,,(0-2),(0,1,2,3,4)");
+		snprintf(buf, 255, "+CGDCONT: (1-%d),\"IP\",,,(0-2),(0,1,2,3,4)",
+					idmap_get_size(cid));
 		ofono_emulator_send_info(em, buf, FALSE);
 		result.type = OFONO_ERROR_TYPE_NO_ERROR;
 		ofono_emulator_send_final(em, &result);
@@ -2888,7 +2909,8 @@ static void cgdcont_cb(struct ofono_emulator *em,
 		break;
 
 	case OFONO_EMULATOR_REQUEST_TYPE_QUERY:
-		result.type = OFONO_ERROR_TYPE_FAILURE;
+		ofono_gprs_list_contexts(em, gprs);
+		result.type = OFONO_ERROR_TYPE_NO_ERROR;
 		ofono_emulator_send_final(em, &result);
 		break;
 	case OFONO_EMULATOR_REQUEST_TYPE_SET:
-- 
1.7.1

Re: [PATCH 3/3] gprs.c: add list contexts for emulator

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3333 bytes --]

Hi Olivier,

On 03/21/2011 08:45 AM, Olivier Guiter wrote:
> ---
>  src/gprs.c |   26 ++++++++++++++++++++++++--
>  1 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/src/gprs.c b/src/gprs.c
> index 04432c3..06d52f3 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -2863,11 +2863,31 @@ static void provision_contexts(struct ofono_gprs *gprs, const char *mcc,
>  	__ofono_gprs_provision_free_settings(settings, count);
>  }
>  
> +static void ofono_gprs_list_contexts(struct ofono_emulator *em, void *userdata)
> +{
> +	struct ofono_gprs *gprs = userdata;
> +	GSList *l;
> +	char buf[256];
> +
> +	struct pri_context *ctx;
> +
> +	for (l = gprs->contexts; l; l = l->next) {
> +		ctx = l->data;
> +
> +		snprintf(buf, 255, "+CGDCONT: %d,\"%s\",\"%s\"",
> +				ctx->id, gprs_proto_to_string(ctx->context.proto),
> +				ctx->context.apn);

So you're mixing concepts here a bit.

gprs.c has two maps:

- pid_map
- cid_map

and two ids:
- pri_context::id
- ofono_gprs_primary_context::cid

Roughly speaking the cid_map lets us pick a context id that maps 1:1 to
the modem driver.  Hence the use of ofono_gprs_set_cid_range.  Basically
the driver tells us the valid range of context ids and oFono picks the
rest.  For an AT modem the driver can simply use the cid picked by oFono
in activate_primary callback.

Internally oFono has another idmap for tracking unique ids for contexts
and ensuring some sane limit on their number.  This is managed by
pid_map and pri_context::id.

The code you have right now would use the pid_map ids...

> +		ofono_emulator_send_info(em, buf, FALSE);
> +	}
> +}
> +
>  /* Process the usual AT+CGDCONT command
>   */
>  static void cgdcont_cb(struct ofono_emulator *em,
>  			struct ofono_emulator_request *req, void *userdata)
>  {
> +	struct ofono_gprs *gprs = userdata;
> +	struct idmap *cid = gprs->cid_map;
>  	struct ofono_error result;
>  	char buf[256];
>  
> @@ -2876,7 +2896,8 @@ static void cgdcont_cb(struct ofono_emulator *em,
>  	switch (ofono_emulator_request_get_type(req)) {
>  	case OFONO_EMULATOR_REQUEST_TYPE_SUPPORT:
>  		/* TODO: check additionnal parameters */
> -		snprintf(buf, 255, "+CGDCONT: (1-2),\"IP\",,,(0-2),(0,1,2,3,4)");
> +		snprintf(buf, 255, "+CGDCONT: (1-%d),\"IP\",,,(0-2),(0,1,2,3,4)",
> +					idmap_get_size(cid));

, but use the size of the cid_map in the CGDCONT=? implementation below.
 These sizes are highly unlikely to match.

However, this brings up a more fundamental question from me.  What do
you actually want to expose here?  Do you want the emulator to be able
to view / edit contexts exposed over D-Bus?

Or is it enough to simply fake all this and maintain the cgdcont list in
memory for each emulator instance?

>  		ofono_emulator_send_info(em, buf, FALSE);
>  		result.type = OFONO_ERROR_TYPE_NO_ERROR;
>  		ofono_emulator_send_final(em, &result);
> @@ -2888,7 +2909,8 @@ static void cgdcont_cb(struct ofono_emulator *em,
>  		break;
>  
>  	case OFONO_EMULATOR_REQUEST_TYPE_QUERY:
> -		result.type = OFONO_ERROR_TYPE_FAILURE;
> +		ofono_gprs_list_contexts(em, gprs);
> +		result.type = OFONO_ERROR_TYPE_NO_ERROR;
>  		ofono_emulator_send_final(em, &result);
>  		break;
>  	case OFONO_EMULATOR_REQUEST_TYPE_SET:

Regards,
-Denis

Re: [PATCH 3/3] gprs.c: add list contexts for emulator

[Olivier Guiter]

[-- Attachment #1: Type: text/plain, Size: 3931 bytes --]

Thanks Denis for your feedback.
The idea behind this code is to propose the complete support of the 
usual AT+CGDCONT in the emulator. I think the best solution might be to 
stick on contexts exposed over DBus, but this could lead to problems 
like : a context is delete throught the DBus interface... how to handle 
this change in the AT emulator, and avoid the use of the deleted context ?
Olivier





On 03/22/2011 03:06 AM, Denis Kenzior wrote:
> Hi Olivier,
>
> On 03/21/2011 08:45 AM, Olivier Guiter wrote:
>> ---
>>   src/gprs.c |   26 ++++++++++++++++++++++++--
>>   1 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/gprs.c b/src/gprs.c
>> index 04432c3..06d52f3 100644
>> --- a/src/gprs.c
>> +++ b/src/gprs.c
>> @@ -2863,11 +2863,31 @@ static void provision_contexts(struct ofono_gprs *gprs, const char *mcc,
>>   	__ofono_gprs_provision_free_settings(settings, count);
>>   }
>>
>> +static void ofono_gprs_list_contexts(struct ofono_emulator *em, void *userdata)
>> +{
>> +	struct ofono_gprs *gprs = userdata;
>> +	GSList *l;
>> +	char buf[256];
>> +
>> +	struct pri_context *ctx;
>> +
>> +	for (l = gprs->contexts; l; l = l->next) {
>> +		ctx = l->data;
>> +
>> +		snprintf(buf, 255, "+CGDCONT: %d,\"%s\",\"%s\"",
>> +				ctx->id, gprs_proto_to_string(ctx->context.proto),
>> +				ctx->context.apn);
> So you're mixing concepts here a bit.
>
> gprs.c has two maps:
>
> - pid_map
> - cid_map
>
> and two ids:
> - pri_context::id
> - ofono_gprs_primary_context::cid
>
> Roughly speaking the cid_map lets us pick a context id that maps 1:1 to
> the modem driver.  Hence the use of ofono_gprs_set_cid_range.  Basically
> the driver tells us the valid range of context ids and oFono picks the
> rest.  For an AT modem the driver can simply use the cid picked by oFono
> in activate_primary callback.
>
> Internally oFono has another idmap for tracking unique ids for contexts
> and ensuring some sane limit on their number.  This is managed by
> pid_map and pri_context::id.
>
> The code you have right now would use the pid_map ids...
>
>> +		ofono_emulator_send_info(em, buf, FALSE);
>> +	}
>> +}
>> +
>>   /* Process the usual AT+CGDCONT command
>>    */
>>   static void cgdcont_cb(struct ofono_emulator *em,
>>   			struct ofono_emulator_request *req, void *userdata)
>>   {
>> +	struct ofono_gprs *gprs = userdata;
>> +	struct idmap *cid = gprs->cid_map;
>>   	struct ofono_error result;
>>   	char buf[256];
>>
>> @@ -2876,7 +2896,8 @@ static void cgdcont_cb(struct ofono_emulator *em,
>>   	switch (ofono_emulator_request_get_type(req)) {
>>   	case OFONO_EMULATOR_REQUEST_TYPE_SUPPORT:
>>   		/* TODO: check additionnal parameters */
>> -		snprintf(buf, 255, "+CGDCONT: (1-2),\"IP\",,,(0-2),(0,1,2,3,4)");
>> +		snprintf(buf, 255, "+CGDCONT: (1-%d),\"IP\",,,(0-2),(0,1,2,3,4)",
>> +					idmap_get_size(cid));
> , but use the size of the cid_map in the CGDCONT=? implementation below.
>   These sizes are highly unlikely to match.
>
> However, this brings up a more fundamental question from me.  What do
> you actually want to expose here?  Do you want the emulator to be able
> to view / edit contexts exposed over D-Bus?
>
> Or is it enough to simply fake all this and maintain the cgdcont list in
> memory for each emulator instance?
>
>>   		ofono_emulator_send_info(em, buf, FALSE);
>>   		result.type = OFONO_ERROR_TYPE_NO_ERROR;
>>   		ofono_emulator_send_final(em,&result);
>> @@ -2888,7 +2909,8 @@ static void cgdcont_cb(struct ofono_emulator *em,
>>   		break;
>>
>>   	case OFONO_EMULATOR_REQUEST_TYPE_QUERY:
>> -		result.type = OFONO_ERROR_TYPE_FAILURE;
>> +		ofono_gprs_list_contexts(em, gprs);
>> +		result.type = OFONO_ERROR_TYPE_NO_ERROR;
>>   		ofono_emulator_send_final(em,&result);
>>   		break;
>>   	case OFONO_EMULATOR_REQUEST_TYPE_SET:
> Regards,
> -Denis

Re: [PATCH 3/3] gprs.c: add list contexts for emulator

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1221 bytes --]

Hi Olivier,

Gentle reminder not to top-post on this mailing list.

On 03/22/2011 05:49 AM, Olivier Guiter wrote:
> Thanks Denis for your feedback.
> The idea behind this code is to propose the complete support of the
> usual AT+CGDCONT in the emulator. I think the best solution might be to
> stick on contexts exposed over DBus, but this could lead to problems
> like : a context is delete throught the DBus interface... how to handle
> this change in the AT emulator, and avoid the use of the deleted context ?

Personally I'm not convinced CGDCONT management is even a good idea.
The typical usecase is for DUN clients to send a CGDCONT string, but
this can simply be accepted and ignored (for instance).

Just overwriting oFono's context store is not a good idea anyway as you
lose certain valuable information.  For instance whether this context is
used for internet (and ConnMan should pay attention to it) or it is used
for mms, etc.

It is also possible for the dial up client to provide context details
that are identical to a context already defined in oFono.  In which case
those details should be used instead (for the reasons outlined in the
previous paragraph)

Regards,
-Denis

RE: [PATCH 3/3] gprs.c: add list contexts for emulator

[Aygon, Bertrand]

[-- Attachment #1: Type: text/plain, Size: 2186 bytes --]

Hi Denis,

>> Thanks Denis for your feedback.
>> The idea behind this code is to propose the complete support of the
>> usual AT+CGDCONT in the emulator. I think the best solution might be to
>> stick on contexts exposed over DBus, but this could lead to problems
>> like : a context is delete throught the DBus interface... how to handle
>> this change in the AT emulator, and avoid the use of the deleted context ?
>
>Personally I'm not convinced CGDCONT management is even a good idea.
>The typical usecase is for DUN clients to send a CGDCONT string, but
>this can simply be accepted and ignored (for instance).

We are not looking at DUN only, but to the 'global' AT Emulator, that will be used during GCF/PTCRB certification.
During certification, a lot of test require to set some GPRS context with specific parameters. And this cannot go 'directly' to the modem, without going through AP, because the test would not be relevant, or worse will failed, due to disconnect between oFono and modem.

>Just overwriting oFono's context store is not a good idea anyway as you
>lose certain valuable information.  For instance whether this context is
>used for internet (and ConnMan should pay attention to it) or it is used
>for mms, etc.
>
>It is also possible for the dial up client to provide context details
>that are identical to a context already defined in oFono.  In which case
>those details should be used instead (for the reasons outlined in the
>previous paragraph)

So from my point of view, all context must be in sync, in oFono side and modem side.

Regards,

Bertrand.
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 3/3] gprs.c: add list contexts for emulator

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2092 bytes --]

Hi Bertrand,

On 03/22/2011 10:17 AM, Aygon, Bertrand wrote:
> Hi Denis,
> 
>>> Thanks Denis for your feedback.
>>> The idea behind this code is to propose the complete support of the
>>> usual AT+CGDCONT in the emulator. I think the best solution might be to
>>> stick on contexts exposed over DBus, but this could lead to problems
>>> like : a context is delete throught the DBus interface... how to handle
>>> this change in the AT emulator, and avoid the use of the deleted context ?
>>
>> Personally I'm not convinced CGDCONT management is even a good idea.
>> The typical usecase is for DUN clients to send a CGDCONT string, but
>> this can simply be accepted and ignored (for instance).
> 
> We are not looking at DUN only, but to the 'global' AT Emulator, that will be used during GCF/PTCRB certification.

I know exactly what you want this for.

> During certification, a lot of test require to set some GPRS context with specific parameters. And this cannot go 'directly' to the modem, without going through AP, because the test would not be relevant, or worse will failed, due to disconnect between oFono and modem.
> 

And I know this as well ;)  However, I'm still not convinced.  oFono
simply works differently and you will encounter too many problems doing
it this way.  I listed some of them out already...

>> Just overwriting oFono's context store is not a good idea anyway as you
>> lose certain valuable information.  For instance whether this context is
>> used for internet (and ConnMan should pay attention to it) or it is used
>> for mms, etc.
>>
>> It is also possible for the dial up client to provide context details
>> that are identical to a context already defined in oFono.  In which case
>> those details should be used instead (for the reasons outlined in the
>> previous paragraph)
> 
> So from my point of view, all context must be in sync, in oFono side and modem side.

The reality is doing so is pretty much a bad idea from a technical point
of view.  So sounds like you're in for some fun ;)

Regards,
-Denis