[PATCH 0/8] HFP 1.6 codec selection

[PATCH 0/8] HFP 1.6 codec selection

[Vinicius Costa Gomes]

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

Hi,

This series adds support for selecting the codec inside the card.

We still don't set the SCO socket options because the API for that is
still being discussed. The patches for that are ready, they can be
sent as a RFC if desired.

The main question with this series is how to notify the audio card
that its selected codec has changed (PATCH 1/8). It was chosen to do
it this way to avoid adding knowledge of ofono_handsfree_card to the
hfpmodem.


Cheers,


Claudio Takahasi (5):
  hfp_hf_bluez5: Register codec watcher
  hfp_hf_bluez5: Set the audio codec in the card
  handsfree-audio: Send the selected codec
  handsfree-audio: Set CVSD as default in the card
  handsfree-audio: Enable wideband speech if defer is enabled

Vinicius Costa Gomes (3):
  hfpmodem: Add codec watcher register
  include: ofono_handsfree_card_select_codec()
  handsfree-audio: Implement ofono_handsfree_card_select_codec()

 drivers/hfpmodem/slc.c    | 62 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/hfpmodem/slc.h    |  4 +++
 include/handsfree-audio.h |  2 ++
 plugins/hfp_hf_bluez5.c   | 15 ++++++++++++
 src/handsfree-audio.c     | 31 +++++++++++++++++++-----
 5 files changed, 108 insertions(+), 6 deletions(-)

-- 
1.8.2

[PATCH 1/8] hfpmodem: Add codec watcher register

[Vinicius Costa Gomes]

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

This patch adds a function to monitor when the AG sends a new codec
before establishing the SCO connection.
---
 drivers/hfpmodem/slc.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/hfpmodem/slc.h |  4 ++++
 2 files changed, 66 insertions(+)

diff --git a/drivers/hfpmodem/slc.c b/drivers/hfpmodem/slc.c
index 40b22a1..51c1373 100644
--- a/drivers/hfpmodem/slc.c
+++ b/drivers/hfpmodem/slc.c
@@ -54,6 +54,12 @@ struct slc_establish_data {
 	gpointer userdata;
 };
 
