[PATCH 0/4] Add ModemTechnologies property to RadioSettings

[PATCH 0/4] Add ModemTechnologies property to RadioSettings

[Alfonso Sanchez-Beato]

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

This patch series add the ModemTechnologies property to the
RadioSettings interface. Its purpose is to show the list of valid
values for the TechnologyPreference property in the same interface.
The use case is letting a UI show the possible RATs so the user cannot
select an invalid value for a given modem.

Alfonso Sanchez-Beato (4):
  doc: Add ModemTechnologies property
  include: Add method to list RATs to radio-settings
  src: Implement RAT list property
  test: Add ModemTechnologies to list-modems

 doc/radio-settings-api.txt |  5 ++++
 include/radio-settings.h   | 11 +++++++++
 src/radio-settings.c       | 61 +++++++++++++++++++++++++++++++++++++++++++++-
 test/list-modems           |  3 ++-
 4 files changed, 78 insertions(+), 2 deletions(-)

-- 
2.1.0

[PATCH 1/4] doc: Add ModemTechnologies property

[Alfonso Sanchez-Beato]

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

This new property in org.ofono.RadioSettings interface will hold the
possible values for radio access technology for the modem.
---
 doc/radio-settings-api.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/doc/radio-settings-api.txt b/doc/radio-settings-api.txt
index 9f9256f..cbeac3b 100644
--- a/doc/radio-settings-api.txt
+++ b/doc/radio-settings-api.txt
@@ -45,6 +45,11 @@ Properties	string TechnologyPreference [readwrite]
 				"umts"	Only UMTS used for radio access.
 				"lte"	Only LTE used for radio access.
 
+		array{string} ModemTechnologies [readonly, optional]
+
+			List of values for TechnologyPreference property
+			supported by the modem.
+
 		string GsmBand [readwrite, optional]
 
 			Frequency band in which the modem is allowed to
-- 
2.1.0

[PATCH 2/4] include: Add method to list RATs to radio-settings

[Alfonso Sanchez-Beato]

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

---
 include/radio-settings.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/radio-settings.h b/include/radio-settings.h
index 1d0ca3e..d7481b3 100644
--- a/include/radio-settings.h
+++ b/include/radio-settings.h
@@ -35,6 +35,9 @@ enum ofono_radio_access_mode {
 	OFONO_RADIO_ACCESS_MODE_LTE	= 3,
 };
 
