[RFC PATCH 0/3] Fix CDMA support with composite plugins

[RFC PATCH 0/3] Fix CDMA support with composite plugins

[Forest Bond]

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

Hi,

Support for Huawei CDMA devices has been broken since the huaweicdma
plugin was merged into the huawei plugin.  The root cause for the
breakage is that:

1. CDMA devices rely on being automatically set to online state once
   enabled.  This is because they do not expose a SIM interface, so
   clients (e.g. ConnMan) don't have a good way to know that a device is
   ready to be set online.

2. The modem is only automatically set online if the modem driver does
   not define a set_online callback.  But since the huawei plugin
   supports both GSM and CDMA devices, it must provide a set_online
   callback.  So oFono core does not automatically set Huawei CDMA
   modems online when enabled.

On IRC Denis suggested solving this problem by splitting CDMA support
from the huawei plugin so that we can again have a huaweicdma driver
with no set_online callback.  I looked into this and of course it is
possible.  But to me it feels like a big step backward for the huawei
driver.

The split plugin approach also complicates handling of Sierra Wireless
CDMA devices (which I'm working on).  It forces us to maintain a
database of USB IDs mapped to the right driver to use.  But if we use a
single driver we can detect CDMA support when the device is enabled like
the huawei driver does.

As Denis said, CDMA support in oFono is a hack because oFono is
SIM-centric.  Right now clients have to jump through a different set of
hoops to get a CDMA device online than they do for a GSM device.  So the
root cause of the CDMA problem is that the D-BUS API exposed for CDMA
devices is crippled, which creates the need to automatically set CDMA
devices online.  The long term fix for this problem is to either further
abstract the D-Bus API from the device or make CDMA devices look more
like GSM devices.

But in the meantime what we really want is a way to tell the core to
automatically set a device online even if the modem driver provides a
set_online callback.  So I propose the following patch series as a
simple way to do this.

Thanks,
Forest

Forest Bond (3):
  modem: Allow setting boolean properties
  modem: Support modem property "AlwaysOnline"
  huawei: Set property "AlwaysOnline" for CDMA modems

 plugins/huawei.c |    3 +++
 src/modem.c      |   18 +++++++++++++++---
 2 files changed, 18 insertions(+), 3 deletions(-)

[RFC PATCH 1/3] modem: Allow setting boolean properties

[Forest Bond]

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

From: Forest Bond <forest.bond@rapidrollout.com>

---
 src/modem.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/src/modem.c b/src/modem.c
index 3c7c80a..0f12f5a 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -1659,7 +1659,8 @@ static int set_modem_property(struct ofono_modem *modem, const char *name,
 	DBG("modem %p property %s", modem, name);
 
 	if (type != PROPERTY_TYPE_STRING &&
-			type != PROPERTY_TYPE_INTEGER)
+			type != PROPERTY_TYPE_INTEGER &&
+			type != PROPERTY_TYPE_BOOLEAN)
 		return -EINVAL;
 
 	property = g_try_new0(struct modem_property, 1);
-- 
1.7.0.4

Re: [RFC PATCH 1/3] modem: Allow setting boolean properties

[Denis Kenzior]

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

Hi Forest,

On 03/28/2013 09:22 AM, Forest Bond wrote:
> From: Forest Bond<forest.bond@rapidrollout.com>
>
> ---
>   src/modem.c |    3 ++-
>   1 files changed, 2 insertions(+), 1 deletions(-)

Patch has been applied, thanks.

Regards,
-Denis

[RFC PATCH 2/3] modem: Support modem property "AlwaysOnline"

[Forest Bond]

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

From: Forest Bond <forest.bond@rapidrollout.com>

---
 src/modem.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/modem.c b/src/modem.c
index 0f12f5a..6c6937a 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -595,6 +595,17 @@ static gboolean modem_has_sim(struct ofono_modem *modem)
 	return FALSE;
 }
 
