[PATCH 1/4] Define packet switched bearers

[PATCH 1/4] Define packet switched bearers

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

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

---
 src/common.c |   14 ++++++++++++++
 src/common.h |   13 +++++++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/src/common.c b/src/common.c
index d4e567b..6664007 100644
--- a/src/common.c
+++ b/src/common.c
@@ -714,6 +714,20 @@ const char *registration_tech_to_string(int tech)
 	}
 }
 
+const char *packet_bearer_to_string(int bearer)
+{
+	static const char list[][6] = {
+		"none",
+		"gsm", "edge",
+		"umts", "hsupa", "hsdpa", "hspa",
+		"lte",
+	};
+
+	if (((unsigned)bearer) < sizeof (list) / sizeof (list[0]))
+		return list[bearer];
+	return "unknown";
+}
+
 gboolean is_valid_apn(const char *apn)
 {
 	int i;
diff --git a/src/common.h b/src/common.h
index 64f297e..d11cd8e 100644
--- a/src/common.h
+++ b/src/common.h
@@ -87,6 +87,18 @@ enum bearer_class {
 	BEARER_CLASS_PAD = 128
 };
 
+/* 27.007 Section 7.29 */
+enum packet_bearer {
+	PACKET_BEARER_NONE = 0,
+	PACKET_BEARER_GPRS = 1,
+	PACKET_BEARER_EDGE = 2,
+	PACKET_BEARER_UMTS = 3,
+	PACKET_BEARER_HSUPA = 4,
+	PACKET_BEARER_HSDPA = 5,
+	PACKET_BEARER_HSUPA_HSDPA = 6,
+	PACKET_BEARER_LTE = 7,
+};
+
 /* 22.030 Section 6.5.2 */
 enum ss_control_type {
 	SS_CONTROL_TYPE_ACTIVATION,
@@ -158,5 +170,6 @@ gboolean is_valid_pin(const char *pin, enum pin_type type);
 
 const char *registration_status_to_string(int status);
 const char *registration_tech_to_string(int tech);
+const char *packet_bearer_to_string(int bearer);
 
 gboolean is_valid_apn(const char *apn);
-- 
1.7.1

[PATCH 2/4] Core support for packet switched bearer reporting

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

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

---
 include/gprs.h |    2 ++
 src/gprs.c     |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/include/gprs.h b/include/gprs.h
index 1c1d116..cd62f2f 100644
--- a/include/gprs.h
+++ b/include/gprs.h
@@ -59,6 +59,8 @@ void ofono_gprs_status_notify(struct ofono_gprs *gprs, int status, int tech);
 void ofono_gprs_detached_notify(struct ofono_gprs *gprs);
 void ofono_gprs_suspend_notify(struct ofono_gprs *gprs, int cause);
 void ofono_gprs_resume_notify(struct ofono_gprs *gprs);
+void ofono_gprs_bearer_notify(struct ofono_gprs *gprs,
+				unsigned int cid, int bearer);
 
 int ofono_gprs_driver_register(const struct ofono_gprs_driver *d);
 void ofono_gprs_driver_unregister(const struct ofono_gprs_driver *d);
diff --git a/src/gprs.c b/src/gprs.c
index bd52676..3d4b1a8 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -107,6 +107,7 @@ struct context_settings {
 
 struct pri_context {
 	ofono_bool_t active;
+	int bearer;
 	enum ofono_gprs_context_type type;
 	char name[MAX_CONTEXT_NAME_LENGTH + 1];
 	char message_proxy[MAX_MESSAGE_PROXY_LENGTH + 1];
@@ -596,6 +597,13 @@ static void append_context_properties(struct pri_context *ctx,
 	value = ctx->active;
 	ofono_dbus_dict_append(dict, "Active", DBUS_TYPE_BOOLEAN, &value);
 
+	if (ctx->active) {
+		const char *bearer = packet_bearer_to_string(ctx->bearer);
+
+		ofono_dbus_dict_append(dict, "Bearer",
+					DBUS_TYPE_STRING, &bearer);
+	}
+
 	ofono_dbus_dict_append(dict, "Type", DBUS_TYPE_STRING, &type);
 
 	ofono_dbus_dict_append(dict, "Protocol", DBUS_TYPE_STRING, &proto);
@@ -1591,6 +1599,7 @@ static struct pri_context *add_context(struct ofono_gprs *gprs,
 		return NULL;
 	}
 
+	context->bearer = -1;
 	context->id = id;
 
 	DBG("Registering new context");
@@ -2001,6 +2010,29 @@ void ofono_gprs_add_context(struct ofono_gprs *gprs,
 	__ofono_atom_register(gc->atom, gprs_context_unregister);
 }
 
+void ofono_gprs_bearer_notify(struct ofono_gprs *gprs,
+				unsigned int cid, int bearer)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	GSList *l;
+	const char *value = packet_bearer_to_string(bearer);
+
+	for (l = gprs->contexts; l; l = l->next) {
+		struct pri_context *ctx = l->data;
+
+		if (ctx->context.cid != cid)
+			continue;
+
+		if (ctx->bearer == bearer)
+			continue;
+
+		ctx->bearer = bearer;
+		ofono_dbus_signal_property_changed(conn, ctx->path,
+					OFONO_CONNECTION_CONTEXT_INTERFACE,
+					"Bearer", DBUS_TYPE_STRING, &value);
+	}
+}
+
 void ofono_gprs_context_deactivated(struct ofono_gprs_context *gc,
 					unsigned int cid)
 {
-- 
1.7.1

[PATCH 3/4] Bearer documentation

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

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

---
 doc/connman-api.txt |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/doc/connman-api.txt b/doc/connman-api.txt
index b13efd1..18185d7 100644
--- a/doc/connman-api.txt
+++ b/doc/connman-api.txt
@@ -150,6 +150,16 @@ Properties	boolean Active [readwrite]
 			Holds whether the context is activated.  This value
 			can be set to activate / deactivate the context.
 
+		string Bearer [readonly, optional]
+
+			Contains the current packet switched bearer of the
+			connection context.
+
+			Possible values are the same as for the Technology
+			property the NetworkRegistration object, plus
+			"none" if no data activity is ongoing, or "unknown"
+			if not known.
+
 		string AccessPointName [readwrite]
 
 			Holds the name of the access point.  This is
-- 
1.7.1

[PATCH 4/4] atmodem: packet switch bearer support

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

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

---
 drivers/atmodem/gprs.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/drivers/atmodem/gprs.c b/drivers/atmodem/gprs.c
index a38c6b4..1d4a8bf 100644
--- a/drivers/atmodem/gprs.c
+++ b/drivers/atmodem/gprs.c
@@ -200,6 +200,54 @@ static void xdatastat_notify(GAtResult *result, gpointer user_data)
 	}
 }
 
+static void cpsb_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_gprs *gprs = user_data;
+	GAtResultIter iter;
+	gint cid, bearer;
+
+	g_at_result_iter_init(&iter, result);
+	if (!g_at_result_iter_next(&iter, "+CPSB:"))
+		return;
+	if (!g_at_result_iter_next_number(&iter, &cid))
+		return;
+	if (!g_at_result_iter_next_number(&iter, &bearer))
+		return;
+
+	ofono_gprs_bearer_notify(gprs, cid, bearer);
+}
+
+static void cpsb_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_gprs *gprs = user_data;
+	GAtResultIter iter;
+	gint cid, bearer;
+
+	if (!ok)
+		return;
+
+	g_at_result_iter_init(&iter, result);
+	if (!g_at_result_iter_next(&iter, "+CPSB:"))
+		return;
+
+	g_at_result_iter_next_number(&iter, NULL);
+	while (g_at_result_iter_next_number(&iter, &cid) &&
+			g_at_result_iter_next_number(&iter, &bearer))
+		ofono_gprs_bearer_notify(gprs, cid, bearer);
+}
+
+static void cpsb_set_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_gprs *gprs = user_data;
+	struct gprs_data *gd = ofono_gprs_get_data(gprs);
+
+	if (!ok)
+		return;
+
+	g_at_chat_register(gd->chat, "+CPSB:", cpsb_notify, FALSE, gprs, NULL);
+	g_at_chat_send(gd->chat, "AT+CPSB?", none_prefix, cpsb_cb, gprs, NULL);
+}
+
 static void gprs_initialized(gboolean ok, GAtResult *result, gpointer user_data)
 {
 	struct ofono_gprs *gprs = user_data;
@@ -208,6 +256,8 @@ static void gprs_initialized(gboolean ok, GAtResult *result, gpointer user_data)
 	g_at_chat_register(gd->chat, "+CGEV:", cgev_notify, FALSE, gprs, NULL);
 	g_at_chat_register(gd->chat, "+CGREG:", cgreg_notify,
 					FALSE, gprs, NULL);
+	g_at_chat_send(gd->chat, "AT+CPSB=1", none_prefix,
+		cpsb_set_cb, gprs, NULL);
 
 	switch (gd->vendor) {
 	case OFONO_VENDOR_IFX:
-- 
1.7.1

Re: [PATCH 3/4] Bearer documentation

[Denis Kenzior]

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

Hi Rémi,

On 01/07/2011 10:02 AM, Rémi Denis-Courmont wrote:
> ---
>  doc/connman-api.txt |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/doc/connman-api.txt b/doc/connman-api.txt
> index b13efd1..18185d7 100644
> --- a/doc/connman-api.txt
> +++ b/doc/connman-api.txt
> @@ -150,6 +150,16 @@ Properties	boolean Active [readwrite]
>  			Holds whether the context is activated.  This value
>  			can be set to activate / deactivate the context.
>  
> +		string Bearer [readonly, optional]
> +
> +			Contains the current packet switched bearer of the
> +			connection context.
> +
> +			Possible values are the same as for the Technology
> +			property the NetworkRegistration object, plus
> +			"none" if no data activity is ongoing, or "unknown"
> +			if not known.
> +

Please remove this "unknown" bit.  The property is already optional, so
if it is never reported by the driver we shouldn't report it up, even if
the context is active.

The other question I have is whether we should report this at the
context level or at the ConnectionManager level.  Can different contexts
truly have different bearers?

>  		string AccessPointName [readwrite]
>  
>  			Holds the name of the access point.  This is

Regards,
-Denis

Re: [PATCH 1/4] Define packet switched bearers

[Denis Kenzior]

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

Hi Rémi,

On 01/07/2011 10:02 AM, Rémi Denis-Courmont wrote:
> ---
>  src/common.c |   14 ++++++++++++++
>  src/common.h |   13 +++++++++++++
>  2 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/src/common.c b/src/common.c
> index d4e567b..6664007 100644
> --- a/src/common.c
> +++ b/src/common.c
> @@ -714,6 +714,20 @@ const char *registration_tech_to_string(int tech)
>  	}
>  }
>  
> +const char *packet_bearer_to_string(int bearer)
> +{
> +	static const char list[][6] = {
> +		"none",
> +		"gsm", "edge",
> +		"umts", "hsupa", "hsdpa", "hspa",
> +		"lte",
> +	};
> +
> +	if (((unsigned)bearer) < sizeof (list) / sizeof (list[0]))
> +		return list[bearer];
> +	return "unknown";

doc/coding-style.txt rule M1

Also, please drop the unknown bit and return ""

> +}
> +
>  gboolean is_valid_apn(const char *apn)
>  {
>  	int i;
> diff --git a/src/common.h b/src/common.h
> index 64f297e..d11cd8e 100644
> --- a/src/common.h
> +++ b/src/common.h
> @@ -87,6 +87,18 @@ enum bearer_class {
>  	BEARER_CLASS_PAD = 128
>  };
>  
> +/* 27.007 Section 7.29 */
> +enum packet_bearer {
> +	PACKET_BEARER_NONE = 0,
> +	PACKET_BEARER_GPRS = 1,
> +	PACKET_BEARER_EDGE = 2,
> +	PACKET_BEARER_UMTS = 3,
> +	PACKET_BEARER_HSUPA = 4,
> +	PACKET_BEARER_HSDPA = 5,
> +	PACKET_BEARER_HSUPA_HSDPA = 6,
> +	PACKET_BEARER_LTE = 7,
> +};
> +

doc/coding-style.txt rule M11

>  /* 22.030 Section 6.5.2 */
>  enum ss_control_type {
>  	SS_CONTROL_TYPE_ACTIVATION,
> @@ -158,5 +170,6 @@ gboolean is_valid_pin(const char *pin, enum pin_type type);
>  
>  const char *registration_status_to_string(int status);
>  const char *registration_tech_to_string(int tech);
> +const char *packet_bearer_to_string(int bearer);
>  
>  gboolean is_valid_apn(const char *apn);

Regards,
-Denis

Re: [PATCH 2/4] Core support for packet switched bearer reporting

[Denis Kenzior]

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

Hi Rémi,

On 01/07/2011 10:02 AM, Rémi Denis-Courmont wrote:
> ---
>  include/gprs.h |    2 ++
>  src/gprs.c     |   32 ++++++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 0 deletions(-)
> 

please see the 'Submitting patches' section in HACKING

> diff --git a/include/gprs.h b/include/gprs.h
> index 1c1d116..cd62f2f 100644
> --- a/include/gprs.h
> +++ b/include/gprs.h
> @@ -59,6 +59,8 @@ void ofono_gprs_status_notify(struct ofono_gprs *gprs, int status, int tech);
>  void ofono_gprs_detached_notify(struct ofono_gprs *gprs);
>  void ofono_gprs_suspend_notify(struct ofono_gprs *gprs, int cause);
>  void ofono_gprs_resume_notify(struct ofono_gprs *gprs);
> +void ofono_gprs_bearer_notify(struct ofono_gprs *gprs,
> +				unsigned int cid, int bearer);
>  
>  int ofono_gprs_driver_register(const struct ofono_gprs_driver *d);
>  void ofono_gprs_driver_unregister(const struct ofono_gprs_driver *d);
> diff --git a/src/gprs.c b/src/gprs.c
> index bd52676..3d4b1a8 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -107,6 +107,7 @@ struct context_settings {
>  
>  struct pri_context {
>  	ofono_bool_t active;
> +	int bearer;
>  	enum ofono_gprs_context_type type;
>  	char name[MAX_CONTEXT_NAME_LENGTH + 1];
>  	char message_proxy[MAX_MESSAGE_PROXY_LENGTH + 1];
> @@ -596,6 +597,13 @@ static void append_context_properties(struct pri_context *ctx,
>  	value = ctx->active;
>  	ofono_dbus_dict_append(dict, "Active", DBUS_TYPE_BOOLEAN, &value);
>  
> +	if (ctx->active) {
> +		const char *bearer = packet_bearer_to_string(ctx->bearer);
> +
> +		ofono_dbus_dict_append(dict, "Bearer",
> +					DBUS_TYPE_STRING, &bearer);
> +	}
> +
>  	ofono_dbus_dict_append(dict, "Type", DBUS_TYPE_STRING, &type);
>  
>  	ofono_dbus_dict_append(dict, "Protocol", DBUS_TYPE_STRING, &proto);
> @@ -1591,6 +1599,7 @@ static struct pri_context *add_context(struct ofono_gprs *gprs,
>  		return NULL;
>  	}
>  
> +	context->bearer = -1;

You might also want to reset the bearer to -1 when the context is
deactivated.

>  	context->id = id;
>  
>  	DBG("Registering new context");
> @@ -2001,6 +2010,29 @@ void ofono_gprs_add_context(struct ofono_gprs *gprs,
>  	__ofono_atom_register(gc->atom, gprs_context_unregister);
>  }
>  
> +void ofono_gprs_bearer_notify(struct ofono_gprs *gprs,
> +				unsigned int cid, int bearer)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	GSList *l;
> +	const char *value = packet_bearer_to_string(bearer);

Why bother running this operation before you're sure you're going to
emit the signal?  Please move this down.

> +
> +	for (l = gprs->contexts; l; l = l->next) {
> +		struct pri_context *ctx = l->data;
> +
> +		if (ctx->context.cid != cid)
> +			continue;
> +
> +		if (ctx->bearer == bearer)
> +			continue;

Shouldn't this be return instead of continue?

> +
> +		ctx->bearer = bearer;
> +		ofono_dbus_signal_property_changed(conn, ctx->path,
> +					OFONO_CONNECTION_CONTEXT_INTERFACE,
> +					"Bearer", DBUS_TYPE_STRING, &value);
> +	}
> +}
> +
>  void ofono_gprs_context_deactivated(struct ofono_gprs_context *gc,
>  					unsigned int cid)
>  {

Regards,
-Denis

Re: [PATCH 4/4] atmodem: packet switch bearer support

[Denis Kenzior]

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

Hi Rémi,

On 01/07/2011 10:02 AM, Rémi Denis-Courmont wrote:
> ---
>  drivers/atmodem/gprs.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 50 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/atmodem/gprs.c b/drivers/atmodem/gprs.c
> index a38c6b4..1d4a8bf 100644
> --- a/drivers/atmodem/gprs.c
> +++ b/drivers/atmodem/gprs.c
> @@ -200,6 +200,54 @@ static void xdatastat_notify(GAtResult *result, gpointer user_data)
>  	}
>  }
>  
> +static void cpsb_notify(GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_gprs *gprs = user_data;
> +	GAtResultIter iter;
> +	gint cid, bearer;
> +
> +	g_at_result_iter_init(&iter, result);
> +	if (!g_at_result_iter_next(&iter, "+CPSB:"))
> +		return;

doc/coding-style.txt Rule M1

> +	if (!g_at_result_iter_next_number(&iter, &cid))
> +		return;

doc/coding-style.txt Rule M1

> +	if (!g_at_result_iter_next_number(&iter, &bearer))
> +		return;
> +
> +	ofono_gprs_bearer_notify(gprs, cid, bearer);
> +}
> +
> +static void cpsb_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_gprs *gprs = user_data;
> +	GAtResultIter iter;
> +	gint cid, bearer;
> +
> +	if (!ok)
> +		return;
> +
> +	g_at_result_iter_init(&iter, result);
> +	if (!g_at_result_iter_next(&iter, "+CPSB:"))
> +		return;

doc/coding-style.txt Rule M1

> +
> +	g_at_result_iter_next_number(&iter, NULL);
> +	while (g_at_result_iter_next_number(&iter, &cid) &&
> +			g_at_result_iter_next_number(&iter, &bearer))
> +		ofono_gprs_bearer_notify(gprs, cid, bearer);

doc/coding-style.txt Rule M1

> +}
> +
> +static void cpsb_set_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_gprs *gprs = user_data;
> +	struct gprs_data *gd = ofono_gprs_get_data(gprs);
> +
> +	if (!ok)
> +		return;
> +
> +	g_at_chat_register(gd->chat, "+CPSB:", cpsb_notify, FALSE, gprs, NULL);
> +	g_at_chat_send(gd->chat, "AT+CPSB?", none_prefix, cpsb_cb, gprs, NULL);

Why do you bother running CPSB query?  No context is active at this point.

> +}
> +
>  static void gprs_initialized(gboolean ok, GAtResult *result, gpointer user_data)
>  {
>  	struct ofono_gprs *gprs = user_data;
> @@ -208,6 +256,8 @@ static void gprs_initialized(gboolean ok, GAtResult *result, gpointer user_data)
>  	g_at_chat_register(gd->chat, "+CGEV:", cgev_notify, FALSE, gprs, NULL);
>  	g_at_chat_register(gd->chat, "+CGREG:", cgreg_notify,
>  					FALSE, gprs, NULL);
> +	g_at_chat_send(gd->chat, "AT+CPSB=1", none_prefix,
> +		cpsb_set_cb, gprs, NULL);
>  
>  	switch (gd->vendor) {
>  	case OFONO_VENDOR_IFX:

Regards,
-Denis

Re: [PATCH 2/4] Core support for packet switched bearer reporting

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

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

On Friday 07 January 2011 18:25:32 ext Denis Kenzior, you wrote:
> > @@ -2001,6 +2010,29 @@ void ofono_gprs_add_context(struct ofono_gprs
> > *gprs,
> > 
> >  	__ofono_atom_register(gc->atom, gprs_context_unregister);
> >  
> >  }
> > 
> > +void ofono_gprs_bearer_notify(struct ofono_gprs *gprs,
> > +				unsigned int cid, int bearer)
> > +{
> > +	DBusConnection *conn = ofono_dbus_get_connection();
> > +	GSList *l;
> > +	const char *value = packet_bearer_to_string(bearer);
> 
> Why bother running this operation before you're sure you're going to
> emit the signal?

Now that we're talking about network-initiated LTE bearers, I would rather be 
safe than sorry. I wouldn't assume that the modem will signal the CPSB 
(unchanged) value later in this case.

> Please move this down.

To avoid any race, we have to query the existing CPSB values after we enable 
the CPSB subscription, right? We could surely move that to a later stage, but 
I don't see the benefit.

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki

Re: [PATCH 3/4] Bearer documentation

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

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

On Friday 07 January 2011 18:23:25 ext Denis Kenzior, you wrote:
> The other question I have is whether we should report this at the
> context level or at the ConnectionManager level.  Can different contexts
> truly have different bearers?

At least for 3G with HS*PA, I believe we can. In my understanding, HSPA 
channels are allocated per contexts. Correct me if I'm wrong.

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki

Re: [PATCH 2/4] Core support for packet switched bearer reporting

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

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

On Friday 07 January 2011 18:25:32 ext Denis Kenzior, you wrote:
> Hi Rémi,
> 
> On 01/07/2011 10:02 AM, Rémi Denis-Courmont wrote:
> > ---
> > 
> >  include/gprs.h |    2 ++
> >  src/gprs.c     |   32 ++++++++++++++++++++++++++++++++
> >  2 files changed, 34 insertions(+), 0 deletions(-)
> 
> please see the 'Submitting patches' section in HACKING

The rules are self-contradictory. Either the patch spreads multiple 
directories, or it breaks compilation.

For the sake of git-bisect'ing, I'd rather not break compilation.

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki

Re: [PATCH 3/4] Bearer documentation

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

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

On Friday 07 January 2011 18:23:25 ext Denis Kenzior, you wrote:
> Hi Rémi,
> 
> On 01/07/2011 10:02 AM, Rémi Denis-Courmont wrote:
> > ---
> > 
> >  doc/connman-api.txt |   10 ++++++++++
> >  1 files changed, 10 insertions(+), 0 deletions(-)
> > 
> > diff --git a/doc/connman-api.txt b/doc/connman-api.txt
> > index b13efd1..18185d7 100644
> > --- a/doc/connman-api.txt
> > +++ b/doc/connman-api.txt
> > @@ -150,6 +150,16 @@ Properties	boolean Active [readwrite]
> > 
> >  			Holds whether the context is activated.  This value
> >  			can be set to activate / deactivate the context.
> > 
> > +		string Bearer [readonly, optional]
> > +
> > +			Contains the current packet switched bearer of the
> > +			connection context.
> > +
> > +			Possible values are the same as for the Technology
> > +			property the NetworkRegistration object, plus
> > +			"none" if no data activity is ongoing, or "unknown"
> > +			if not known.
> > +
> 
> Please remove this "unknown" bit.  The property is already optional, so
> if it is never reported by the driver we shouldn't report it up, even if
> the context is active.

That would not work. We need some default value otherwise we cannot signal the 
change from known to unknown, at least not with the oFono property change 
interface.

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki

Re: [PATCH 2/4] Core support for packet switched bearer reporting

[Denis Kenzior]

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

Hi Rémi,

On 01/10/2011 05:19 AM, Rémi Denis-Courmont wrote:
> On Friday 07 January 2011 18:25:32 ext Denis Kenzior, you wrote:
>>> @@ -2001,6 +2010,29 @@ void ofono_gprs_add_context(struct ofono_gprs
>>> *gprs,
>>>
>>>  	__ofono_atom_register(gc->atom, gprs_context_unregister);
>>>  
>>>  }
>>>
>>> +void ofono_gprs_bearer_notify(struct ofono_gprs *gprs,
>>> +				unsigned int cid, int bearer)
>>> +{
>>> +	DBusConnection *conn = ofono_dbus_get_connection();
>>> +	GSList *l;
>>> +	const char *value = packet_bearer_to_string(bearer);
>>
>> Why bother running this operation before you're sure you're going to
>> emit the signal?
> 
> Now that we're talking about network-initiated LTE bearers, I would rather be 
> safe than sorry. I wouldn't assume that the modem will signal the CPSB 
> (unchanged) value later in this case.
> 

Are we both talking about the packet_bearer_to_string call?

>> Please move this down.
> 
> To avoid any race, we have to query the existing CPSB values after we enable 
> the CPSB subscription, right? We could surely move that to a later stage, but 
> I don't see the benefit.
> 

What race?  I don't think you really read my comments...

Regards,
-Denis

Re: [PATCH 2/4] Core support for packet switched bearer reporting

[Denis Kenzior]

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

Hi Rémi,

On 01/10/2011 05:33 AM, Rémi Denis-Courmont wrote:
> On Friday 07 January 2011 18:25:32 ext Denis Kenzior, you wrote:
>> Hi Rémi,
>>
>> On 01/07/2011 10:02 AM, Rémi Denis-Courmont wrote:
>>> ---
>>>
>>>  include/gprs.h |    2 ++
>>>  src/gprs.c     |   32 ++++++++++++++++++++++++++++++++
>>>  2 files changed, 34 insertions(+), 0 deletions(-)
>>
>> please see the 'Submitting patches' section in HACKING
> 
> The rules are self-contradictory. Either the patch spreads multiple 
> directories, or it breaks compilation.
> 

They are really not.  For 99% of the time you need to break up your
patches like this and it works out nicely.

> For the sake of git-bisect'ing, I'd rather not break compilation.
> 

What are you talking about?  Adding a function declaration that nobody
yet calls is not going to break compilation or git bisect.

Regards,
-Denis

Re: [PATCH 3/4] Bearer documentation

[Denis Kenzior]

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

Hi Rémi,

On 01/10/2011 06:41 AM, Rémi Denis-Courmont wrote:
> On Friday 07 January 2011 18:23:25 ext Denis Kenzior, you wrote:
>> Hi Rémi,
>>
>> On 01/07/2011 10:02 AM, Rémi Denis-Courmont wrote:
>>> ---
>>>
>>>  doc/connman-api.txt |   10 ++++++++++
>>>  1 files changed, 10 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/doc/connman-api.txt b/doc/connman-api.txt
>>> index b13efd1..18185d7 100644
>>> --- a/doc/connman-api.txt
>>> +++ b/doc/connman-api.txt
>>> @@ -150,6 +150,16 @@ Properties	boolean Active [readwrite]
>>>
>>>  			Holds whether the context is activated.  This value
>>>  			can be set to activate / deactivate the context.
>>>
>>> +		string Bearer [readonly, optional]
>>> +
>>> +			Contains the current packet switched bearer of the
>>> +			connection context.
>>> +
>>> +			Possible values are the same as for the Technology
>>> +			property the NetworkRegistration object, plus
>>> +			"none" if no data activity is ongoing, or "unknown"
>>> +			if not known.
>>> +
>>
>> Please remove this "unknown" bit.  The property is already optional, so
>> if it is never reported by the driver we shouldn't report it up, even if
>> the context is active.
> 
> That would not work. We need some default value otherwise we cannot signal the 
> change from known to unknown, at least not with the oFono property change 
> interface.
> 

When do you think that will ever happen?

Regards,
-Denis

Re: [PATCH 3/4] Bearer documentation

[Denis Kenzior]

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

Hi Rémi,

On 01/10/2011 05:21 AM, Rémi Denis-Courmont wrote:
> On Friday 07 January 2011 18:23:25 ext Denis Kenzior, you wrote:
>> The other question I have is whether we should report this at the
>> context level or at the ConnectionManager level.  Can different contexts
>> truly have different bearers?
> 
> At least for 3G with HS*PA, I believe we can. In my understanding, HSPA 
> channels are allocated per contexts. Correct me if I'm wrong.
> 

I don't know, to me it would not make sense to allocate different
bearers per context, but I'm not an RF engineer.  Do note that at least
several vendors provide extension commands that report the global active
bearer, not per context.  For instance, see the Ericsson *CPSB command...

Someone more knowledgeable would have to weigh in here...

Regards,
-Denis

Re: [PATCH 3/4] Bearer documentation

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

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

On Monday 10 January 2011 18:27:33 ext Denis Kenzior, you wrote:
> Hi Rémi,
> 
> On 01/10/2011 05:21 AM, Rémi Denis-Courmont wrote:
> > On Friday 07 January 2011 18:23:25 ext Denis Kenzior, you wrote:
> >> The other question I have is whether we should report this at the
> >> context level or at the ConnectionManager level.  Can different contexts
> >> truly have different bearers?
> > 
> > At least for 3G with HS*PA, I believe we can. In my understanding, HSPA
> > channels are allocated per contexts. Correct me if I'm wrong.
> 
> I don't know, to me it would not make sense to allocate different
> bearers per context, but I'm not an RF engineer.  Do note that at least
> several vendors provide extension commands that report the global active
> bearer, not per context.  For instance, see the Ericsson *CPSB command...

The UI wants to show a single icon for the number of "G's". I would assume 
that's meant to address this use case. Then you don't need to iterate over all 
contexts, especially those not allocated through that this particular AT 
serial port.

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki

Re: [PATCH 3/4] Bearer documentation

[Marcel Holtmann]

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

Hi Remi,

> > > On Friday 07 January 2011 18:23:25 ext Denis Kenzior, you wrote:
> > >> The other question I have is whether we should report this at the
> > >> context level or at the ConnectionManager level.  Can different contexts
> > >> truly have different bearers?
> > > 
> > > At least for 3G with HS*PA, I believe we can. In my understanding, HSPA
> > > channels are allocated per contexts. Correct me if I'm wrong.
> > 
> > I don't know, to me it would not make sense to allocate different
> > bearers per context, but I'm not an RF engineer.  Do note that at least
> > several vendors provide extension commands that report the global active
> > bearer, not per context.  For instance, see the Ericsson *CPSB command...
> 
> The UI wants to show a single icon for the number of "G's". I would assume 
> that's meant to address this use case. Then you don't need to iterate over all 
> contexts, especially those not allocated through that this particular AT 
> serial port.

I am confused by your answer.

So from what I understand so far it is that with AT modems the bearer
information is a per context information. So the real question is if
this is just an AT command thing of how they defined it or if it can be
really independent.

As Denis said, the vendor commands indicate that it is a global bearer
information. If one context is on HSPA, then others will be as well. So
how does ISI provide this information? Is it globally or per context?

Regards

Marcel

Re: [PATCH 3/4] Bearer documentation

[Aki Niemi]

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

Hi Marcel,

On Mon, 2011-01-10 at 16:26 -0800, ext Marcel Holtmann wrote:
> As Denis said, the vendor commands indicate that it is a global bearer
> information. If one context is on HSPA, then others will be as well. So
> how does ISI provide this information? Is it globally or per context?

I doubt contexts can even be on different bearers. At least on ISI, the
indication for HS*PA channel allocations is global, and so is cell
supported technologies.

Cheers,
Aki

Re: [PATCH 3/4] Bearer documentation

[Marcel Holtmann]

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

Hi Aki,

> > As Denis said, the vendor commands indicate that it is a global bearer
> > information. If one context is on HSPA, then others will be as well. So
> > how does ISI provide this information? Is it globally or per context?
> 
> I doubt contexts can even be on different bearers. At least on ISI, the
> indication for HS*PA channel allocations is global, and so is cell
> supported technologies.

that is what I thought. It is just a stupidity in the AT command
specification that they give a CID with it.

Regards

Marcel

Re: [PATCH 3/4] Bearer documentation

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

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

On Wednesday 12 January 2011 18:40:45 ext Marcel Holtmann, you wrote:
> that is what I thought. It is just a stupidity in the AT command
> specification that they give a CID with it.

If that's the understanding, I will rewrite the patch to expose the current 
bearer in the connection manager and keep the network access technology as it 
currently is - in the network registration.

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki