Re: [PATCH v4 05/12] handsfree: Supported AG features exposed in D-Bus

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Mikel,

On 10/21/2011 11:51 AM, Mikel Astiz wrote:
> ---
>  src/handsfree.c |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 58 insertions(+), 0 deletions(-)
> 
> diff --git a/src/handsfree.c b/src/handsfree.c
> index 24f2349..27af31c 100644
> --- a/src/handsfree.c
> +++ b/src/handsfree.c
> @@ -41,9 +41,12 @@
>  #include "ofono.h"
>  #include "common.h"
>  
> +#define MAX_AG_FEATURE_COUNT 32
> +

So you probably meant 33 here right?

>  static GSList *g_drivers = NULL;
>  
>  struct ofono_handsfree {
> +	unsigned int ag_features;
>  	ofono_bool_t inband_ringing;
>  	ofono_bool_t voice_recognition;
>  	ofono_bool_t voice_recognition_pending;
> @@ -54,6 +57,54 @@ struct ofono_handsfree {
>  	DBusMessage *pending;
>  };
>  
> +static void append_ag_features(struct ofono_handsfree *hf, char **features)
> +{
> +	unsigned int bit;
> +	int feature_count = 0;
> +
> +	for (bit = 1; bit != 0; bit <<= 1) {
> +		if (hf->ag_features & bit) {
> +			char *name = NULL;
> +
> +			switch ((enum hfp_ag_feature) bit) {
> +			case HFP_AG_FEATURE_VOICE_RECOG:
> +				name = "voice-recognition";
> +				break;
> +			case HFP_AG_FEATURE_ATTACH_VOICE_TAG:
> +				name = "attach-voice-tag";
> +				break;
> +			default:
> +				continue;
> +			}
> +
> +			features[feature_count++] = name;
> +		}
> +	}
> +
> +	features[feature_count] = NULL;
> +}
> +

This function seemed way too complicated for what you were trying to
accomplish.

> +void ofono_handsfree_set_ag_features(struct ofono_handsfree *hf,
> +					unsigned int ag_features)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path = __ofono_atom_get_path(hf->atom);
> +	char **features;
> +
> +	if (hf->ag_features == ag_features)
> +		return;
> +
> +	hf->ag_features = ag_features;
> +
> +	features = g_new0(char *, MAX_AG_FEATURE_COUNT + 1);
> +	append_ag_features(hf, features);
> +	ofono_dbus_signal_array_property_changed(conn, path,
> +						OFONO_HANDSFREE_INTERFACE,
> +						"Features", DBUS_TYPE_STRING,
> +						&features);
> +	g_free(features);

I also didn't like this part.  The AG features should be set by the atom
driver prior to calling ofono_handsfree_register.  And since the BRSF
cannot be changed during the lifetime of the atom, there's no need for
signaling the property and the rest of the logic in this implementation.

> +}
> +
>  void ofono_handsfree_set_inband_ringing(struct ofono_handsfree *hf,
>  						ofono_bool_t enabled)
>  {
> @@ -102,6 +153,7 @@ static DBusMessage *handsfree_get_properties(DBusConnection *conn,
>  	DBusMessageIter dict;
>  	dbus_bool_t inband_ringing;
>  	dbus_bool_t voice_recognition;
> +	char **features;
>  
>  	reply = dbus_message_new_method_return(msg);
>  	if (reply == NULL)
> @@ -121,6 +173,12 @@ static DBusMessage *handsfree_get_properties(DBusConnection *conn,
>  	ofono_dbus_dict_append(&dict, "VoiceRecognition", DBUS_TYPE_BOOLEAN,
>  				&voice_recognition);
>  
> +	features = g_new0(char *, MAX_AG_FEATURE_COUNT + 1);
> +	append_ag_features(hf, features);
> +	ofono_dbus_dict_append_array(&dict, "Features", DBUS_TYPE_STRING,
> +					&features);
> +	g_free(features);
> +
>  	dbus_message_iter_close_container(&iter, &dict);
>  
>  	return reply;

I went ahead and pushed my own version of this patch.  Please review it
and let me know if you find any problems.

Regards,
-Denis