+static gboolean modem_is_always_online(struct ofono_modem *modem)
+{
+	if (modem->driver->set_online == NULL)
+		return TRUE;
+
+	if (ofono_modem_get_boolean(modem, "AlwaysOnline") == TRUE)
+		return TRUE;
+
+	return FALSE;
+}
+
 static void common_online_cb(const struct ofono_error *error, void *data)
 {
 	struct ofono_modem *modem = data;
@@ -706,7 +717,7 @@ static void sim_state_watch(enum ofono_sim_state new_state, void *user)
 		 * If we don't have the set_online method, also proceed
 		 * straight to the online state
 		 */
-		if (modem->driver->set_online == NULL)
+		if (modem_is_always_online(modem) == TRUE)
 			set_online(modem, TRUE);
 
 		if (modem->online == TRUE)
@@ -745,7 +756,7 @@ static DBusMessage *set_property_online(struct ofono_modem *modem,
 	if (ofono_modem_get_emergency_mode(modem) == TRUE)
 		return __ofono_error_emergency_active(msg);
 
-	if (driver->set_online == NULL)
+	if (modem_is_always_online(modem) == TRUE)
 		return __ofono_error_not_implemented(msg);
 
 	modem->pending = dbus_message_ref(msg);
-- 
1.7.0.4

Re: [RFC PATCH 2/3] modem: Support modem property "AlwaysOnline"

[Denis Kenzior]

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

Hi Forest,

On 03/28/2013 09:22 AM, Forest Bond wrote:
> From: Forest Bond<forest.bond@rapidrollout.com>
>
> ---
>   src/modem.c |   15 +++++++++++++--
>   1 files changed, 13 insertions(+), 2 deletions(-)
>

I would actually be okay with this as an interim solution, but see my 
comments to patch 3.

Regards,
-Denis

Re: [RFC PATCH 2/3] modem: Support modem property "AlwaysOnline"

[Forest Bond]

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

Hi Denis,

On Thu, Mar 28, 2013 at 09:55:25AM -0500, Denis Kenzior wrote:
> Hi Forest,
> 
> On 03/28/2013 09:22 AM, Forest Bond wrote:
> >From: Forest Bond<forest.bond@rapidrollout.com>
> >
> >---
> >  src/modem.c |   15 +++++++++++++--
> >  1 files changed, 13 insertions(+), 2 deletions(-)
> >
> 
> I would actually be okay with this as an interim solution, but see
> my comments to patch 3.

Great, glad to hear it.

Quick question:  Once this change is in place, set_online is not called for CDMA
devices.  Should I kill the CDMA-only code paths there?

Thanks,
Forest
-- 
Forest Bond
http://www.alittletooquiet.net
http://www.rapidrollout.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFC PATCH 2/3] modem: Support modem property "AlwaysOnline"

[Denis Kenzior]

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

Hi Forest,

On 03/28/2013 10:36 AM, Forest Bond wrote:
> Hi Denis,
>
> On Thu, Mar 28, 2013 at 09:55:25AM -0500, Denis Kenzior wrote:
>> Hi Forest,
>>
>> On 03/28/2013 09:22 AM, Forest Bond wrote:
>>> From: Forest Bond<forest.bond@rapidrollout.com>
>>>
>>> ---
>>>   src/modem.c |   15 +++++++++++++--
>>>   1 files changed, 13 insertions(+), 2 deletions(-)
>>>
>>
>> I would actually be okay with this as an interim solution, but see
>> my comments to patch 3.
>
> Great, glad to hear it.
>
> Quick question:  Once this change is in place, set_online is not called for CDMA
> devices.  Should I kill the CDMA-only code paths there?

I don't recall off hand which code paths these are.  Can you elaborate?

Regards,
-Denis

Re: [RFC PATCH 2/3] modem: Support modem property "AlwaysOnline"

[Forest Bond]

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

Hi Denis,

On Thu, Mar 28, 2013 at 11:05:38AM -0500, Denis Kenzior wrote:
> On 03/28/2013 10:36 AM, Forest Bond wrote:
> >On Thu, Mar 28, 2013 at 09:55:25AM -0500, Denis Kenzior wrote:
> >>On 03/28/2013 09:22 AM, Forest Bond wrote:
> >>>From: Forest Bond<forest.bond@rapidrollout.com>
> >>>
> >>>---
> >>>  src/modem.c |   15 +++++++++++++--
> >>>  1 files changed, 13 insertions(+), 2 deletions(-)
> >>>
> >>
> >>I would actually be okay with this as an interim solution, but see
> >>my comments to patch 3.
> >
> >Great, glad to hear it.
> >
> >Quick question:  Once this change is in place, set_online is not called for CDMA
> >devices.  Should I kill the CDMA-only code paths there?
> 
> I don't recall off hand which code paths these are.  Can you elaborate?

Sorry, I guess I was thinking of the sierra plugin.  In the huawei plugin the
only candidate for removal would be this chunk in rfswitch_support:

if (data->have_gsm == FALSE && data->have_cdma == TRUE) {
	data->offline_command = "AT+CFUN=5";
	goto done;
}

But I guess we might as well leave that.

Thanks,
Forest
-- 
Forest Bond
http://www.alittletooquiet.net
http://www.rapidrollout.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[RFC PATCH 3/3] huawei: Set property "AlwaysOnline" for CDMA modems

[Forest Bond]

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

From: Forest Bond <forest.bond@rapidrollout.com>

---
 plugins/huawei.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/plugins/huawei.c b/plugins/huawei.c
index 5d8875a..0621d5b 100644
--- a/plugins/huawei.c
+++ b/plugins/huawei.c
@@ -377,6 +377,9 @@ static void cfun_offline(gboolean ok, GAtResult *result, gpointer user_data)
 		return;
 	}
 
+	if (data->have_gsm == FALSE && data->have_cdma == TRUE)
+		ofono_modem_set_boolean(modem, "AlwaysOnline", TRUE);
+
 	ofono_modem_set_powered(modem, TRUE);
 }
 