+/* Set this to latest in ofono_radio_access_mode + 1 */
+#define OFONO_RADIO_ACCESS_MODE_LAST (OFONO_RADIO_ACCESS_MODE_LTE + 1)
+
 enum ofono_radio_band_gsm {
 	OFONO_RADIO_BAND_GSM_ANY,
 	OFONO_RADIO_BAND_GSM_850,
@@ -80,6 +83,11 @@ typedef void (*ofono_radio_settings_fast_dormancy_query_cb_t)(
 						ofono_bool_t enable,
 						void *data);
 
+typedef void (*ofono_radio_settings_modem_rats_query_cb_t)(
+						const struct ofono_error *error,
+						const ofono_bool_t rats[],
+						void *data);
+
 struct ofono_radio_settings_driver {
 	const char *name;
 	int (*probe)(struct ofono_radio_settings *rs, unsigned int vendor,
@@ -107,6 +115,9 @@ struct ofono_radio_settings_driver {
 				ofono_bool_t enable,
 				ofono_radio_settings_fast_dormancy_set_cb_t,
 				void *data);
+	void (*query_modem_rats)(struct ofono_radio_settings *rs,
+				ofono_radio_settings_modem_rats_query_cb_t cb,
+				void *data);
 };
 
 int ofono_radio_settings_driver_register(
-- 
2.1.0

[PATCH 3/4] src: Implement RAT list property

[Alfonso Sanchez-Beato]

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

---
 src/radio-settings.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 60 insertions(+), 1 deletion(-)

diff --git a/src/radio-settings.c b/src/radio-settings.c
index d1b1cc1..4967994 100644
--- a/src/radio-settings.c
+++ b/src/radio-settings.c
@@ -48,6 +48,8 @@ struct ofono_radio_settings {
 	enum ofono_radio_band_gsm pending_band_gsm;
 	enum ofono_radio_band_umts pending_band_umts;
 	ofono_bool_t fast_dormancy_pending;
+	ofono_bool_t modem_rats[OFONO_RADIO_ACCESS_MODE_LAST];
+	ofono_bool_t modem_rats_filled;
 	const struct ofono_radio_settings_driver *driver;
 	void *driver_data;
 	struct ofono_atom *atom;
@@ -222,6 +224,21 @@ static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
 					DBUS_TYPE_BOOLEAN, &value);
 	}
 
+	if (rs->driver->query_modem_rats) {
+		const char *rats_strs[OFONO_RADIO_ACCESS_MODE_LAST + 1];
+		const char *(*strs)[] = &rats_strs;
+		int i, str_i;
+
+		for (i = 0, str_i = 0; i < OFONO_RADIO_ACCESS_MODE_LAST; ++i)
+			if (rs->modem_rats[i])
+				rats_strs[str_i++] =
+						radio_access_mode_to_string(i);
+		rats_strs[str_i] = NULL;
+
+		ofono_dbus_dict_append_array(&dict, "ModemTechnologies",
+						DBUS_TYPE_STRING, &strs);
+	}
+
 	dbus_message_iter_close_container(&iter, &dict);
 
 	return reply;
@@ -374,6 +391,43 @@ static void radio_send_properties_reply(struct ofono_radio_settings *rs)
 	__ofono_dbus_pending_reply(&rs->pending, reply);
 }
 
+static void radio_set_modem_rats(struct ofono_radio_settings *rs,
+					const ofono_bool_t rats[])
+{
+	memcpy(rs->modem_rats, rats, sizeof(rs->modem_rats));
+	rs->modem_rats_filled = TRUE;
+}
+
+static void radio_modem_rats_query_callback(const struct ofono_error *error,
+						const ofono_bool_t rats[],
+						void *data)
+{
+	struct ofono_radio_settings *rs = data;
+	DBusMessage *reply;
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+		DBG("Error during modem rats query");
+
+		reply = __ofono_error_failed(rs->pending);
+		__ofono_dbus_pending_reply(&rs->pending, reply);
+
+		return;
+	}
+
+	radio_set_modem_rats(rs, rats);
+	radio_send_properties_reply(rs);
+}
+
+static void radio_query_modem_rats(struct ofono_radio_settings *rs)
+{
+	if (rs->driver->query_modem_rats == NULL) {
+		radio_send_properties_reply(rs);
+		return;
+	}
+
+	rs->driver->query_modem_rats(rs, radio_modem_rats_query_callback, rs);
+}
+
 static void radio_fast_dormancy_query_callback(const struct ofono_error *error,
 						ofono_bool_t enable, void *data)
 {
@@ -390,7 +444,12 @@ static void radio_fast_dormancy_query_callback(const struct ofono_error *error,
 	}
 
 	radio_set_fast_dormancy(rs, enable);
-	radio_send_properties_reply(rs);
+
+	/* Modem technology is not supposed to change, so one query is enough */
+	if (rs->modem_rats_filled)
+		radio_send_properties_reply(rs);
+	else
+		radio_query_modem_rats(rs);
 }
 
 static void radio_query_fast_dormancy(struct ofono_radio_settings *rs)
-- 
2.1.0

[PATCH 4/4] test: Add ModemTechnologies to list-modems

[Alfonso Sanchez-Beato]

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

---
 test/list-modems | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/test/list-modems b/test/list-modems
index b9f510a..cc150f6 100755
--- a/test/list-modems
+++ b/test/list-modems
@@ -40,7 +40,8 @@ for path, properties in modems:
 					"PreferredLanguages",
 					"PrimaryContexts",
 					"LockedPins",
-					"Features"]:
+					"Features",
+					"ModemTechnologies"]:
 				val = ""
 				for i in properties[key]:
 					val += i + " "
-- 
2.1.0

Re: [PATCH 1/4] doc: Add ModemTechnologies property

[Denis Kenzior]

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

Hi Alfonso,

