[PATCH 1/5] hfp_hf_bluez5: Add extracting version

[PATCH 1/5] hfp_hf_bluez5: Add extracting version

[Vinicius Costa Gomes]

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

From: Claudio Takahasi <claudio.takahasi@openbossa.org>

This patch parses and reads the profile "Version" that comes in the fd
dictionary of the NewConnection method. "Version" is input for Audio Card
registration.
---
 plugins/hfp_hf_bluez5.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index f068c70..a6cc156 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -350,6 +350,45 @@ static ofono_bool_t device_path_compare(struct ofono_modem *modem,
 	return g_str_equal(path, value);
 }
 
+static int get_version(DBusMessageIter *iter, uint16_t *version)
+{
+	DBusMessageIter dict, entry, valiter;
+	const char *key;
+	uint16_t value;
+
+	/* Points to dict */
+	dbus_message_iter_recurse(iter, &dict);
+
+	/* For each entry in this dict */
+	while (dbus_message_iter_get_arg_type(&dict) != DBUS_TYPE_INVALID) {
+		/* I want to access the entry's contents */
+		dbus_message_iter_recurse(&dict, &entry);
+
+		if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_STRING)
+			return -EINVAL;
+
+		/* If the current key isn't "Version", keep looking */
+		dbus_message_iter_get_basic(&entry, &key);
+		if (!g_str_equal("Version", key)) {
+			dbus_message_iter_next(&dict);
+			continue;
+		}
+
+		dbus_message_iter_next(&entry);
+		if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_VARIANT)
+			return -EINVAL;
+
+		dbus_message_iter_recurse(&entry, &valiter);
+		dbus_message_iter_get_basic(&valiter, &value);
+		break;
+	}
+
+	if (version)
+		*version = value;
+
+	return 0;
+}
+
 static DBusMessage *profile_new_connection(DBusConnection *conn,
 					DBusMessage *msg, void *user_data)
 {
@@ -360,6 +399,7 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 	DBusMessageIter entry;
 	const char *device;
 	char local[18], remote[18];
+	uint16_t version = HFP_VERSION_1_5;
 	int fd, err;
 
 	DBG("Profile handler NewConnection");
@@ -380,6 +420,13 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 	if (fd < 0)
 		goto invalid;
 
+	dbus_message_iter_next(&entry);
+	if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_ARRAY)
+		goto invalid;
+
+	if (get_version(&entry, &version) < 0)
+		goto invalid;
+
 	modem = ofono_modem_find(device_path_compare, (void *) device);
 	if (modem == NULL) {
 		close(fd);
@@ -388,7 +435,7 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 					"Unknown Bluetooth device");
 	}
 