-- 
1.7.0.4

Re: [RFC PATCH 3/3] huawei: Set property "AlwaysOnline" for CDMA modems

[Denis Kenzior]

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

Hi Forest,

On 03/28/2013 09:22 AM, Forest Bond wrote:
> From: Forest Bond<forest.bond@rapidrollout.com>
>
> ---
>   plugins/huawei.c |    3 +++
>   1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/plugins/huawei.c b/plugins/huawei.c
> index 5d8875a..0621d5b 100644
> --- a/plugins/huawei.c
> +++ b/plugins/huawei.c
> @@ -377,6 +377,9 @@ static void cfun_offline(gboolean ok, GAtResult *result, gpointer user_data)
>   		return;
>   	}
>
> +	if (data->have_gsm == FALSE&&  data->have_cdma == TRUE)
> +		ofono_modem_set_boolean(modem, "AlwaysOnline", TRUE);
> +
>   	ofono_modem_set_powered(modem, TRUE);
>   }
>

Things don't seem to be this simple.  The Huawei driver sets the modem 
offline as part of the init procedure (grep for 'offline_command').  So 
once we enable the device, it is put into radio off mode.  With the 
changes you're proposing there will be no way to get it online ;)

Regards,
-Denis

Re: [RFC PATCH 3/3] huawei: Set property "AlwaysOnline" for CDMA modems

[Forest Bond]

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

Hi Denis,

Thanks for the quick reply.

On Thu, Mar 28, 2013 at 09:54:42AM -0500, Denis Kenzior wrote:
> On 03/28/2013 09:22 AM, Forest Bond wrote:
> >From: Forest Bond<forest.bond@rapidrollout.com>
> >
> >---
> >  plugins/huawei.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> >
> >diff --git a/plugins/huawei.c b/plugins/huawei.c
> >index 5d8875a..0621d5b 100644
> >--- a/plugins/huawei.c
> >+++ b/plugins/huawei.c
> >@@ -377,6 +377,9 @@ static void cfun_offline(gboolean ok, GAtResult *result, gpointer user_data)
> >  		return;
> >  	}
> >
> >+	if (data->have_gsm == FALSE&&  data->have_cdma == TRUE)
> >+		ofono_modem_set_boolean(modem, "AlwaysOnline", TRUE);
> >+
> >  	ofono_modem_set_powered(modem, TRUE);
> >  }
> >
> 
> Things don't seem to be this simple.  The Huawei driver sets the
> modem offline as part of the init procedure (grep for
> 'offline_command').  So once we enable the device, it is put into
> radio off mode.  With the changes you're proposing there will be no
> way to get it online ;)

Okay, I see that now.  I only have a Huawei GSM device and a Sierra Wireless
CDMA device to test with.

Thanks,
Forest
-- 
Forest Bond
http://www.alittletooquiet.net
http://www.rapidrollout.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]