hfp_hf_bluez5: Add support for Transparent SCO

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(+)

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

src/handsfree-audio.c +11 -0

index 0bdeb99..0caa458 100644
@@ -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];

Hi Vinicius,

Quoted 37 lines · Vinicius Costa Gomes

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

src/handsfree-audio.c +11 -0

index 0bdeb99..0caa458 100644
@@ -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;
 	}
 

src/handsfree-audio.c static gboolean sco_accept(GIOChannel *io, GIOCondition cond, · line 170

Quoted 7 lines

src/handsfree-audio.c +7 -0

+	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?

src/handsfree-audio.c static gboolean sco_accept(GIOChannel *io, GIOCondition cond, · line 173

Quoted 5 lines

src/handsfree-audio.c +0 -0

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

Regards,
-Denis

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

include/handsfree-audio.h +1 -1

index 846a032..03e3b38 100644
@@ -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);

plugins/hfp_hf_bluez5.c +0 -0

index 40d8ce0..f08bf53 100644

plugins/hfp_hf_bluez5.c +0 -0

  	DBG("Registering External Profile handler ...");

src/handsfree-audio.c +15 -5

index 0caa458..c83086f 100644
@@ -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)

Hi Vinicius,

Quoted 17 lines · Vinicius Costa Gomes

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

include/handsfree-audio.h +1 -1

index 846a032..03e3b38 100644
@@ -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);
 

plugins/hfp_hf_bluez5.c static void connect_handler(DBusConnection *conn, void *user_data) · line 649

Quoted 13 lines

include/handsfree-audio.h +0 -0

 void ofono_handsfree_card_set_data(struct ofono_handsfree_card *card,
 					void *data);

plugins/hfp_hf_bluez5.c +1 -1

index 40d8ce0..f08bf53 100644
@@ -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.

Quoted 76 lines

plugins/hfp_hf_bluez5.c +0 -0

 	DBG("Registering External Profile handler ...");

src/handsfree-audio.c +17 -6

index 0caa458..c83086f 100644
@@ -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");
 	}
 

src/handsfree-audio.c static DBusMessage *am_agent_register(DBusConnection *conn, · line 729

Quoted 2 lines

src/handsfree-audio.c +0 -0

 	if (has_cvsd == FALSE) {

Regards,
-Denis

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(+)

src/handsfree-audio.c +1 -0

index c83086f..d550e6e 100644
@@ -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,

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(-)

plugins/hfp_hf_bluez5.c +0 -0

index 7e98a45..391a74e 100644

Hi Vinicius,

Quoted 14 lines · Vinicius Costa Gomes

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(-)

plugins/hfp_hf_bluez5.c +0 -0

index 7e98a45..391a74e 100644
@@ -564,9 +564,7 @@ static DBusMessage *profile_release(DBusConnection *conn,
 {
 	DBG("Profile handler Release");
 

plugins/hfp_hf_bluez5.c static DBusMessage *profile_release(DBusConnection *conn, · line 567

Quoted 4 lines

plugins/hfp_hf_bluez5.c +1 -3

-	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.

plugins/hfp_hf_bluez5.c static DBusMessage *profile_release(DBusConnection *conn, · line 570

Quoted 5 lines

plugins/hfp_hf_bluez5.c +0 -0

 }
 
 static DBusMessage *profile_cancel(DBusConnection *conn,

Regards,
-Denis

Hi Denis,

Quoted 17 lines · Denis Kenzior

Hi Vinicius,

Quoted 14 lines · Vinicius Costa Gomes

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(-)

plugins/hfp_hf_bluez5.c +0 -0

index 7e98a45..391a74e 100644
@@ -564,9 +564,7 @@ static DBusMessage *profile_release(DBusConnection *conn,
 {
    DBG("Profile handler Release");
 

Quoted 8 lines

plugins/hfp_hf_bluez5.c static DBusMessage *profile_release(DBusConnection *conn, · line 567

Quoted 4 lines

plugins/hfp_hf_bluez5.c +1 -3

-    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.

Quoted 1 line

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.

Hi Luiz,

Quoted 3 lines

Quoted 1 line

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

Quoted 5 lines

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.

Quoted 7 lines

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?

Hi Denis,

Quoted 6 lines · Denis Kenzior

Hi Luiz,

Quoted 3 lines

Quoted 1 line

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

Quoted 8 lines

Quoted 5 lines

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?

Quoted 9 lines

Quoted 8 lines

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.

Quoted 8 lines

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.

Hi Luiz,

Quoted 22 lines · Luiz Augusto von Dentz

Hi Denis,

Quoted 8 lines · Denis Kenzior

Hi Luiz,

Quoted 4 lines

Quoted 1 line

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

Quoted 8 lines

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 24 lines

Quoted 14 lines

Quoted 9 lines

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.

Quoted 8 lines

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?

Quoted 7 lines

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?