[PATCH 0/6] hfp_hf_bluez5: Add support for Transparent SCO

[PATCH 0/6] hfp_hf_bluez5: Add support for Transparent SCO

[Vinicius Costa Gomes]

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

Hi,

Sorry for the delay.

This series adds support for using transparent SCO, necessary for
wideband speech, the main feature is that it should do the right thing
even if the kernel doesn't have code for this.

The last two patches add some missing method implementations to the
Profile1 interface. One is more or less clear that there's nothing to
do, the other is less clear.

For .Cancel(): we only receive the .Cancel() message if the we will
not receive the NewConnection() call, so there's nothing to cancel for
the ofono side.

For .Release(): right now, this is doing nothing. Perhaps it would be
worth changing it to stop listening for SCO connections. Which would
mean to only start listening when the profile is registered. Does this
make sense?

Cheers,
Vinicius

Vinicius Costa Gomes (6):
  bluetooth: Add define for SCO voice settings
  handsfree-audio: Add setting SCO air mode
  handsfree-audio: Add support for detecting Transparent SCO support
  handsfree-audio: Set the voice setting for outgoing connections
  hfp_hf_bluez5: Handle org.bluez.Profile1.Cancel()
  hfp_hf_bluez5: Handle org.bluez.Profile1.Release()

 include/handsfree-audio.h |  2 +-
 plugins/hfp_hf_bluez5.c   | 10 +++-------
 src/bluetooth.h           |  9 +++++++++
 src/handsfree-audio.c     | 49 +++++++++++++++++++++++++++++++++++++++++------
 4 files changed, 56 insertions(+), 14 deletions(-)

-- 
1.8.3.4

[PATCH 1/6] bluetooth: Add define for SCO voice settings

[Vinicius Costa Gomes]

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

Add defines for SCO voice setting (Air Coding). Air mode "Transparent
Data" shall be supported if wide band speech is supported.
---
 src/bluetooth.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/bluetooth.h b/src/bluetooth.h
index af6f02c..825da3e 100644
--- a/src/bluetooth.h
+++ b/src/bluetooth.h
@@ -34,6 +34,15 @@
 
 #define BT_DEFER_SETUP		7
 