On 12/05/2014 10:46 AM, Alfonso Sanchez-Beato wrote:
> This new property in org.ofono.RadioSettings interface will hold the
> possible values for radio access technology for the modem.
> ---
>   doc/radio-settings-api.txt | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/doc/radio-settings-api.txt b/doc/radio-settings-api.txt
> index 9f9256f..cbeac3b 100644
> --- a/doc/radio-settings-api.txt
> +++ b/doc/radio-settings-api.txt
> @@ -45,6 +45,11 @@ Properties	string TechnologyPreference [readwrite]
>   				"umts"	Only UMTS used for radio access.
>   				"lte"	Only LTE used for radio access.
>
> +		array{string} ModemTechnologies [readonly, optional]
> +

Lets name this AvailableTechnologies

> +			List of values for TechnologyPreference property
> +			supported by the modem.
> +
>   		string GsmBand [readwrite, optional]
>
>   			Frequency band in which the modem is allowed to
>

Regards,
-Denis

Re: [PATCH 2/4] include: Add method to list RATs to radio-settings

[Denis Kenzior]

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

Hi Alfonso,

On 12/05/2014 10:46 AM, Alfonso Sanchez-Beato wrote:
> ---
>   include/radio-settings.h | 11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/include/radio-settings.h b/include/radio-settings.h
> index 1d0ca3e..d7481b3 100644
> --- a/include/radio-settings.h
> +++ b/include/radio-settings.h
> @@ -35,6 +35,9 @@ enum ofono_radio_access_mode {
>   	OFONO_RADIO_ACCESS_MODE_LTE	= 3,
>   };
>
> +/* Set this to latest in ofono_radio_access_mode + 1 */
> +#define OFONO_RADIO_ACCESS_MODE_LAST (OFONO_RADIO_ACCESS_MODE_LTE + 1)
> +

I would actually change radio_access_mode into a bit field, that way the 
available access modes can be passed as an unsigned integer.

e.g. something like:

OFONO_RADIO_ACCESS_MODE_ANY = 0,
OFONO_RADIO_ACCESS_MODE_GSM = 0x1,
OFONO_RADIO_ACCESS_MODE_UMTS = 0x2,
OFONO_RADIO_ACCESS_MODE_LTE = 0x4,