-	err = service_level_connection(modem, fd, HFP_VERSION_LATEST);
+	err = service_level_connection(modem, fd, version);
 	if (err < 0 && err != -EINPROGRESS) {
 		close(fd);
 		return g_dbus_create_error(msg, BLUEZ_ERROR_INTERFACE
-- 
1.8.2

[PATCH 2/5] hfp_hf_bluez5: Add audio card .connect() for HFP 1.6

[Vinicius Costa Gomes]

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

---
 plugins/hfp_hf_bluez5.c | 41 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index a6cc156..4c8f412 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -67,6 +67,8 @@ struct hfp {
 	struct hfp_slc_info info;
 	DBusMessage *msg;
 	struct ofono_handsfree_card *card;
+	ofono_handsfree_card_connect_cb_t cb;
+	void *user_data;
 };
 
 static GDBusClient *bluez = NULL;
@@ -324,11 +326,48 @@ static void hfp16_card_remove(struct ofono_handsfree_card *card)
 
 }
 
+static void bcc_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct hfp *hfp = user_data;
+	struct hfp_slc_info *info = &hfp->info;
+
+	if (ok)
+		CALLBACK_WITH_SUCCESS(hfp->cb, hfp->user_data);
+	else
+		CALLBACK_WITH_FAILURE(hfp->cb, hfp->user_data);
+
+	g_at_chat_unref(info->chat);
+}
+
 static void hfp16_card_connect(struct ofono_handsfree_card *card,
 					ofono_handsfree_card_connect_cb_t cb,
 					void *data)
 {
-	CALLBACK_WITH_FAILURE(cb, data);
+	struct hfp *hfp = ofono_handsfree_card_get_data(card);
+	struct hfp_slc_info *info = &hfp->info;
+
+	if (info->chat == NULL) {
+		CALLBACK_WITH_FAILURE(cb, data);
+		return;
+	}
+
+	if (!(info->hf_features & HFP_HF_FEATURE_CODEC_NEGOTIATION &&
+			info->ag_features & HFP_AG_FEATURE_CODEC_NEGOTIATION)) {
+		/*
+		 * If both sides don't support codec negotiation,
+		 * fallback to direct SCO connection. Calling
+		 * connect_sco() hands the connection responsability
+		 * to the core, so no need to call the callback
+		 */
+		ofono_handsfree_card_connect_sco(card);
+		return;
+	}
+
+	info->chat = g_at_chat_ref(info->chat);
+	hfp->cb = cb;
+	hfp->user_data = data;
+
+	g_at_chat_send(info->chat, "AT+BCC", NULL, bcc_cb, hfp, NULL);
 }
 
 static struct ofono_handsfree_card_driver hfp16_hf_driver = {
-- 
1.8.2

[PATCH 3/5] hfp_hf_bluez5: Select the audio card driver based on version

[Vinicius Costa Gomes]

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

---
 plugins/hfp_hf_bluez5.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index 4c8f412..41ea549 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -436,7 +436,7 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 	struct sockaddr_rc saddr;
 	socklen_t optlen;
 	DBusMessageIter entry;
-	const char *device;
+	const char *device, *driver;
 	char local[18], remote[18];
 	uint16_t version = HFP_VERSION_1_5;
 	int fd, err;
@@ -510,7 +510,15 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
 
 	hfp = ofono_modem_get_data(modem);
 	hfp->msg = dbus_message_ref(msg);
-	hfp->card = ofono_handsfree_card_create(0, NULL, NULL);
+
+	driver = NULL;
+
+	if (version >= HFP_VERSION_1_6)
+		driver = HFP16_HF_DRIVER;
+
+	hfp->card = ofono_handsfree_card_create(0, driver, hfp);
+	ofono_handsfree_card_set_data(hfp->card, hfp);
+
 	ofono_handsfree_card_set_local(hfp->card, local);
 	ofono_handsfree_card_set_remote(hfp->card, remote);
 
-- 
1.8.2

[PATCH 4/5] handsfree-audio: Add function to get the codecs

[Vinicius Costa Gomes]

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

From: Claudio Takahasi <claudio.takahasi@openbossa.org>

This patch adds a new handsfree-audio public function to read the
available codecs registered by the agent.
---
 include/handsfree-audio.h |  1 +
 src/handsfree-audio.c     | 13 +++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/handsfree-audio.h b/include/handsfree-audio.h
index 49b0605..f8a731f 100644
--- a/include/handsfree-audio.h
+++ b/include/handsfree-audio.h
@@ -48,6 +48,7 @@ struct ofono_handsfree_card *ofono_handsfree_card_create(unsigned int vendor,
 							void *data);
 int ofono_handsfree_card_register(struct ofono_handsfree_card *card);
 void ofono_handsfree_card_remove(struct ofono_handsfree_card *card);
+int ofono_handsfree_get_codecs(unsigned char *buffer, int buffer_len);
 
 void ofono_handsfree_card_set_data(struct ofono_handsfree_card *card,
 					void *data);
diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
index 3f24a2a..d1a22db 100644
--- a/src/handsfree-audio.c
+++ b/src/handsfree-audio.c
@@ -541,6 +541,19 @@ void ofono_handsfree_card_remove(struct ofono_handsfree_card *card)
 	g_free(card);
 }
 