+
+#define BT_VOICE		11
+struct bt_voice {
+	uint16_t setting;
+};
+
+#define BT_VOICE_TRANSPARENT			0x0003
+#define BT_VOICE_CVSD_16BIT			0x0060
+
 /* BD Address */
 typedef struct {
 	uint8_t b[6];
-- 
1.8.3.4

[PATCH 2/6] handsfree-audio: Add setting SCO air mode

[Vinicius Costa Gomes]

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

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

diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
index 0bdeb99..0caa458 100644
--- a/src/handsfree-audio.c
+++ b/src/handsfree-audio.c
@@ -68,6 +68,16 @@ static GSList *drivers = 0;
 static ofono_bool_t has_wideband = FALSE;
 static int defer_setup = 1;
 
+static uint16_t codec2setting(uint8_t codec)
+{
+	switch (codec) {
+		case HFP_CODEC_CVSD:
+			return BT_VOICE_CVSD_16BIT;
+		default:
+			return BT_VOICE_TRANSPARENT;
+	}
+}
+
 static void send_new_connection(const char *card, int fd, uint8_t codec)
 {
 	DBusMessage *msg;
@@ -107,6 +117,7 @@ static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
 {
 	struct ofono_handsfree_card *card;
 	struct sockaddr_sco saddr;
+	struct bt_voice voice;
 	socklen_t alen;
 	int sk, nsk;
 	char local[18], remote[18];
@@ -150,6 +161,13 @@ static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
 		return TRUE;
 	}
 
+	memset(&voice, 0, sizeof(voice));
+	voice.setting = codec2setting(card->selected_codec);
+
+	if (setsockopt(nsk, SOL_BLUETOOTH, BT_VOICE, &voice, sizeof(voice)) < 0)
+		ofono_error("Can't set voice settings: %s (%d)",
+						strerror(errno), errno);
+
 	send_new_connection(card->path, nsk, card->selected_codec);
 	close(nsk);
 
-- 
1.8.3.4

[PATCH 3/6] handsfree-audio: Add support for detecting Transparent SCO support

[Vinicius Costa Gomes]

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

---
 include/handsfree-audio.h |  2 +-
 plugins/hfp_hf_bluez5.c   |  2 +-
 src/handsfree-audio.c     | 23 +++++++++++++++++------
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/include/handsfree-audio.h b/include/handsfree-audio.h
index 846a032..03e3b38 100644
--- a/include/handsfree-audio.h
+++ b/include/handsfree-audio.h
@@ -53,7 +53,7 @@ ofono_bool_t ofono_handsfree_card_set_codec(struct ofono_handsfree_card *card,
 
 ofono_bool_t ofono_handsfree_audio_has_wideband(void);
 
-ofono_bool_t ofono_handsfree_audio_has_defer_setup(void);
+ofono_bool_t ofono_handsfree_audio_has_transparent_sco(void);
 
 void ofono_handsfree_card_set_data(struct ofono_handsfree_card *card,
 					void *data);
diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index 40d8ce0..f08bf53 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -645,7 +645,7 @@ static void connect_handler(DBusConnection *conn, void *user_data)
 	 * Assuming that if defer_setup is supported, then SCO transparent
 	 * mode is also supported
 	*/
-	if (ofono_handsfree_audio_has_defer_setup())
+	if (ofono_handsfree_audio_has_transparent_sco())
 		features |= HFP_SDP_HF_FEATURE_WIDEBAND_SPEECH;
 
 	DBG("Registering External Profile handler ...");
diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
index 0caa458..c83086f 100644
--- a/src/handsfree-audio.c
+++ b/src/handsfree-audio.c
@@ -66,7 +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 ofono_bool_t transparent_sco = FALSE;
 
 static uint16_t codec2setting(uint8_t codec)
 {
@@ -178,7 +178,9 @@ static int sco_init(void)
 {
 	GIOChannel *sco_io;
 	struct sockaddr_sco saddr;
-	int sk;
+	struct bt_voice voice;
+	int defer_setup, sk;
+	socklen_t len;
 
 	sk = socket(PF_BLUETOOTH, SOCK_SEQPACKET | O_NONBLOCK | SOCK_CLOEXEC,
 								BTPROTO_SCO);
@@ -195,6 +197,8 @@ static int sco_init(void)
 		return -errno;
 	}
 
+	defer_setup = 1;
+
 	if (setsockopt(sk, SOL_BLUETOOTH, BT_DEFER_SETUP,
 				&defer_setup, sizeof(defer_setup)) < 0) {
 		defer_setup = 0;
@@ -202,6 +206,13 @@ static int sco_init(void)
 						strerror(errno), errno);
 	}
 
+	memset(&voice, 0, sizeof(voice));
+	len = sizeof(voice);
+
+	if (defer_setup && getsockopt(sk, SOL_BLUETOOTH, BT_VOICE,
+						&voice, &len) == 0)
+		transparent_sco = TRUE;
+
 	if (listen(sk, 5) < 0) {
 		close(sk);
 		return -errno;
@@ -585,9 +596,9 @@ ofono_bool_t ofono_handsfree_audio_has_wideband(void)
 	return has_wideband;
 }
 
-ofono_bool_t ofono_handsfree_audio_has_defer_setup(void)
+ofono_bool_t ofono_handsfree_audio_has_transparent_sco(void)
 {
-	return defer_setup == 1;
+	return transparent_sco;
 }
 
 static void agent_free(struct agent *agent)
@@ -707,12 +718,12 @@ static DBusMessage *am_agent_register(DBusConnection *conn,
 	DBG("Agent %s registered with the CODECs:%s%s", sender,
 		has_cvsd ? " CVSD" : "", has_msbc ? " mSBC" : "");
 
-	if (has_msbc && defer_setup == 1)
+	if (has_msbc && transparent_sco)
 		has_wideband = TRUE;
 	else {
 		has_wideband = FALSE;
 		DBG("Wideband speech disabled: %s", has_msbc ?
-			"no SCO defer setup support" : "no mSBC support");
+			"no Transparent SCO support" : "no mSBC support");
 	}
 
 	if (has_cvsd == FALSE) {
-- 
1.8.3.4

[PATCH 4/6] handsfree-audio: Set the voice setting for outgoing connections

[Vinicius Costa Gomes]

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

In the AG case, the voice setting needs to be set before we can
use Transparent SCO mode, which is necessary for Wideband speech
support.
---
 src/handsfree-audio.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
index c83086f..d550e6e 100644
--- a/src/handsfree-audio.c
+++ b/src/handsfree-audio.c
@@ -442,6 +442,7 @@ int ofono_handsfree_card_connect_sco(struct ofono_handsfree_card *card)
 {
 	GIOChannel *io;
 	struct sockaddr_sco addr;
+	struct bt_voice voice;
 	int sk, ret;
 
 	sk = socket(PF_BLUETOOTH, SOCK_SEQPACKET | O_NONBLOCK | SOCK_CLOEXEC,
@@ -465,6 +466,13 @@ int ofono_handsfree_card_connect_sco(struct ofono_handsfree_card *card)
 	addr.sco_family = AF_BLUETOOTH;
 	bt_str2ba(card->remote, &addr.sco_bdaddr);
 
+	memset(&voice, 0, sizeof(voice));
+	voice.setting = codec2setting(card->selected_codec);
+
+	if (setsockopt(sk, SOL_BLUETOOTH, BT_VOICE, &voice, sizeof(voice)) < 0)
+		ofono_error("Can't set voice settings: %s (%d)",
+						strerror(errno), errno);
+
 	ret = connect(sk, (struct sockaddr *) &addr, sizeof(addr));
 	if (ret < 0 && errno != EINPROGRESS) {
 		close(sk);
-- 
1.8.3.4

[PATCH 5/6] hfp_hf_bluez5: Handle org.bluez.Profile1.Cancel()

[Vinicius Costa Gomes]

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

Cancel() will be called when the ConnectProfile() is called, perhaps
even replied with a success, and the user cancels the connection
attempt.
---
 plugins/hfp_hf_bluez5.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index f08bf53..7e98a45 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -574,9 +574,7 @@ static DBusMessage *profile_cancel(DBusConnection *conn,
 {
 	DBG("Profile handler Cancel");
 
-	return g_dbus_create_error(msg, BLUEZ_ERROR_INTERFACE
-					".NotImplemented",
-					"Implementation not provided");
+	return dbus_message_new_method_return(msg);
 }
 
 static DBusMessage *profile_disconnection(DBusConnection *conn,
-- 
1.8.3.4

[PATCH 6/6] hfp_hf_bluez5: Handle org.bluez.Profile1.Release()

[Vinicius Costa Gomes]

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

There's nothing we can do in this case, so we only reply with a
success.
---
 plugins/hfp_hf_bluez5.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
index 7e98a45..391a74e 100644
--- a/plugins/hfp_hf_bluez5.c
+++ b/plugins/hfp_hf_bluez5.c
@@ -564,9 +564,7 @@ static DBusMessage *profile_release(DBusConnection *conn,
 {
 	DBG("Profile handler Release");
 
-	return g_dbus_create_error(msg, BLUEZ_ERROR_INTERFACE
-						".NotImplemented",
-						"Implementation not provided");
+	return dbus_message_new_method_return(msg);
 }
 
 static DBusMessage *profile_cancel(DBusConnection *conn,
-- 
1.8.3.4

Re: [PATCH 2/6] handsfree-audio: Add setting SCO air mode

[Denis Kenzior]

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

Hi Vinicius,

On 08/15/2013 09:26 PM, Vinicius Costa Gomes wrote:
> ---
>   src/handsfree-audio.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
> index 0bdeb99..0caa458 100644
> --- a/src/handsfree-audio.c
> +++ b/src/handsfree-audio.c
> @@ -68,6 +68,16 @@ static GSList *drivers = 0;
>   static ofono_bool_t has_wideband = FALSE;
>   static int defer_setup = 1;
>
> +static uint16_t codec2setting(uint8_t codec)
> +{
> +	switch (codec) {
> +		case HFP_CODEC_CVSD:
> +			return BT_VOICE_CVSD_16BIT;
> +		default:
> +			return BT_VOICE_TRANSPARENT;
> +	}
> +}
> +
>   static void send_new_connection(const char *card, int fd, uint8_t codec)
>   {
>   	DBusMessage *msg;
> @@ -107,6 +117,7 @@ static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
>   {
>   	struct ofono_handsfree_card *card;
>   	struct sockaddr_sco saddr;
> +	struct bt_voice voice;
>   	socklen_t alen;
>   	int sk, nsk;
>   	char local[18], remote[18];
> @@ -150,6 +161,13 @@ static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
>   		return TRUE;
>   	}
>
> +	memset(&voice, 0, sizeof(voice));
> +	voice.setting = codec2setting(card->selected_codec);
> +
> +	if (setsockopt(nsk, SOL_BLUETOOTH, BT_VOICE, &voice, sizeof(voice)) < 0)
> +		ofono_error("Can't set voice settings: %s (%d)",
> +						strerror(errno), errno);
> +

So two things:
- If we do not support BT_VOICE socket option, then printing an error is 
not a good idea in the first place.
- If setting BT_VOICE fails and we indeed are trying to set wideband 
codec, then why are we still sending the (improperly configured) file 
descriptor to the agent?

>   	send_new_connection(card->path, nsk, card->selected_codec);
>   	close(nsk);
>
>

Regards,
-Denis

Re: [PATCH 3/6] handsfree-audio: Add support for detecting Transparent SCO support

[Denis Kenzior]

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

Hi Vinicius,

On 08/15/2013 09:26 PM, Vinicius Costa Gomes wrote:
> ---
>   include/handsfree-audio.h |  2 +-
>   plugins/hfp_hf_bluez5.c   |  2 +-
>   src/handsfree-audio.c     | 23 +++++++++++++++++------
>   3 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/include/handsfree-audio.h b/include/handsfree-audio.h
> index 846a032..03e3b38 100644
> --- a/include/handsfree-audio.h
> +++ b/include/handsfree-audio.h
> @@ -53,7 +53,7 @@ ofono_bool_t ofono_handsfree_card_set_codec(struct ofono_handsfree_card *card,
>
>   ofono_bool_t ofono_handsfree_audio_has_wideband(void);
>
> -ofono_bool_t ofono_handsfree_audio_has_defer_setup(void);
> +ofono_bool_t ofono_handsfree_audio_has_transparent_sco(void);
>
>   void ofono_handsfree_card_set_data(struct ofono_handsfree_card *card,
>   					void *data);
> diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
> index 40d8ce0..f08bf53 100644
> --- a/plugins/hfp_hf_bluez5.c
> +++ b/plugins/hfp_hf_bluez5.c
> @@ -645,7 +645,7 @@ static void connect_handler(DBusConnection *conn, void *user_data)
>   	 * Assuming that if defer_setup is supported, then SCO transparent
>   	 * mode is also supported
>   	*/
> -	if (ofono_handsfree_audio_has_defer_setup())
> +	if (ofono_handsfree_audio_has_transparent_sco())
>   		features |= HFP_SDP_HF_FEATURE_WIDEBAND_SPEECH;

This seems fine, however we should probably not be enabling the plugin 
if defer_setup is missing.

>
>   	DBG("Registering External Profile handler ...");
> diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
> index 0caa458..c83086f 100644
> --- a/src/handsfree-audio.c
> +++ b/src/handsfree-audio.c
> @@ -66,7 +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 ofono_bool_t transparent_sco = FALSE;
>
>   static uint16_t codec2setting(uint8_t codec)
>   {
> @@ -178,7 +178,9 @@ static int sco_init(void)
>   {
>   	GIOChannel *sco_io;
>   	struct sockaddr_sco saddr;
> -	int sk;
> +	struct bt_voice voice;
> +	int defer_setup, sk;
> +	socklen_t len;
>
>   	sk = socket(PF_BLUETOOTH, SOCK_SEQPACKET | O_NONBLOCK | SOCK_CLOEXEC,
>   								BTPROTO_SCO);
> @@ -195,6 +197,8 @@ static int sco_init(void)
>   		return -errno;
>   	}
>
> +	defer_setup = 1;
> +
>   	if (setsockopt(sk, SOL_BLUETOOTH, BT_DEFER_SETUP,
>   				&defer_setup, sizeof(defer_setup)) < 0) {
>   		defer_setup = 0;
> @@ -202,6 +206,13 @@ static int sco_init(void)
>   						strerror(errno), errno);
>   	}
>
> +	memset(&voice, 0, sizeof(voice));
> +	len = sizeof(voice);
> +
> +	if (defer_setup && getsockopt(sk, SOL_BLUETOOTH, BT_VOICE,
> +						&voice, &len) == 0)
> +		transparent_sco = TRUE;
> +
>   	if (listen(sk, 5) < 0) {
>   		close(sk);
>   		return -errno;
> @@ -585,9 +596,9 @@ ofono_bool_t ofono_handsfree_audio_has_wideband(void)
>   	return has_wideband;
>   }
>
> -ofono_bool_t ofono_handsfree_audio_has_defer_setup(void)
> +ofono_bool_t ofono_handsfree_audio_has_transparent_sco(void)
>   {
> -	return defer_setup == 1;
> +	return transparent_sco;
>   }
>
>   static void agent_free(struct agent *agent)
> @@ -707,12 +718,12 @@ static DBusMessage *am_agent_register(DBusConnection *conn,
>   	DBG("Agent %s registered with the CODECs:%s%s", sender,
>   		has_cvsd ? " CVSD" : "", has_msbc ? " mSBC" : "");
>
> -	if (has_msbc && defer_setup == 1)
> +	if (has_msbc && transparent_sco)
>   		has_wideband = TRUE;
>   	else {
>   		has_wideband = FALSE;
>   		DBG("Wideband speech disabled: %s", has_msbc ?
> -			"no SCO defer setup support" : "no mSBC support");
> +			"no Transparent SCO support" : "no mSBC support");
>   	}
>
>   	if (has_cvsd == FALSE) {
>

Regards,
-Denis

Re: [PATCH 4/6] handsfree-audio: Set the voice setting for outgoing connections

[Denis Kenzior]

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

Hi Vinicius,

On 08/15/2013 09:26 PM, Vinicius Costa Gomes wrote:
> In the AG case, the voice setting needs to be set before we can
> use Transparent SCO mode, which is necessary for Wideband speech
> support.
> ---
>   src/handsfree-audio.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
> index c83086f..d550e6e 100644
> --- a/src/handsfree-audio.c
> +++ b/src/handsfree-audio.c
> @@ -442,6 +442,7 @@ int ofono_handsfree_card_connect_sco(struct ofono_handsfree_card *card)
>   {
>   	GIOChannel *io;
>   	struct sockaddr_sco addr;
> +	struct bt_voice voice;
>   	int sk, ret;
>
>   	sk = socket(PF_BLUETOOTH, SOCK_SEQPACKET | O_NONBLOCK | SOCK_CLOEXEC,
> @@ -465,6 +466,13 @@ int ofono_handsfree_card_connect_sco(struct ofono_handsfree_card *card)
>   	addr.sco_family = AF_BLUETOOTH;
>   	bt_str2ba(card->remote, &addr.sco_bdaddr);
>
> +	memset(&voice, 0, sizeof(voice));
> +	voice.setting = codec2setting(card->selected_codec);
> +
> +	if (setsockopt(sk, SOL_BLUETOOTH, BT_VOICE, &voice, sizeof(voice)) < 0)
> +		ofono_error("Can't set voice settings: %s (%d)",
> +						strerror(errno), errno);
> +

Same comment here as for patch 2.

- If we fail to set BT_VOICE and our kernel does not support it, then 
printing an error is superfluous.
- If we indeed try to set wideband codec and fail, then continuing with 
the connection is kinda bad.

>   	ret = connect(sk, (struct sockaddr *) &addr, sizeof(addr));
>   	if (ret < 0 && errno != EINPROGRESS) {
>   		close(sk);
>

Regards,
-Denis

Re: [PATCH 6/6] hfp_hf_bluez5: Handle org.bluez.Profile1.Release()

[Denis Kenzior]

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

Hi Vinicius,

On 08/15/2013 09:26 PM, Vinicius Costa Gomes wrote:
> There's nothing we can do in this case, so we only reply with a
> success.
> ---
>   plugins/hfp_hf_bluez5.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
> index 7e98a45..391a74e 100644
> --- a/plugins/hfp_hf_bluez5.c
> +++ b/plugins/hfp_hf_bluez5.c
> @@ -564,9 +564,7 @@ static DBusMessage *profile_release(DBusConnection *conn,
>   {
>   	DBG("Profile handler Release");
>
> -	return g_dbus_create_error(msg, BLUEZ_ERROR_INTERFACE
> -						".NotImplemented",
> -						"Implementation not provided");
> +	return dbus_message_new_method_return(msg);

To me it seems like this method is not properly labeled as NOREPLY 
inside the BlueZ API specification.  Sending a reply here seems pointless.

>   }
>
>   static DBusMessage *profile_cancel(DBusConnection *conn,
>

Regards,
-Denis

Re: [PATCH 1/6] bluetooth: Add define for SCO voice settings

[Denis Kenzior]

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

Hi Vinicius,

On 08/15/2013 09:26 PM, Vinicius Costa Gomes wrote:
> Add defines for SCO voice setting (Air Coding). Air mode "Transparent
> Data" shall be supported if wide band speech is supported.
> ---
>   src/bluetooth.h | 9 +++++++++
>   1 file changed, 9 insertions(+)
>

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH 2/6] handsfree-audio: Add setting SCO air mode

[Vinicius Costa Gomes]

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

Hi Denis,

On 12:49 Mon 19 Aug, Denis Kenzior wrote:
> Hi Vinicius,
> 
> On 08/15/2013 09:26 PM, Vinicius Costa Gomes wrote:
> >---
> >  src/handsfree-audio.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> >diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
> >index 0bdeb99..0caa458 100644
> >--- a/src/handsfree-audio.c
> >+++ b/src/handsfree-audio.c
> >@@ -68,6 +68,16 @@ static GSList *drivers = 0;
> >  static ofono_bool_t has_wideband = FALSE;
> >  static int defer_setup = 1;
> >
> >+static uint16_t codec2setting(uint8_t codec)
> >+{
> >+	switch (codec) {
> >+		case HFP_CODEC_CVSD:
> >+			return BT_VOICE_CVSD_16BIT;
> >+		default:
> >+			return BT_VOICE_TRANSPARENT;
> >+	}
> >+}
> >+
> >  static void send_new_connection(const char *card, int fd, uint8_t codec)
> >  {
> >  	DBusMessage *msg;
> >@@ -107,6 +117,7 @@ static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
> >  {
> >  	struct ofono_handsfree_card *card;
> >  	struct sockaddr_sco saddr;
> >+	struct bt_voice voice;
> >  	socklen_t alen;
> >  	int sk, nsk;
> >  	char local[18], remote[18];
> >@@ -150,6 +161,13 @@ static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
> >  		return TRUE;
> >  	}
> >
> >+	memset(&voice, 0, sizeof(voice));
> >+	voice.setting = codec2setting(card->selected_codec);
> >+
> >+	if (setsockopt(nsk, SOL_BLUETOOTH, BT_VOICE, &voice, sizeof(voice)) < 0)
> >+		ofono_error("Can't set voice settings: %s (%d)",
> >+						strerror(errno), errno);
> >+
> 
> So two things:
> - If we do not support BT_VOICE socket option, then printing an
> error is not a good idea in the first place.
> - If setting BT_VOICE fails and we indeed are trying to set wideband
> codec, then why are we still sending the (improperly configured)
> file descriptor to the agent?

I agree with both points. Going to fix.

> 
> >  	send_new_connection(card->path, nsk, card->selected_codec);
> >  	close(nsk);
> >
> >
> 
> Regards,
> -Denis


Cheers,
--
Vinicius

Re: [PATCH 6/6] hfp_hf_bluez5: Handle org.bluez.Profile1.Release()

[Vinicius Costa Gomes]

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

Hi Denis, 

On 12:57 Mon 19 Aug, Denis Kenzior wrote:
> Hi Vinicius,
> 
> On 08/15/2013 09:26 PM, Vinicius Costa Gomes wrote:
> >There's nothing we can do in this case, so we only reply with a
> >success.
> >---
> >  plugins/hfp_hf_bluez5.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> >diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
> >index 7e98a45..391a74e 100644
> >--- a/plugins/hfp_hf_bluez5.c
> >+++ b/plugins/hfp_hf_bluez5.c
> >@@ -564,9 +564,7 @@ static DBusMessage *profile_release(DBusConnection *conn,
> >  {
> >  	DBG("Profile handler Release");
> >
> >-	return g_dbus_create_error(msg, BLUEZ_ERROR_INTERFACE
> >-						".NotImplemented",
> >-						"Implementation not provided");
> >+	return dbus_message_new_method_return(msg);
> 
> To me it seems like this method is not properly labeled as NOREPLY
> inside the BlueZ API specification.  Sending a reply here seems
> pointless.

I agree. Going to fix, and send a patch to BlueZ to add that label. 

> 
> >  }
> >
> >  static DBusMessage *profile_cancel(DBusConnection *conn,
> >
> 
> Regards,
> -Denis


Cheers,
-- 
Vinicius

Re: [PATCH 3/6] handsfree-audio: Add support for detecting Transparent SCO support

[Vinicius Costa Gomes]

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

Hi Denis,

On 12:55 Mon 19 Aug, Denis Kenzior wrote:
> Hi Vinicius,
> 
> On 08/15/2013 09:26 PM, Vinicius Costa Gomes wrote:
> >---
> >  include/handsfree-audio.h |  2 +-
> >  plugins/hfp_hf_bluez5.c   |  2 +-
> >  src/handsfree-audio.c     | 23 +++++++++++++++++------
> >  3 files changed, 19 insertions(+), 8 deletions(-)
> >
> >diff --git a/include/handsfree-audio.h b/include/handsfree-audio.h
> >index 846a032..03e3b38 100644
> >--- a/include/handsfree-audio.h
> >+++ b/include/handsfree-audio.h
> >@@ -53,7 +53,7 @@ ofono_bool_t ofono_handsfree_card_set_codec(struct ofono_handsfree_card *card,
> >
> >  ofono_bool_t ofono_handsfree_audio_has_wideband(void);
> >
> >-ofono_bool_t ofono_handsfree_audio_has_defer_setup(void);
> >+ofono_bool_t ofono_handsfree_audio_has_transparent_sco(void);
> >
> >  void ofono_handsfree_card_set_data(struct ofono_handsfree_card *card,
> >  					void *data);
> >diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
> >index 40d8ce0..f08bf53 100644
> >--- a/plugins/hfp_hf_bluez5.c
> >+++ b/plugins/hfp_hf_bluez5.c
> >@@ -645,7 +645,7 @@ static void connect_handler(DBusConnection *conn, void *user_data)
> >  	 * Assuming that if defer_setup is supported, then SCO transparent
> >  	 * mode is also supported
> >  	*/
> >-	if (ofono_handsfree_audio_has_defer_setup())
> >+	if (ofono_handsfree_audio_has_transparent_sco())
> >  		features |= HFP_SDP_HF_FEATURE_WIDEBAND_SPEECH;
> 
> This seems fine, however we should probably not be enabling the
> plugin if defer_setup is missing.

Makes sense. Will look into it.

> 
> >
> >  	DBG("Registering External Profile handler ...");
> >diff --git a/src/handsfree-audio.c b/src/handsfree-audio.c
> >index 0caa458..c83086f 100644
> >--- a/src/handsfree-audio.c
> >+++ b/src/handsfree-audio.c
> >@@ -66,7 +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 ofono_bool_t transparent_sco = FALSE;
> >
> >  static uint16_t codec2setting(uint8_t codec)
> >  {
> >@@ -178,7 +178,9 @@ static int sco_init(void)
> >  {
> >  	GIOChannel *sco_io;
> >  	struct sockaddr_sco saddr;
> >-	int sk;
> >+	struct bt_voice voice;
> >+	int defer_setup, sk;
> >+	socklen_t len;
> >
> >  	sk = socket(PF_BLUETOOTH, SOCK_SEQPACKET | O_NONBLOCK | SOCK_CLOEXEC,
> >  								BTPROTO_SCO);
> >@@ -195,6 +197,8 @@ static int sco_init(void)
> >  		return -errno;
> >  	}
> >
> >+	defer_setup = 1;
> >+
> >  	if (setsockopt(sk, SOL_BLUETOOTH, BT_DEFER_SETUP,
> >  				&defer_setup, sizeof(defer_setup)) < 0) {
> >  		defer_setup = 0;
> >@@ -202,6 +206,13 @@ static int sco_init(void)
> >  						strerror(errno), errno);
> >  	}
> >
> >+	memset(&voice, 0, sizeof(voice));
> >+	len = sizeof(voice);
> >+
> >+	if (defer_setup && getsockopt(sk, SOL_BLUETOOTH, BT_VOICE,
> >+						&voice, &len) == 0)
> >+		transparent_sco = TRUE;
> >+
> >  	if (listen(sk, 5) < 0) {
> >  		close(sk);
> >  		return -errno;
> >@@ -585,9 +596,9 @@ ofono_bool_t ofono_handsfree_audio_has_wideband(void)
> >  	return has_wideband;
> >  }
> >
> >-ofono_bool_t ofono_handsfree_audio_has_defer_setup(void)
> >+ofono_bool_t ofono_handsfree_audio_has_transparent_sco(void)
> >  {
> >-	return defer_setup == 1;
> >+	return transparent_sco;
> >  }
> >
> >  static void agent_free(struct agent *agent)
> >@@ -707,12 +718,12 @@ static DBusMessage *am_agent_register(DBusConnection *conn,
> >  	DBG("Agent %s registered with the CODECs:%s%s", sender,
> >  		has_cvsd ? " CVSD" : "", has_msbc ? " mSBC" : "");
> >
> >-	if (has_msbc && defer_setup == 1)
> >+	if (has_msbc && transparent_sco)
> >  		has_wideband = TRUE;
> >  	else {
> >  		has_wideband = FALSE;
> >  		DBG("Wideband speech disabled: %s", has_msbc ?
> >-			"no SCO defer setup support" : "no mSBC support");
> >+			"no Transparent SCO support" : "no mSBC support");
> >  	}
> >
> >  	if (has_cvsd == FALSE) {
> >
> 
> Regards,
> -Denis


Cheers,
-- 
Vinicius

Re: [PATCH 6/6] hfp_hf_bluez5: Handle org.bluez.Profile1.Release()

[Luiz Augusto von Dentz]

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

Hi Vinicius,

On Mon, Aug 19, 2013 at 9:47 PM, Vinicius Costa Gomes <vcgomes@gmail.com> wrote:
> Hi Denis,
>
> On 12:57 Mon 19 Aug, Denis Kenzior wrote:
>> Hi Vinicius,
>>
>> On 08/15/2013 09:26 PM, Vinicius Costa Gomes wrote:
>> >There's nothing we can do in this case, so we only reply with a
>> >success.
>> >---
>> >  plugins/hfp_hf_bluez5.c | 4 +---
>> >  1 file changed, 1 insertion(+), 3 deletions(-)
>> >
>> >diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
>> >index 7e98a45..391a74e 100644
>> >--- a/plugins/hfp_hf_bluez5.c
>> >+++ b/plugins/hfp_hf_bluez5.c
>> >@@ -564,9 +564,7 @@ static DBusMessage *profile_release(DBusConnection *conn,
>> >  {
>> >     DBG("Profile handler Release");
>> >
>> >-    return g_dbus_create_error(msg, BLUEZ_ERROR_INTERFACE
>> >-                                            ".NotImplemented",
>> >-                                            "Implementation not provided");
>> >+    return dbus_message_new_method_return(msg);
>>
>> To me it seems like this method is not properly labeled as NOREPLY
>> inside the BlueZ API specification.  Sending a reply here seems
>> pointless.
>
> I agree. Going to fix, and send a patch to BlueZ to add that label.

While at it you might consider actually handling the Release because
right now oFono doesn't react to either org.bluez disappearing nor
adapter proxy being removed so if one of those events happen without
calling RequestDisconnection oFono will keep the connection.

Before anyone asks, there is another difference between Release and
RequestDisconnection, the former should cause a forceful disconnect if
connected while the latter should disconnect gracefully so bluetoothd
will wait for the response so in theory Release can happen without
RequestDisconnection being called before.


-- 
Luiz Augusto von Dentz

Re: [PATCH 6/6] hfp_hf_bluez5: Handle org.bluez.Profile1.Release()

[Denis Kenzior]

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

Hi Luiz,

 >> I agree. Going to fix, and send a patch to BlueZ to add that label.
>
> While at it you might consider actually handling the Release because
> right now oFono doesn't react to either org.bluez disappearing nor
> adapter proxy being removed so if one of those events happen without
> calling RequestDisconnection oFono will keep the connection.
>

As long as BlueZ calls shutdown() on the socket, the resources will be 
cleaned up by the oFono HUP handler.

> Before anyone asks, there is another difference between Release and
> RequestDisconnection, the former should cause a forceful disconnect if
> connected while the latter should disconnect gracefully so bluetoothd
> will wait for the response so in theory Release can happen without
> RequestDisconnection being called before.
>

The only time BlueZ calls Release() today is when gracefully shutting 
down.  It calls shutdown() then anyway.

It might be argued that we should shut down the SCO server socket if 
BlueZ has called Release() on our profile.  Looking at the code, it 
looks like we only accept SCO connections when a given 
handsfree_audio_card matches a connection request.  So assuming the 
kernel has SCO defer setup handling, we should be okay with the current 
behavior.  Comments?

Regards,
-Denis

Re: [PATCH 6/6] hfp_hf_bluez5: Handle org.bluez.Profile1.Release()

[Luiz Augusto von Dentz]

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

Hi Denis,

On Tue, Aug 20, 2013 at 6:41 PM, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Luiz,
>
>
>>> I agree. Going to fix, and send a patch to BlueZ to add that label.
>>
>>
>> While at it you might consider actually handling the Release because
>> right now oFono doesn't react to either org.bluez disappearing nor
>> adapter proxy being removed so if one of those events happen without
>> calling RequestDisconnection oFono will keep the connection.
>>
>
> As long as BlueZ calls shutdown() on the socket, the resources will be
> cleaned up by the oFono HUP handler.

That works as long bluetoothd doesn't crash so it is probably a good
idea to at least check if the org.bluez has disappeared and cleanup if
any connection still exists. I also don't understand how the
enumeration is supposed to work in hfp_hf_bluez5, it doesn't seems to
be able to detect devices being removed as there is no proxy_removed
callback, Im missing something?

>
>> Before anyone asks, there is another difference between Release and
>> RequestDisconnection, the former should cause a forceful disconnect if
>> connected while the latter should disconnect gracefully so bluetoothd
>> will wait for the response so in theory Release can happen without
>> RequestDisconnection being called before.
>>
>
> The only time BlueZ calls Release() today is when gracefully shutting down.
> It calls shutdown() then anyway.
>
> It might be argued that we should shut down the SCO server socket if BlueZ
> has called Release() on our profile.  Looking at the code, it looks like we
> only accept SCO connections when a given handsfree_audio_card matches a
> connection request.  So assuming the kernel has SCO defer setup handling, we
> should be okay with the current behavior.  Comments?

That can create a situation where nobody else can listen on SCO even
before oFono register HFP, while this is mostly fine as platforms
should probably have a single solution for HFP it would probably be
safer to start listening to SCO only when registered so we give a nice
example how to implement external profiles properly.


-- 
Luiz Augusto von Dentz

Re: [PATCH 6/6] hfp_hf_bluez5: Handle org.bluez.Profile1.Release()

[Denis Kenzior]

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

Hi Luiz,

On 08/21/2013 05:29 AM, Luiz Augusto von Dentz wrote:
> Hi Denis,
>
> On Tue, Aug 20, 2013 at 6:41 PM, Denis Kenzior <denkenz@gmail.com> wrote:
>> Hi Luiz,
>>
>>
>>>> I agree. Going to fix, and send a patch to BlueZ to add that label.
>>>
>>>
>>> While at it you might consider actually handling the Release because
>>> right now oFono doesn't react to either org.bluez disappearing nor
>>> adapter proxy being removed so if one of those events happen without
>>> calling RequestDisconnection oFono will keep the connection.
>>>
>>
>> As long as BlueZ calls shutdown() on the socket, the resources will be
>> cleaned up by the oFono HUP handler.
>
> That works as long bluetoothd doesn't crash so it is probably a good
> idea to at least check if the org.bluez has disappeared and cleanup if
> any connection still exists. I also don't understand how the
> enumeration is supposed to work in hfp_hf_bluez5, it doesn't seems to
> be able to detect devices being removed as there is no proxy_removed
> callback, Im missing something?

Correct me if I'm wrong, but BlueZ disappearing should be handled by 
freeing all the proxies associated with it when it goes away.  e.g. 
inside gdbus/client.c message_filter().

oFono does not use proxy_removed, instead we use 
g_dbus_proxy_set_removed_watch.  This lets us avoid an unnecessary hash 
lookup.

>
>>
>>> Before anyone asks, there is another difference between Release and
>>> RequestDisconnection, the former should cause a forceful disconnect if
>>> connected while the latter should disconnect gracefully so bluetoothd
>>> will wait for the response so in theory Release can happen without
>>> RequestDisconnection being called before.
>>>
>>
>> The only time BlueZ calls Release() today is when gracefully shutting down.
>> It calls shutdown() then anyway.
>>
>> It might be argued that we should shut down the SCO server socket if BlueZ
>> has called Release() on our profile.  Looking at the code, it looks like we
>> only accept SCO connections when a given handsfree_audio_card matches a
>> connection request.  So assuming the kernel has SCO defer setup handling, we
>> should be okay with the current behavior.  Comments?
>
> That can create a situation where nobody else can listen on SCO even
> before oFono register HFP, while this is mostly fine as platforms
> should probably have a single solution for HFP it would probably be
> safer to start listening to SCO only when registered so we give a nice
> example how to implement external profiles properly.
>
>

The HFP plugin registers the profile with BlueZ on startup.  So who 
would try to open a SCO socket in that small time window?

Regards,
-Denis

Re: [PATCH 6/6] hfp_hf_bluez5: Handle org.bluez.Profile1.Release()

[Luiz Augusto von Dentz]

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

Hi Denis,

On Wed, Aug 21, 2013 at 6:21 PM, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Luiz,
>
>
> On 08/21/2013 05:29 AM, Luiz Augusto von Dentz wrote:
>>
>> Hi Denis,
>>
>> On Tue, Aug 20, 2013 at 6:41 PM, Denis Kenzior <denkenz@gmail.com> wrote:
>>>
>>> Hi Luiz,
>>>
>>>
>>>>> I agree. Going to fix, and send a patch to BlueZ to add that label.
>>>>
>>>>
>>>>
>>>> While at it you might consider actually handling the Release because
>>>> right now oFono doesn't react to either org.bluez disappearing nor
>>>> adapter proxy being removed so if one of those events happen without
>>>> calling RequestDisconnection oFono will keep the connection.
>>>>
>>>
>>> As long as BlueZ calls shutdown() on the socket, the resources will be
>>> cleaned up by the oFono HUP handler.
>>
>>
>> That works as long bluetoothd doesn't crash so it is probably a good
>> idea to at least check if the org.bluez has disappeared and cleanup if
>> any connection still exists. I also don't understand how the
>> enumeration is supposed to work in hfp_hf_bluez5, it doesn't seems to
>> be able to detect devices being removed as there is no proxy_removed
>> callback, Im missing something?
>
>
> Correct me if I'm wrong, but BlueZ disappearing should be handled by freeing
> all the proxies associated with it when it goes away.  e.g. inside
> gdbus/client.c message_filter().
>
> oFono does not use proxy_removed, instead we use
> g_dbus_proxy_set_removed_watch.  This lets us avoid an unnecessary hash
> lookup.

Well it looks like it is not working I just killed bluetoothd while
connected and nothing happened in ofono side, probably a bug in
GDBusClient code.

>
>>
>>>
>>>> Before anyone asks, there is another difference between Release and
>>>> RequestDisconnection, the former should cause a forceful disconnect if
>>>> connected while the latter should disconnect gracefully so bluetoothd
>>>> will wait for the response so in theory Release can happen without
>>>> RequestDisconnection being called before.
>>>>
>>>
>>> The only time BlueZ calls Release() today is when gracefully shutting
>>> down.
>>> It calls shutdown() then anyway.
>>>
>>> It might be argued that we should shut down the SCO server socket if
>>> BlueZ
>>> has called Release() on our profile.  Looking at the code, it looks like
>>> we
>>> only accept SCO connections when a given handsfree_audio_card matches a
>>> connection request.  So assuming the kernel has SCO defer setup handling,
>>> we
>>> should be okay with the current behavior.  Comments?
>>
>>
>> That can create a situation where nobody else can listen on SCO even
>> before oFono register HFP, while this is mostly fine as platforms
>> should probably have a single solution for HFP it would probably be
>> safer to start listening to SCO only when registered so we give a nice
>> example how to implement external profiles properly.
>>
>>
>
> The HFP plugin registers the profile with BlueZ on startup.  So who would
> try to open a SCO socket in that small time window?

Well we cannot really assume the order of startup is fixed, in fact a
lot of things can be started in parallel nowadays with systemd, so yes
it is unlikely but that doesn't mean we should just ignore it
specially because I believe it is not that hard to implement a logic
that waits the response to RegisterProfile to start listing in the SCO
socket.


-- 
Luiz Augusto von Dentz