>   enum ofono_radio_band_gsm {
>   	OFONO_RADIO_BAND_GSM_ANY,
>   	OFONO_RADIO_BAND_GSM_850,
> @@ -80,6 +83,11 @@ typedef void (*ofono_radio_settings_fast_dormancy_query_cb_t)(
>   						ofono_bool_t enable,
>   						void *data);
>
> +typedef void (*ofono_radio_settings_modem_rats_query_cb_t)(
> +						const struct ofono_error *error,
> +						const ofono_bool_t rats[],
> +						void *data);
> +

Then this becomes:

typedef void (*ofono_radio_settings_available_rats_query_cb_t)(
				const struct ofono_error *error,
				unsigned int available_rats,
				void *data);

>   struct ofono_radio_settings_driver {
>   	const char *name;
>   	int (*probe)(struct ofono_radio_settings *rs, unsigned int vendor,
> @@ -107,6 +115,9 @@ struct ofono_radio_settings_driver {
>   				ofono_bool_t enable,
>   				ofono_radio_settings_fast_dormancy_set_cb_t,
>   				void *data);
> +	void (*query_modem_rats)(struct ofono_radio_settings *rs,
> +				ofono_radio_settings_modem_rats_query_cb_t cb,
> +				void *data);

name this query_available_rats please.

>   };
>
>   int ofono_radio_settings_driver_register(
>

Regards,
-Denis

Re: [PATCH 3/4] src: Implement RAT list property

[Denis Kenzior]

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

Hi Alfonso,

On 12/05/2014 10:46 AM, Alfonso Sanchez-Beato wrote:
> ---
>   src/radio-settings.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 60 insertions(+), 1 deletion(-)
>
> diff --git a/src/radio-settings.c b/src/radio-settings.c
> index d1b1cc1..4967994 100644
> --- a/src/radio-settings.c
> +++ b/src/radio-settings.c
> @@ -48,6 +48,8 @@ struct ofono_radio_settings {
>   	enum ofono_radio_band_gsm pending_band_gsm;
>   	enum ofono_radio_band_umts pending_band_umts;
>   	ofono_bool_t fast_dormancy_pending;
> +	ofono_bool_t modem_rats[OFONO_RADIO_ACCESS_MODE_LAST];
> +	ofono_bool_t modem_rats_filled;

In general we use uint32_t flags and set the appropriate flag.  See e.g. 
src/voicecall.c for an example.

I think if you take my suggestions from patch 2, then a simple uint32_t 
available_rats variable should be sufficient.  If it is set to zero, 
then the AvailableTechnologies property should be hidden.

>   	const struct ofono_radio_settings_driver *driver;
>   	void *driver_data;
>   	struct ofono_atom *atom;
> @@ -222,6 +224,21 @@ static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
>   					DBUS_TYPE_BOOLEAN, &value);
>   	}
>
> +	if (rs->driver->query_modem_rats) {
> +		const char *rats_strs[OFONO_RADIO_ACCESS_MODE_LAST + 1];
> +		const char *(*strs)[] = &rats_strs;

I'm seriously lost what you're trying to do here.  &rats_strs is a no-op.

I suspect that const char **strs = rats_strs; would be a bit easier to 
understand.

> +		int i, str_i;
> +
> +		for (i = 0, str_i = 0; i < OFONO_RADIO_ACCESS_MODE_LAST; ++i)
> +			if (rs->modem_rats[i])
> +				rats_strs[str_i++] =
> +						radio_access_mode_to_string(i);
> +		rats_strs[str_i] = NULL;
> +
> +		ofono_dbus_dict_append_array(&dict, "ModemTechnologies",
> +						DBUS_TYPE_STRING, &strs);

Please make this into AvailableTechnologies

> +	}
> +
>   	dbus_message_iter_close_container(&iter, &dict);
>
>   	return reply;
> @@ -374,6 +391,43 @@ static void radio_send_properties_reply(struct ofono_radio_settings *rs)
>   	__ofono_dbus_pending_reply(&rs->pending, reply);
>   }
>
> +static void radio_set_modem_rats(struct ofono_radio_settings *rs,
> +					const ofono_bool_t rats[])
> +{
> +	memcpy(rs->modem_rats, rats, sizeof(rs->modem_rats));
> +	rs->modem_rats_filled = TRUE;

A two line function that is called from only one place is not a good idea.

> +}
> +
> +static void radio_modem_rats_query_callback(const struct ofono_error *error,
> +						const ofono_bool_t rats[],
> +						void *data)
> +{
> +	struct ofono_radio_settings *rs = data;
> +	DBusMessage *reply;
> +
> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> +		DBG("Error during modem rats query");
> +
> +		reply = __ofono_error_failed(rs->pending);
> +		__ofono_dbus_pending_reply(&rs->pending, reply);
> +
> +		return;

If possible the GetProperties call should not return an error.  In this 
particular case, if the query fails, simply omit AvailableTechnologies 
from the properties dictionary.  Hopefully the query succeeds next time.

> +	}
> +
> +	radio_set_modem_rats(rs, rats);
> +	radio_send_properties_reply(rs);
> +}
> +
> +static void radio_query_modem_rats(struct ofono_radio_settings *rs)
> +{
> +	if (rs->driver->query_modem_rats == NULL) {
> +		radio_send_properties_reply(rs);
> +		return;
> +	}
> +
> +	rs->driver->query_modem_rats(rs, radio_modem_rats_query_callback, rs);
> +}
> +
>   static void radio_fast_dormancy_query_callback(const struct ofono_error *error,
>   						ofono_bool_t enable, void *data)
>   {
> @@ -390,7 +444,12 @@ static void radio_fast_dormancy_query_callback(const struct ofono_error *error,
>   	}
>
>   	radio_set_fast_dormancy(rs, enable);
> -	radio_send_properties_reply(rs);
> +
> +	/* Modem technology is not supposed to change, so one query is enough */
> +	if (rs->modem_rats_filled)
> +		radio_send_properties_reply(rs);
> +	else
> +		radio_query_modem_rats(rs);
>   }
>
>   static void radio_query_fast_dormancy(struct ofono_radio_settings *rs)
>

Regards,
-Denis

Re: [PATCH 4/4] test: Add ModemTechnologies to list-modems