+int ofono_handsfree_get_codecs(unsigned char *buffer, int buffer_len)
+{
+	int len;
+
+	if (agent == NULL)
+		return -EIO;
+
+	len = MIN(buffer_len, agent->codecs_len);
+	memcpy(buffer, agent->codecs, len);
+
+	return len;
+}
+
 static void agent_free(struct agent *agent)
 {
 	if (agent->watch > 0)
-- 
1.8.2

[PATCH 5/5] hfpmodem: Send AT+BAC with the supported codecs

[Vinicius Costa Gomes]

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

Before, the AT+BAC command was being sent with fixed information,
now we send the command (that inform the AG of the codecs supported by
the HF) with the codecs supported by the registered Handsfree Audio
Agent.
---
 drivers/hfpmodem/slc.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/hfpmodem/slc.c b/drivers/hfpmodem/slc.c
index 646befa..619a783 100644
--- a/drivers/hfpmodem/slc.c
+++ b/drivers/hfpmodem/slc.c
@@ -34,6 +34,7 @@
 #include <ofono/log.h>
 #include <ofono/modem.h>
 #include <ofono/emulator.h>
+#include <ofono/handsfree-audio.h>
 
 #include <drivers/atmodem/atutil.h>
 
@@ -305,9 +306,27 @@ static void brsf_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	if (info->ag_features & HFP_AG_FEATURE_CODEC_NEGOTIATION &&
 			info->hf_features & HFP_HF_FEATURE_CODEC_NEGOTIATION) {
 
+		unsigned char codecs[8];
+		GString *str;
+		int i, len;
+
+		len = ofono_handsfree_get_codecs(codecs, sizeof(codecs));
+		if (len <= 0)
+			goto error;
+
+		str = g_string_new("AT+BAC=");
+
+		for (i = 0; i < (len - 1); i++)
+			g_string_append_printf(str, "%d,", codecs[i]);
+
+		g_string_append_printf(str, "%d", codecs[i]);
+
 		slc_establish_data_ref(sed);
-		g_at_chat_send(info->chat, "AT+BAC=1", NULL, bac_cb, sed,
+		g_at_chat_send(info->chat, str->str, NULL, bac_cb, sed,
 						slc_establish_data_unref);
+
+		g_string_free(str, TRUE);
+
 		return;
 	}
 
-- 
1.8.2

Re: [PATCH 2/5] hfp_hf_bluez5: Add audio card .connect() for HFP 1.6

[Denis Kenzior]

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

Hi Vinicius,

On 04/03/2013 06:24 PM, Vinicius Costa Gomes wrote:
> ---
>   plugins/hfp_hf_bluez5.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
> index a6cc156..4c8f412 100644
> --- a/plugins/hfp_hf_bluez5.c
> +++ b/plugins/hfp_hf_bluez5.c
> @@ -67,6 +67,8 @@ struct hfp {
>   	struct hfp_slc_info info;
>   	DBusMessage *msg;
>   	struct ofono_handsfree_card *card;
> +	ofono_handsfree_card_connect_cb_t cb;
> +	void *user_data;

Please use the cb_data_new mechanism instead of storing this here.

>   };
>
>   static GDBusClient *bluez = NULL;
> @@ -324,11 +326,48 @@ static void hfp16_card_remove(struct ofono_handsfree_card *card)
>
>   }
>
> +static void bcc_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct hfp *hfp = user_data;
> +	struct hfp_slc_info *info =&hfp->info;
> +
> +	if (ok)
> +		CALLBACK_WITH_SUCCESS(hfp->cb, hfp->user_data);
> +	else
> +		CALLBACK_WITH_FAILURE(hfp->cb, hfp->user_data);

Please use decode_at_error instead.

e.g. decode_at_error(&error, g_at_result_final_response(result));
callback(&error, ...);

> +
> +	g_at_chat_unref(info->chat);

No need to reference count the g_at_chat.  This is just needless overhead.

> +}
> +
>   static void hfp16_card_connect(struct ofono_handsfree_card *card,
>   					ofono_handsfree_card_connect_cb_t cb,
>   					void *data)
>   {
> -	CALLBACK_WITH_FAILURE(cb, data);
> +	struct hfp *hfp = ofono_handsfree_card_get_data(card);
> +	struct hfp_slc_info *info =&hfp->info;
> +
> +	if (info->chat == NULL) {
> +		CALLBACK_WITH_FAILURE(cb, data);
> +		return;
> +	}

Can this case even happen?

> +
> +	if (!(info->hf_features&  HFP_HF_FEATURE_CODEC_NEGOTIATION&&
> +			info->ag_features&  HFP_AG_FEATURE_CODEC_NEGOTIATION)) {
> +		/*
> +		 * If both sides don't support codec negotiation,
> +		 * fallback to direct SCO connection. Calling
> +		 * connect_sco() hands the connection responsability
> +		 * to the core, so no need to call the callback
> +		 */
> +		ofono_handsfree_card_connect_sco(card);
> +		return;
> +	}

Can we reverse this logic to make it more readable?

e.g. if (HF has CodecNegotiation and AG has CodecNegotiation) {
	send BCC;
	return;
}

> +
> +	info->chat = g_at_chat_ref(info->chat);

No need to take a reference.  Remove this part

> +	hfp->cb = cb;
> +	hfp->user_data = data;
> +

Use the cb_data_new mechanism here

> +	g_at_chat_send(info->chat, "AT+BCC", NULL, bcc_cb, hfp, NULL);

And make sure to set the destroy callback.  There are examples of this 
throughout the drivers/atmodem/* and drivers/hfpmodem/*.

>   }
>
>   static struct ofono_handsfree_card_driver hfp16_hf_driver = {

Regards,
-Denis

Re: [PATCH 1/5] hfp_hf_bluez5: Add extracting version

[Denis Kenzior]

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

Hi Vinicius,

On 04/03/2013 06:24 PM, Vinicius Costa Gomes wrote:
> From: Claudio Takahasi<claudio.takahasi@openbossa.org>
>
> This patch parses and reads the profile "Version" that comes in the fd
> dictionary of the NewConnection method. "Version" is input for Audio Card
> registration.
> ---
>   plugins/hfp_hf_bluez5.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 48 insertions(+), 1 deletion(-)
>

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH 4/5] handsfree-audio: Add function to get the codecs

[Denis Kenzior]

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

Hi Vinicius,

On 04/03/2013 06:24 PM, Vinicius Costa Gomes wrote:
> From: Claudio Takahasi<claudio.takahasi@openbossa.org>
>
> This patch adds a new handsfree-audio public function to read the
> available codecs registered by the agent.
> ---
>   include/handsfree-audio.h |  1 +
>   src/handsfree-audio.c     | 13 +++++++++++++
>   2 files changed, 14 insertions(+)

Please split up these two patches according to our patch submission 
guidelines.

>
> diff --git a/include/handsfree-audio.h b/include/handsfree-audio.h
> index 49b0605..f8a731f 100644
> --- a/include/handsfree-audio.h
> +++ b/include/handsfree-audio.h
> @@ -48,6 +48,7 @@ struct ofono_handsfree_card *ofono_handsfree_card_create(unsigned int vendor,
>   							void *data);
>   int ofono_handsfree_card_register(struct ofono_handsfree_card *card);
>   void ofono_handsfree_card_remove(struct ofono_handsfree_card *card);
> +int ofono_handsfree_get_codecs(unsigned char *buffer, int buffer_len);

This is confusing, there is an ofono_handsfree object already and this 
is not related.  Rename this into ofono_handsfree_audio_get_codecs.

>
>   void ofono_handsfree_card_set_data(struct ofono_handsfree_card *card,
>   					void *data);
> diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
> index 3f24a2a..d1a22db 100644
> --- a/src/handsfree-audio.c
> +++ b/src/handsfree-audio.c
> @@ -541,6 +541,19 @@ void ofono_handsfree_card_remove(struct ofono_handsfree_card *card)
>   	g_free(card);
>   }
>
> +int ofono_handsfree_get_codecs(unsigned char *buffer, int buffer_len)
> +{
> +	int len;
> +
> +	if (agent == NULL)
> +		return -EIO;
> +
> +	len = MIN(buffer_len, agent->codecs_len);
> +	memcpy(buffer, agent->codecs, len);
> +
> +	return len;
> +}
> +

Do we really want to memcpy here or return a const unsigned char array? 
  Also, right now we explicitly only support CVSD and mSBC, so this is 
overkill.

>   static void agent_free(struct agent *agent)
>   {
>   	if (agent->watch>  0)

Regards,
-Denis

Re: [PATCH 3/5] hfp_hf_bluez5: Select the audio card driver based on version

[Denis Kenzior]

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

Hi Vinicius,

On 04/03/2013 06:24 PM, Vinicius Costa Gomes wrote:
> ---
>   plugins/hfp_hf_bluez5.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
>

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH 2/5] hfp_hf_bluez5: Add audio card .connect() for HFP 1.6

[Vinicius Costa Gomes]

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

Hi Denis,

On 12:22 Fri 05 Apr, Denis Kenzior wrote:
> Hi Vinicius,
> 
> On 04/03/2013 06:24 PM, Vinicius Costa Gomes wrote:
> >---
> >  plugins/hfp_hf_bluez5.c | 41 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 40 insertions(+), 1 deletion(-)
> >
> >diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
> >index a6cc156..4c8f412 100644
> >--- a/plugins/hfp_hf_bluez5.c
> >+++ b/plugins/hfp_hf_bluez5.c
> >@@ -67,6 +67,8 @@ struct hfp {
> >  	struct hfp_slc_info info;
> >  	DBusMessage *msg;
> >  	struct ofono_handsfree_card *card;
> >+	ofono_handsfree_card_connect_cb_t cb;
> >+	void *user_data;
> 
> Please use the cb_data_new mechanism instead of storing this here.

Somehow I missed this. Yeah, this (and the atutil.h helpers) will simplify the
code a lot.

> 
> >  };
> >
> >  static GDBusClient *bluez = NULL;
> >@@ -324,11 +326,48 @@ static void hfp16_card_remove(struct ofono_handsfree_card *card)
> >
> >  }
> >
> >+static void bcc_cb(gboolean ok, GAtResult *result, gpointer user_data)
> >+{
> >+	struct hfp *hfp = user_data;
> >+	struct hfp_slc_info *info =&hfp->info;
> >+
> >+	if (ok)
> >+		CALLBACK_WITH_SUCCESS(hfp->cb, hfp->user_data);
> >+	else
> >+		CALLBACK_WITH_FAILURE(hfp->cb, hfp->user_data);
> 
> Please use decode_at_error instead.
> 
> e.g. decode_at_error(&error, g_at_result_final_response(result));
> callback(&error, ...);
> 
> >+
> >+	g_at_chat_unref(info->chat);
> 
> No need to reference count the g_at_chat.  This is just needless overhead.
> 
> >+}
> >+
> >  static void hfp16_card_connect(struct ofono_handsfree_card *card,
> >  					ofono_handsfree_card_connect_cb_t cb,
> >  					void *data)
> >  {
> >-	CALLBACK_WITH_FAILURE(cb, data);
> >+	struct hfp *hfp = ofono_handsfree_card_get_data(card);
> >+	struct hfp_slc_info *info =&hfp->info;
> >+
> >+	if (info->chat == NULL) {
> >+		CALLBACK_WITH_FAILURE(cb, data);
> >+		return;
> >+	}
> 
> Can this case even happen?

Not anymore. Will fix.

> 
> >+
> >+	if (!(info->hf_features&  HFP_HF_FEATURE_CODEC_NEGOTIATION&&
> >+			info->ag_features&  HFP_AG_FEATURE_CODEC_NEGOTIATION)) {
> >+		/*
> >+		 * If both sides don't support codec negotiation,
> >+		 * fallback to direct SCO connection. Calling
> >+		 * connect_sco() hands the connection responsability
> >+		 * to the core, so no need to call the callback
> >+		 */
> >+		ofono_handsfree_card_connect_sco(card);
> >+		return;
> >+	}
> 
> Can we reverse this logic to make it more readable?