+struct codec_watch {
+	struct hfp_slc_info *slc;
+	hfp_slc_codec_watch_cb_t cb;
+	gpointer user_data;
+};
+
 void hfp_slc_info_init(struct hfp_slc_info *info, guint16 version)
 {
 	info->ag_features = 0;
@@ -347,3 +353,59 @@ void hfp_slc_establish(struct hfp_slc_info *info, hfp_slc_cb_t connect_cb,
 	g_at_chat_send(info->chat, buf, brsf_prefix,
 				brsf_cb, sed, slc_establish_data_unref);
 }
+
+static void bcs_notify(GAtResult *result, gpointer user_data)
+{
+	struct codec_watch *watch = user_data;
+	struct hfp_slc_info *info = watch->slc;
+	GAtResultIter iter;
+	char str[32];
+	int value;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+BCS:"))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &value))
+		return;
+
+	memset(str, 0, sizeof(str));
+
+	switch (value) {
+	case HFP_CODEC_MSBC:
+		if (!ofono_handsfree_audio_has_wideband()) {
+			sprintf(str, "AT+BAC=%d", HFP_CODEC_CVSD);
+			break;
+		}
+
+		/* fallthrough */
+
+	case HFP_CODEC_CVSD:
+		sprintf(str, "AT+BCS=%d", value);
+		watch->cb(value, watch->user_data);
+		break;
+
+	default:
+		if (ofono_handsfree_audio_has_wideband())
+			sprintf(str, "AT+BAC=%d,%d", HFP_CODEC_CVSD,
+							HFP_CODEC_MSBC);
+		else
+			sprintf(str, "AT+BAC=%d", HFP_CODEC_CVSD);
+	}
+
+	g_at_chat_send(info->chat, str, NULL, NULL, NULL, NULL);
+}
+
+guint hfp_slc_codec_watch_register(struct hfp_slc_info *info,
+				hfp_slc_codec_watch_cb_t cb, void *user_data)
+{
+	struct codec_watch *watch = g_new0(struct codec_watch, 1);
+
+	watch->slc = info;
+	watch->cb = cb;
+	watch->user_data = user_data;
+
+	return g_at_chat_register(info->chat, "+BCS:", bcs_notify, FALSE,
+								watch, g_free);
+}
diff --git a/drivers/hfpmodem/slc.h b/drivers/hfpmodem/slc.h
index b71ffe9..95539c8 100644
--- a/drivers/hfpmodem/slc.h
+++ b/drivers/hfpmodem/slc.h
@@ -45,6 +45,7 @@ enum hfp_indicator {
 };
 
 typedef void (*hfp_slc_cb_t)(void *userdata);
+typedef void (*hfp_slc_codec_watch_cb_t)(unsigned char codec, void *userdata);
 
 struct hfp_slc_info {
 	GAtChat *chat;
@@ -60,3 +61,6 @@ void hfp_slc_info_free(struct hfp_slc_info *info);
 
 void hfp_slc_establish(struct hfp_slc_info *info, hfp_slc_cb_t connect_cb,
 				hfp_slc_cb_t failed_cb, void *userdata);
+
+guint hfp_slc_codec_watch_register(struct hfp_slc_info *info,
+				hfp_slc_codec_watch_cb_t cb, void *user_data);
-- 
1.8.2

Re: [PATCH 1/8] hfpmodem: Add codec watcher register

[Denis Kenzior]

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

Hi Vinicius,

On 04/09/2013 03:45 PM, Vinicius Costa Gomes wrote:
> This patch adds a function to monitor when the AG sends a new codec
> before establishing the SCO connection.
> ---
>   drivers/hfpmodem/slc.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   drivers/hfpmodem/slc.h |  4 ++++
>   2 files changed, 66 insertions(+)
>
> diff --git a/drivers/hfpmodem/slc.c b/drivers/hfpmodem/slc.c
> index 40b22a1..51c1373 100644
> --- a/drivers/hfpmodem/slc.c
> +++ b/drivers/hfpmodem/slc.c
> @@ -54,6 +54,12 @@ struct slc_establish_data {
>   	gpointer userdata;
>   };
>
> +struct codec_watch {
> +	struct hfp_slc_info *slc;
> +	hfp_slc_codec_watch_cb_t cb;
> +	gpointer user_data;
> +};
> +
>   void hfp_slc_info_init(struct hfp_slc_info *info, guint16 version)
>   {
>   	info->ag_features = 0;
> @@ -347,3 +353,59 @@ void hfp_slc_establish(struct hfp_slc_info *info, hfp_slc_cb_t connect_cb,
>   	g_at_chat_send(info->chat, buf, brsf_prefix,
>   				brsf_cb, sed, slc_establish_data_unref);
>   }
> +
> +static void bcs_notify(GAtResult *result, gpointer user_data)
> +{
> +	struct codec_watch *watch = user_data;
> +	struct hfp_slc_info *info = watch->slc;
> +	GAtResultIter iter;
> +	char str[32];
> +	int value;
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "+BCS:"))
> +		return;
> +
> +	if (!g_at_result_iter_next_number(&iter,&value))
> +		return;
> +
> +	memset(str, 0, sizeof(str));
> +
> +	switch (value) {
> +	case HFP_CODEC_MSBC:
> +		if (!ofono_handsfree_audio_has_wideband()) {
> +			sprintf(str, "AT+BAC=%d", HFP_CODEC_CVSD);
> +			break;
> +		}
> +
> +		/* fallthrough */
> +
> +	case HFP_CODEC_CVSD:
> +		sprintf(str, "AT+BCS=%d", value);
> +		watch->cb(value, watch->user_data);
> +		break;
> +
> +	default:
> +		if (ofono_handsfree_audio_has_wideband())
> +			sprintf(str, "AT+BAC=%d,%d", HFP_CODEC_CVSD,
> +							HFP_CODEC_MSBC);
> +		else
> +			sprintf(str, "AT+BAC=%d", HFP_CODEC_CVSD);
> +	}
> +
> +	g_at_chat_send(info->chat, str, NULL, NULL, NULL, NULL);
> +}
> +
> +guint hfp_slc_codec_watch_register(struct hfp_slc_info *info,
> +				hfp_slc_codec_watch_cb_t cb, void *user_data)
> +{
> +	struct codec_watch *watch = g_new0(struct codec_watch, 1);
> +
> +	watch->slc = info;
> +	watch->cb = cb;
> +	watch->user_data = user_data;
> +
> +	return g_at_chat_register(info->chat, "+BCS:", bcs_notify, FALSE,
> +								watch, g_free);
> +}

Why would you do this in the SLC ?  The SLC establishment only requires 
HF to send a BAC after the BRSF has been exchanged.  We do this 
successfully in brsf_cb.  I do not see why we can't monitor +BCS inside 
plugins/hfp_hf_bluez5.c?

> diff --git a/drivers/hfpmodem/slc.h b/drivers/hfpmodem/slc.h
> index b71ffe9..95539c8 100644
> --- a/drivers/hfpmodem/slc.h
> +++ b/drivers/hfpmodem/slc.h
> @@ -45,6 +45,7 @@ enum hfp_indicator {
>   };
>
>   typedef void (*hfp_slc_cb_t)(void *userdata);
> +typedef void (*hfp_slc_codec_watch_cb_t)(unsigned char codec, void *userdata);
>
>   struct hfp_slc_info {
>   	GAtChat *chat;
> @@ -60,3 +61,6 @@ void hfp_slc_info_free(struct hfp_slc_info *info);
>
>   void hfp_slc_establish(struct hfp_slc_info *info, hfp_slc_cb_t connect_cb,
>   				hfp_slc_cb_t failed_cb, void *userdata);
> +
> +guint hfp_slc_codec_watch_register(struct hfp_slc_info *info,
> +				hfp_slc_codec_watch_cb_t cb, void *user_data);

Regards,
-Denis

Re: [PATCH 1/8] hfpmodem: Add codec watcher register

[Vinicius Costa Gomes]

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

Hi Denis,

[snip]

> >+guint hfp_slc_codec_watch_register(struct hfp_slc_info *info,
> >+				hfp_slc_codec_watch_cb_t cb, void *user_data)
> >+{
> >+	struct codec_watch *watch = g_new0(struct codec_watch, 1);
> >+
> >+	watch->slc = info;
> >+	watch->cb = cb;
> >+	watch->user_data = user_data;
> >+
> >+	return g_at_chat_register(info->chat, "+BCS:", bcs_notify, FALSE,
> >+								watch, g_free);
> >+}
> 
> Why would you do this in the SLC ?  The SLC establishment only
> requires HF to send a BAC after the BRSF has been exchanged.  We do
> this successfully in brsf_cb.  I do not see why we can't monitor
> +BCS inside plugins/hfp_hf_bluez5.c?

The only point was to avoid having any AT logic inside the hfp_hf_bluez5
plugin. Will send an updated version with this logic inside hfp_hf_bluez5. It
does indeed make the code easier to follow.


Cheers,
-- 
Vinicius

[PATCH 2/8] hfp_hf_bluez5: Register codec watcher

[Vinicius Costa Gomes]

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

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

This patch registers the codec watcher when the Handsfree Audio Card
driver probe callback gets called.
---
 plugins/hfp_hf_bluez5.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index c63b1a2..63af484 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -68,6 +68,7 @@ struct hfp {
 	struct hfp_slc_info info;
 	DBusMessage *msg;
 	struct ofono_handsfree_card *card;
+	guint codec_watch;
 };
 
 static GDBusClient *bluez = NULL;
@@ -79,6 +80,13 @@ static void hfp_debug(const char *str, void *user_data)
 	ofono_info("%s%s", prefix, str);
 }
 
+static void codec_watcher_cb(unsigned char codec, gpointer user_data)
+{
+	struct hfp *hfp = user_data;;
+
+	DBG("Card: %p selected codec: %d", hfp->card, codec);
+}
+
 static void slc_established(gpointer userdata)
 {
 	struct ofono_modem *modem = userdata;
@@ -317,6 +325,12 @@ static struct ofono_modem_driver hfp_driver = {
 static int hfp16_card_probe(struct ofono_handsfree_card *card,
 					unsigned int vendor, void *data)
 {
+	struct hfp *hfp = data;
+	struct hfp_slc_info *info = &hfp->info;
+
+	hfp->codec_watch = hfp_slc_codec_watch_register(info, codec_watcher_cb,
+									hfp);
+
 	return 0;
 }
 
-- 
1.8.2

[PATCH 3/8] include: ofono_handsfree_card_select_codec()

[Vinicius Costa Gomes]

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

This will be used by the drivers that a given codec was negotiated
for a card.
---
 include/handsfree-audio.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/handsfree-audio.h b/include/handsfree-audio.h
index 82d1312..5d8e2b2 100644
--- a/include/handsfree-audio.h
+++ b/include/handsfree-audio.h
@@ -48,6 +48,8 @@ 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_card_select_codec(struct ofono_handsfree_card *card,
+							unsigned char codec);
 
 ofono_bool_t ofono_handsfree_audio_has_wideband(void);
 
-- 
1.8.2

[PATCH 4/8] handsfree-audio: Implement ofono_handsfree_card_select_codec()

[Vinicius Costa Gomes]

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

---
 src/handsfree-audio.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
index 50be691..12d49db 100644
--- a/src/handsfree-audio.c
+++ b/src/handsfree-audio.c
@@ -49,6 +49,7 @@ struct ofono_handsfree_card {
 	char *local;
 	char *path;
 	DBusMessage *msg;
+	unsigned char selected_codec;
 	const struct ofono_handsfree_card_driver *driver;
 	void *driver_data;
 };
@@ -535,6 +536,17 @@ void ofono_handsfree_card_remove(struct ofono_handsfree_card *card)
 	g_free(card);
 }
 
+int ofono_handsfree_card_select_codec(struct ofono_handsfree_card *card,
+							unsigned char codec)
+{
+	if (card == NULL)
+		return -EINVAL;
+
+	card->selected_codec = codec;
+
+	return 0;
+}
+
 ofono_bool_t ofono_handsfree_audio_has_wideband(void)
 {
 	return has_wideband;
-- 
1.8.2

Re: [PATCH 4/8] handsfree-audio: Implement ofono_handsfree_card_select_codec()

[Denis Kenzior]

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

Hi Vinicius,

On 04/09/2013 03:45 PM, Vinicius Costa Gomes wrote:
> ---
>   src/handsfree-audio.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
>
> diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
> index 50be691..12d49db 100644
> --- a/src/handsfree-audio.c
> +++ b/src/handsfree-audio.c
> @@ -49,6 +49,7 @@ struct ofono_handsfree_card {
>   	char *local;
>   	char *path;
>   	DBusMessage *msg;
> +	unsigned char selected_codec;
>   	const struct ofono_handsfree_card_driver *driver;
>   	void *driver_data;
>   };
> @@ -535,6 +536,17 @@ void ofono_handsfree_card_remove(struct ofono_handsfree_card *card)
>   	g_free(card);
>   }
>
> +int ofono_handsfree_card_select_codec(struct ofono_handsfree_card *card,
> +							unsigned char codec)
> +{

Actually make this return void.  If the driver is trying to operate on 
an invalid object then the driver is bugged and I want it to crash 
early, not later.

> +	if (card == NULL)
> +		return -EINVAL;
> +
> +	card->selected_codec = codec;
> +
> +	return 0;
> +}
> +
>   ofono_bool_t ofono_handsfree_audio_has_wideband(void)
>   {
>   	return has_wideband;

Regards,
-Denis

[PATCH 5/8] hfp_hf_bluez5: Set the audio codec in the card

[Vinicius Costa Gomes]

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

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

This patch updates the handsfree audio card codec when the AG notifies
the codec for the succeeding incoming SCO connection.
---
 plugins/hfp_hf_bluez5.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index 63af484..85b8de9 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -85,6 +85,7 @@ static void codec_watcher_cb(unsigned char codec, gpointer user_data)
 	struct hfp *hfp = user_data;;
 
 	DBG("Card: %p selected codec: %d", hfp->card, codec);
+	ofono_handsfree_card_select_codec(hfp->card, codec);
 }
 
 static void slc_established(gpointer userdata)
-- 
1.8.2

[PATCH 6/8] handsfree-audio: Send the selected codec

[Vinicius Costa Gomes]

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

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

This patch removes the hard-coded CVSD codec, and adds the selected
codec in the NewConnection method call, notifying the agent the codec
previously selected for the audio connection.
---
 src/handsfree-audio.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
index 12d49db..c00697b 100644
--- a/src/handsfree-audio.c
+++ b/src/handsfree-audio.c
@@ -67,11 +67,10 @@ static guint sco_watch = 0;
 static GSList *drivers = 0;
 static ofono_bool_t has_wideband = FALSE;
 
-static void send_new_connection(const char *card, int fd)
+static void send_new_connection(const char *card, int fd, uint8_t codec)
 {
 	DBusMessage *msg;
 	DBusMessageIter iter;
-	uint8_t codec = HFP_CODEC_CVSD;
 
 	msg = dbus_message_new_method_call(agent->owner, agent->path,
 				HFP_AUDIO_AGENT_INTERFACE, "NewConnection");
@@ -150,7 +149,7 @@ static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
 		return TRUE;
 	}
 
-	send_new_connection(card->path, nsk);
+	send_new_connection(card->path, nsk, card->selected_codec);
 	close(nsk);
 
 	return TRUE;
@@ -252,7 +251,7 @@ static gboolean sco_connect_cb(GIOChannel *io, GIOCondition cond,
 
 	sk = g_io_channel_unix_get_fd(io);
 
-	send_new_connection(card->path, sk);
+	send_new_connection(card->path, sk, card->selected_codec);
 
 	close(sk);
 
-- 
1.8.2

[PATCH 7/8] handsfree-audio: Set CVSD as default in the card

[Vinicius Costa Gomes]

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

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

If the device doesn't support codec negotiation, the selected codec is
not being initialized. For this case set CVSD as default.
---
 src/handsfree-audio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
index c00697b..d55e6fc 100644
--- a/src/handsfree-audio.c
+++ b/src/handsfree-audio.c
@@ -342,6 +342,8 @@ struct ofono_handsfree_card *ofono_handsfree_card_create(unsigned int vendor,
 
 	card = g_new0(struct ofono_handsfree_card, 1);
 
+	card->selected_codec = HFP_CODEC_CVSD;
+
 	card_list = g_slist_prepend(card_list, card);
 
 	for (l = drivers; l; l = l->next) {
-- 
1.8.2

[PATCH 8/8] handsfree-audio: Enable wideband speech if defer is enabled

[Vinicius Costa Gomes]

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

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

Only enable support for wideband speech codecs, at this point mSBC, if
the kernel support defer setup.
---
 src/handsfree-audio.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
index d55e6fc..cbaeb1f 100644
--- a/src/handsfree-audio.c
+++ b/src/handsfree-audio.c
@@ -66,6 +66,7 @@ static GSList *card_list = 0;
 static guint sco_watch = 0;
 static GSList *drivers = 0;
 static ofono_bool_t has_wideband = FALSE;
+static int defer_setup = 1;
 
 static void send_new_connection(const char *card, int fd, uint8_t codec)
 {
@@ -159,7 +160,7 @@ static int sco_init(void)
 {
 	GIOChannel *sco_io;
 	struct sockaddr_sco saddr;
-	int sk, defer_setup = 1;
+	int sk;
 
 	sk = socket(PF_BLUETOOTH, SOCK_SEQPACKET | O_NONBLOCK | SOCK_CLOEXEC,
 								BTPROTO_SCO);
@@ -177,9 +178,11 @@ static int sco_init(void)
 	}
 
 	if (setsockopt(sk, SOL_BLUETOOTH, BT_DEFER_SETUP,
-				&defer_setup, sizeof(defer_setup)) < 0)
+				&defer_setup, sizeof(defer_setup)) < 0) {
+		defer_setup = 0;
 		ofono_warn("Can't enable deferred setup: %s (%d)",
 						strerror(errno), errno);
+	}
 
 	if (listen(sk, 5) < 0) {
 		close(sk);
@@ -661,6 +664,9 @@ static DBusMessage *am_agent_register(DBusConnection *conn,
 			has_cvsd = TRUE;
 		else if (codecs[i] != HFP_CODEC_MSBC)
 			return __ofono_error_invalid_args(msg);
+
+		if (defer_setup)
+			has_wideband = TRUE;
 	}
 
 	if (has_cvsd == FALSE) {
-- 
1.8.2