[Denis Kenzior]

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

Hi Alfonso,

On 12/05/2014 10:46 AM, Alfonso Sanchez-Beato wrote:
> ---
>   test/list-modems | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/test/list-modems b/test/list-modems
> index b9f510a..cc150f6 100755
> --- a/test/list-modems
> +++ b/test/list-modems
> @@ -40,7 +40,8 @@ for path, properties in modems:
>   					"PreferredLanguages",
>   					"PrimaryContexts",
>   					"LockedPins",
> -					"Features"]:
> +					"Features",
> +					"ModemTechnologies"]:

AvailableTechnologies here please

>   				val = ""
>   				for i in properties[key]:
>   					val += i + " "
>

Regards,
-Denis

Re: [PATCH 0/4] Add ModemTechnologies property to RadioSettings

[Denis Kenzior]

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

Hi Alfonso,

On 12/05/2014 10:46 AM, Alfonso Sanchez-Beato wrote:
> This patch series add the ModemTechnologies property to the
> RadioSettings interface. Its purpose is to show the list of valid
> values for the TechnologyPreference property in the same interface.
> The use case is letting a UI show the possible RATs so the user cannot
> select an invalid value for a given modem.
>

Use case makes sense to me at least.  Thanks for doing this.

Regards,
-Denis

Re: [PATCH 0/4] Add ModemTechnologies property to RadioSettings

[Alfonso Sanchez-Beato]

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

On Mon, Dec 8, 2014 at 5:51 AM, Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Alfonso,
>
> On 12/05/2014 10:46 AM, Alfonso Sanchez-Beato wrote:
>
>> This patch series add the ModemTechnologies property to the
>> RadioSettings interface. Its purpose is to show the list of valid
>> values for the TechnologyPreference property in the same interface.
>> The use case is letting a UI show the possible RATs so the user cannot
>> select an invalid value for a given modem.
>>
>>
> Use case makes sense to me at least.  Thanks for doing this.
>
> Regards,
> -Denis
>

Thanks for the detailed review.  Sending the new patch set right now...

Br,
Alfonso

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 1165 bytes --]

[PATCH 2/4] include: Add method to list RATs to radio-settings

[Alfonso Sanchez-Beato]

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

---
 include/radio-settings.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/radio-settings.h b/include/radio-settings.h
index 1d0ca3e..545a29d 100644
--- a/include/radio-settings.h
+++ b/include/radio-settings.h
@@ -35,6 +35,13 @@ enum ofono_radio_access_mode {
 	OFONO_RADIO_ACCESS_MODE_LTE	= 3,
 };
 
+#define OFONO_FLAG_RADIO_ACCESS_MODE_GSM \
+				(1 << (OFONO_RADIO_ACCESS_MODE_GSM - 1))
+#define OFONO_FLAG_RADIO_ACCESS_MODE_UMTS \
+				(1 << (OFONO_RADIO_ACCESS_MODE_UMTS - 1))
+#define OFONO_FLAG_RADIO_ACCESS_MODE_LTE \
+				(1 << (OFONO_RADIO_ACCESS_MODE_LTE - 1))
+
 enum ofono_radio_band_gsm {
 	OFONO_RADIO_BAND_GSM_ANY,
 	OFONO_RADIO_BAND_GSM_850,
@@ -80,6 +87,11 @@ typedef void (*ofono_radio_settings_fast_dormancy_query_cb_t)(
 						ofono_bool_t enable,
 						void *data);
 
+typedef void (*ofono_radio_settings_available_rats_query_cb_t)(
+						const struct ofono_error *error,
+						unsigned int available_rats,
+						void *data);
+
 struct ofono_radio_settings_driver {
 	const char *name;
 	int (*probe)(struct ofono_radio_settings *rs, unsigned int vendor,
@@ -107,6 +119,9 @@ struct ofono_radio_settings_driver {
 				ofono_bool_t enable,
 				ofono_radio_settings_fast_dormancy_set_cb_t,
 				void *data);
+	void (*query_available_rats)(struct ofono_radio_settings *rs,
+			ofono_radio_settings_available_rats_query_cb_t cb,
+			void *data);
 };
 
 int ofono_radio_settings_driver_register(
-- 
2.1.0