Sure.

> 
> e.g. if (HF has CodecNegotiation and AG has CodecNegotiation) {
> 	send BCC;
> 	return;
> }
> 
> >+
> >+	info->chat = g_at_chat_ref(info->chat);
> 
> No need to take a reference.  Remove this part
> 
> >+	hfp->cb = cb;
> >+	hfp->user_data = data;
> >+
> 
> Use the cb_data_new mechanism here
> 
> >+	g_at_chat_send(info->chat, "AT+BCC", NULL, bcc_cb, hfp, NULL);
> 
> And make sure to set the destroy callback.  There are examples of
> this throughout the drivers/atmodem/* and drivers/hfpmodem/*.
> 
> >  }
> >
> >  static struct ofono_handsfree_card_driver hfp16_hf_driver = {
> 
> Regards,
> -Denis


Cheers,
-- 
Vinicius

Re: [PATCH 4/5] handsfree-audio: Add function to get the codecs

[Vinicius Costa Gomes]

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

Hi Denis,

On 12:32 Fri 05 Apr, Denis Kenzior wrote:
> >
> >+int ofono_handsfree_get_codecs(unsigned char *buffer, int buffer_len)
> >+{
> >+	int len;
> >+
> >+	if (agent == NULL)
> >+		return -EIO;
> >+
> >+	len = MIN(buffer_len, agent->codecs_len);
> >+	memcpy(buffer, agent->codecs, len);
> >+
> >+	return len;
> >+}
> >+
> 
> Do we really want to memcpy here or return a const unsigned char
> array?  Also, right now we explicitly only support CVSD and mSBC, so
> this is overkill.

As we can assume that we always support CVSD, one idea is to have something
like ofono_handsfree_audio_has_msbc() instead of the get_codecs() function.
How does it sound?

> 
> >  static void agent_free(struct agent *agent)
> >  {
> >  	if (agent->watch>  0)
> 
> Regards,
> -Denis


Cheers,
-- 
Vinicius

Re: [PATCH 4/5] handsfree-audio: Add function to get the codecs

[Denis Kenzior]

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

Hi Vinicius,

On 04/05/2013 03:56 PM, Vinicius Costa Gomes wrote:
> Hi Denis,
>
> On 12:32 Fri 05 Apr, Denis Kenzior wrote:
>>>
>>> +int ofono_handsfree_get_codecs(unsigned char *buffer, int buffer_len)
>>> +{
>>> +	int len;
>>> +
>>> +	if (agent == NULL)
>>> +		return -EIO;
>>> +
>>> +	len = MIN(buffer_len, agent->codecs_len);
>>> +	memcpy(buffer, agent->codecs, len);
>>> +
>>> +	return len;
>>> +}
>>> +
>>
>> Do we really want to memcpy here or return a const unsigned char
>> array?  Also, right now we explicitly only support CVSD and mSBC, so
>> this is overkill.
>
> As we can assume that we always support CVSD, one idea is to have something
> like ofono_handsfree_audio_has_msbc() instead of the get_codecs() function.
> How does it sound?

I tend to like this better.  Perhaps we can name this function a more 
generic 'has_wideband()' instead.  In case we want to provide support 
for custom codecs in the future.

>
>>
>>>   static void agent_free(struct agent *agent)
>>>   {
>>>   	if (agent->watch>   0)
>>
>> Regards,
>> -Denis
>
>
> Cheers,

Regards,
-Denis

Re: [PATCH 4/5] handsfree-audio: Add function to get the codecs

[Vinicius Costa Gomes]

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

Hi Denis,

On 16:40 Fri 05 Apr, Denis Kenzior wrote:
> Hi Vinicius,
> 
> On 04/05/2013 03:56 PM, Vinicius Costa Gomes wrote:
> >Hi Denis,
> >
> >On 12:32 Fri 05 Apr, Denis Kenzior wrote:
> >>>
> >>>+int ofono_handsfree_get_codecs(unsigned char *buffer, int buffer_len)
> >>>+{
> >>>+	int len;
> >>>+
> >>>+	if (agent == NULL)
> >>>+		return -EIO;
> >>>+
> >>>+	len = MIN(buffer_len, agent->codecs_len);
> >>>+	memcpy(buffer, agent->codecs, len);
> >>>+
> >>>+	return len;
> >>>+}
> >>>+
> >>
> >>Do we really want to memcpy here or return a const unsigned char
> >>array?  Also, right now we explicitly only support CVSD and mSBC, so
> >>this is overkill.
> >
> >As we can assume that we always support CVSD, one idea is to have something
> >like ofono_handsfree_audio_has_msbc() instead of the get_codecs() function.
> >How does it sound?
> 
> I tend to like this better.  Perhaps we can name this function a
> more generic 'has_wideband()' instead.  In case we want to provide
> support for custom codecs in the future.

Sounds good.


Cheers,
-- 
Vinicius