[v5,3/4] bluetooth: add correct HFP rfcomm negotiation

Submitted by James Bottomley on Sept. 21, 2017, 7:28 p.m.

Details

Message ID 1506022094.3848.69.camel@HansenPartnership.com
State New
Headers show
Series "use bluetooth HFP in pulseaudio native backend when available" ( rev: 4 3 2 1 ) in PulseAudio

Not browsing as part of any series.

Commit Message

James Bottomley Sept. 21, 2017, 7:28 p.m.
HFP 1.6 requires a stateful negotiation of AT commands.  The prior
version got away with initialising HFP simply by replying 'OK' to
every negotiation attempt.  This one actually tries to parse the state
and make sure the negotiation occurs correctly

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---

v4:

- Update for PA 11.0
- Finally sort out CIND negotiaton for complex headsets

v3:

- remove internal debugging
- added comment for t->config being not null for hfp
- removed unused returns from hfp_rfcomm_handle()
- remove rfcomm comment
- use pa_startswith
- simplify negotiation
---
 src/modules/bluetooth/backend-native.c | 124 +++++++++++++++++++++++++++++++--
 src/modules/bluetooth/bluez5-util.c    |   5 +-
 src/modules/bluetooth/bluez5-util.h    |   2 +-
 3 files changed, 125 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/modules/bluetooth/backend-native.c b/src/modules/bluetooth/backend-native.c
index 9ec9244b..99efa066 100644
--- a/src/modules/bluetooth/backend-native.c
+++ b/src/modules/bluetooth/backend-native.c
@@ -53,6 +53,43 @@  struct transport_data {
     pa_mainloop_api *mainloop;
 };
 
+struct hfp_config {
+    uint32_t capabilities;
+    int state;
+};
+
+/*
+ * the separate hansfree headset (HF) and Audio Gateway (AG) features
+ */
+enum hfp_hf_features {
+    HFP_HF_EC_NR = 0,
+    HFP_HF_CALL_WAITING = 1,
+    HFP_HF_CLI = 2,
+    HFP_HF_VR = 3,
+    HFP_HF_RVOL = 4,
+    HFP_HF_ESTATUS = 5,
+    HFP_HF_ECALL = 6,
+    HFP_HF_CODECS = 7,
+};
+
+enum hfp_ag_features {
+    HFP_AG_THREE_WAY = 0,
+    HFP_AG_EC_NR = 1,
+    HFP_AG_VR = 2,
+    HFP_AG_RING = 3,
+    HFP_AG_NUM_TAG = 4,
+    HFP_AG_REJECT = 5,
+    HFP_AG_ESTATUS = 6,
+    HFP_AG_ECALL = 7,
+    HFP_AG_EERR = 8,
+    HFP_AG_CODECS = 9,
+};
+
+/* gateway features we support, which is as little as we can get away with */
+static uint32_t hfp_features =
+    /* HFP 1.6 requires this */
+    (1 << HFP_AG_ESTATUS );
+
 #define BLUEZ_SERVICE "org.bluez"
 #define BLUEZ_MEDIA_TRANSPORT_INTERFACE BLUEZ_SERVICE ".MediaTransport1"
 
@@ -109,6 +146,27 @@  static pa_dbus_pending* send_and_add_to_pending(pa_bluetooth_backend *backend, D
     return p;
 }
 
+static void rfcomm_write(int fd, const char *str)
+{
+    size_t len;
+    char buf[512];
+
+    pa_log_debug("RFCOMM >> %s", str);
+    sprintf(buf, "\r\n%s\r\n", str);
+    len = write(fd, buf, strlen(buf));
+
+    if (len != strlen(buf))
+        pa_log_error("RFCOMM write error: %s", pa_cstrerror(errno));
+}
+
+static void hfp_send_features(int fd)
+{
+    char buf[512];
+
+    sprintf(buf, "+BRSF: %d", hfp_features);
+    rfcomm_write(fd, buf);
+}
+
 static int sco_do_connect(pa_bluetooth_transport *t) {
     pa_bluetooth_device *d = t->device;
     struct sockaddr_sco addr;
@@ -352,6 +410,61 @@  static void register_profile(pa_bluetooth_backend *b, const char *profile, const
     send_and_add_to_pending(b, m, register_profile_reply, pa_xstrdup(profile));
 }
 
+static void transport_put(pa_bluetooth_transport *t)
+{
+    pa_bluetooth_transport_put(t);
+
+    pa_log_debug("Transport %s available for profile %s", t->path, pa_bluetooth_profile_to_string(t->profile));
+}
+
+static bool hfp_rfcomm_handle(int fd, pa_bluetooth_transport *t, const char *buf)
+{
+    struct hfp_config *c = t->config;
+    int val;
+
+    /* stateful negotiation */
+    if (c->state == 0 && sscanf(buf, "AT+BRSF=%d", &val) == 1) {
+          c->capabilities = val;
+          pa_log_info("HFP capabilities returns 0x%x", val);
+          hfp_send_features(fd);
+          c->state = 1;
+          return true;
+    } else if (c->state == 1 && pa_startswith(buf, "AT+CIND=?")) {
+          /* we declare minimal no indicators */
+        rfcomm_write(fd, "+CIND: "
+                     /* many indicators can be supported, only call and
+                      * callheld are mandatory, so that's all we repy */
+                     "(\"call\",(0-1)),"
+                     "(\"callheld\",(0-2))");
+        c->state = 2;
+        return true;
+    } else if (c->state == 2 && pa_startswith(buf, "AT+CIND?")) {
+        rfcomm_write(fd, "+CIND: 0,0");
+        c->state = 3;
+        return true;
+    } else if ((c->state == 2 || c->state == 3) && pa_startswith(buf, "AT+CMER=")) {
+        rfcomm_write(fd, "\r\nOK\r\n");
+        c->state = 4;
+        transport_put(t);
+        return false;
+    }
+
+    /* if we get here, negotiation should be complete */
+    if (c->state != 4) {
+        pa_log_error("HFP negotiation failed in state %d with inbound %s\n",
+                     c->state, buf);
+        rfcomm_write(fd, "ERROR");
+        return false;
+    }
+
+    /*
+     * once we're fully connected, just reply OK to everything
+     * it will just be the headset sending the occasional status
+     * update, but we process only the ones we care about
+     */
+    return true;
+}
+
 static void rfcomm_io_callback(pa_mainloop_api *io, pa_io_event *e, int fd, pa_io_event_flags_t events, void *userdata) {
     pa_bluetooth_transport *t = userdata;
 
@@ -398,6 +511,8 @@  static void rfcomm_io_callback(pa_mainloop_api *io, pa_io_event *e, int fd, pa_i
             do_reply = true;
         } else if (sscanf(buf, "AT+CKPD=%d", &dummy) == 1) {
             do_reply = true;
+        } else if (t->config) { /* t->config is only non-null for hfp profile */
+            do_reply = hfp_rfcomm_handle(fd, t, buf);
         } else {
             do_reply = false;
         }
@@ -540,7 +655,9 @@  static DBusMessage *profile_new_connection(DBusConnection *conn, DBusMessage *m,
     sender = dbus_message_get_sender(m);
 
     pathfd = pa_sprintf_malloc ("%s/fd%d", path, fd);
-    t = pa_bluetooth_transport_new(d, sender, pathfd, p, NULL, 0);
+    t = pa_bluetooth_transport_new(d, sender, pathfd, p, NULL,
+                                   p == PA_BLUETOOTH_PROFILE_HFP_HF ?
+                                   sizeof(struct hfp_config) : 0);
     pa_xfree(pathfd);
 
     t->acquire = sco_acquire_cb;
@@ -558,9 +675,8 @@  static DBusMessage *profile_new_connection(DBusConnection *conn, DBusMessage *m,
 
     sco_listen(t);
 
-    pa_bluetooth_transport_put(t);
-
-    pa_log_debug("Transport %s available for profile %s", t->path, pa_bluetooth_profile_to_string(t->profile));
+    if (p != PA_BLUETOOTH_PROFILE_HFP_HF)
+        transport_put(t);
 
     pa_assert_se(r = dbus_message_new_method_return(m));
 
diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
index 80a025d5..8be8a11d 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -150,7 +150,10 @@  pa_bluetooth_transport *pa_bluetooth_transport_new(pa_bluetooth_device *d, const
 
     if (size > 0) {
         t->config = pa_xnew(uint8_t, size);
-        memcpy(t->config, config, size);
+        if (config)
+            memcpy(t->config, config, size);
+        else
+            memset(t->config, 0, size);
     }
 
     return t;
diff --git a/src/modules/bluetooth/bluez5-util.h b/src/modules/bluetooth/bluez5-util.h
index b077ca2c..23f9a798 100644
--- a/src/modules/bluetooth/bluez5-util.h
+++ b/src/modules/bluetooth/bluez5-util.h
@@ -73,7 +73,7 @@  struct pa_bluetooth_transport {
     pa_bluetooth_profile_t profile;
 
     uint8_t codec;
-    uint8_t *config;
+    void *config;
     size_t config_size;
 
     uint16_t microphone_gain;

Comments

Hi James,

On Thu, Sep 21, 2017 at 10:28 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> HFP 1.6 requires a stateful negotiation of AT commands.  The prior
> version got away with initialising HFP simply by replying 'OK' to
> every negotiation attempt.  This one actually tries to parse the state
> and make sure the negotiation occurs correctly
>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>
> ---
>
> v4:
>
> - Update for PA 11.0
> - Finally sort out CIND negotiaton for complex headsets
>
> v3:
>
> - remove internal debugging
> - added comment for t->config being not null for hfp
> - removed unused returns from hfp_rfcomm_handle()
> - remove rfcomm comment
> - use pa_startswith
> - simplify negotiation
> ---
>  src/modules/bluetooth/backend-native.c | 124 +++++++++++++++++++++++++++++++--
>  src/modules/bluetooth/bluez5-util.c    |   5 +-
>  src/modules/bluetooth/bluez5-util.h    |   2 +-
>  3 files changed, 125 insertions(+), 6 deletions(-)
>
> diff --git a/src/modules/bluetooth/backend-native.c b/src/modules/bluetooth/backend-native.c
> index 9ec9244b..99efa066 100644
> --- a/src/modules/bluetooth/backend-native.c
> +++ b/src/modules/bluetooth/backend-native.c
> @@ -53,6 +53,43 @@ struct transport_data {
>      pa_mainloop_api *mainloop;
>  };
>
> +struct hfp_config {
> +    uint32_t capabilities;
> +    int state;
> +};
> +
> +/*
> + * the separate hansfree headset (HF) and Audio Gateway (AG) features
> + */
> +enum hfp_hf_features {
> +    HFP_HF_EC_NR = 0,
> +    HFP_HF_CALL_WAITING = 1,
> +    HFP_HF_CLI = 2,
> +    HFP_HF_VR = 3,
> +    HFP_HF_RVOL = 4,
> +    HFP_HF_ESTATUS = 5,
> +    HFP_HF_ECALL = 6,
> +    HFP_HF_CODECS = 7,
> +};
> +
> +enum hfp_ag_features {
> +    HFP_AG_THREE_WAY = 0,
> +    HFP_AG_EC_NR = 1,
> +    HFP_AG_VR = 2,
> +    HFP_AG_RING = 3,
> +    HFP_AG_NUM_TAG = 4,
> +    HFP_AG_REJECT = 5,
> +    HFP_AG_ESTATUS = 6,
> +    HFP_AG_ECALL = 7,
> +    HFP_AG_EERR = 8,
> +    HFP_AG_CODECS = 9,
> +};
> +
> +/* gateway features we support, which is as little as we can get away with */
> +static uint32_t hfp_features =
> +    /* HFP 1.6 requires this */
> +    (1 << HFP_AG_ESTATUS );
> +
>  #define BLUEZ_SERVICE "org.bluez"
>  #define BLUEZ_MEDIA_TRANSPORT_INTERFACE BLUEZ_SERVICE ".MediaTransport1"
>
> @@ -109,6 +146,27 @@ static pa_dbus_pending* send_and_add_to_pending(pa_bluetooth_backend *backend, D
>      return p;
>  }
>
> +static void rfcomm_write(int fd, const char *str)
> +{
> +    size_t len;
> +    char buf[512];
> +
> +    pa_log_debug("RFCOMM >> %s", str);
> +    sprintf(buf, "\r\n%s\r\n", str);
> +    len = write(fd, buf, strlen(buf));
> +
> +    if (len != strlen(buf))
> +        pa_log_error("RFCOMM write error: %s", pa_cstrerror(errno));
> +}
> +
> +static void hfp_send_features(int fd)
> +{
> +    char buf[512];
> +
> +    sprintf(buf, "+BRSF: %d", hfp_features);
> +    rfcomm_write(fd, buf);
> +}
> +
>  static int sco_do_connect(pa_bluetooth_transport *t) {
>      pa_bluetooth_device *d = t->device;
>      struct sockaddr_sco addr;
> @@ -352,6 +410,61 @@ static void register_profile(pa_bluetooth_backend *b, const char *profile, const
>      send_and_add_to_pending(b, m, register_profile_reply, pa_xstrdup(profile));
>  }
>
> +static void transport_put(pa_bluetooth_transport *t)
> +{
> +    pa_bluetooth_transport_put(t);
> +
> +    pa_log_debug("Transport %s available for profile %s", t->path, pa_bluetooth_profile_to_string(t->profile));
> +}
> +
> +static bool hfp_rfcomm_handle(int fd, pa_bluetooth_transport *t, const char *buf)
> +{
> +    struct hfp_config *c = t->config;
> +    int val;
> +

Was this code tested against PTS or you just got it working with some
specific headsets? Im guessing this is for SLC handling but Im afraid
this was never really qualified properly so perhaps some statement
here would be nice.

If you have no idea what Im talking about you please have a look at:

https://www.bluetooth.org/docman/handlers/DownloadDoc.ashx?doc_id=41165

There may be commands we would need to implement even if the
indicators are not supported to pass qualification. If this is just a
'works for me' solution then we better put a disclaimer about not
using this code on Bluetooth certified products.

> +    /* stateful negotiation */
> +    if (c->state == 0 && sscanf(buf, "AT+BRSF=%d", &val) == 1) {
> +          c->capabilities = val;
> +          pa_log_info("HFP capabilities returns 0x%x", val);
> +          hfp_send_features(fd);
> +          c->state = 1;
> +          return true;
> +    } else if (c->state == 1 && pa_startswith(buf, "AT+CIND=?")) {
> +          /* we declare minimal no indicators */
> +        rfcomm_write(fd, "+CIND: "
> +                     /* many indicators can be supported, only call and
> +                      * callheld are mandatory, so that's all we repy */
> +                     "(\"call\",(0-1)),"
> +                     "(\"callheld\",(0-2))");
> +        c->state = 2;
> +        return true;
> +    } else if (c->state == 2 && pa_startswith(buf, "AT+CIND?")) {
> +        rfcomm_write(fd, "+CIND: 0,0");
> +        c->state = 3;
> +        return true;
> +    } else if ((c->state == 2 || c->state == 3) && pa_startswith(buf, "AT+CMER=")) {
> +        rfcomm_write(fd, "\r\nOK\r\n");
> +        c->state = 4;
> +        transport_put(t);
> +        return false;
> +    }
> +
> +    /* if we get here, negotiation should be complete */
> +    if (c->state != 4) {
> +        pa_log_error("HFP negotiation failed in state %d with inbound %s\n",
> +                     c->state, buf);
> +        rfcomm_write(fd, "ERROR");
> +        return false;
> +    }
> +
> +    /*
> +     * once we're fully connected, just reply OK to everything
> +     * it will just be the headset sending the occasional status
> +     * update, but we process only the ones we care about
> +     */

Im not really sure responding "OK" to everything will do well for
interoperability, some vendors uses HFP with the its own set of vendor
commands which may not always accept OK as a response or it may
trigger some special mode.

> +    return true;
> +}
> +
>  static void rfcomm_io_callback(pa_mainloop_api *io, pa_io_event *e, int fd, pa_io_event_flags_t events, void *userdata) {
>      pa_bluetooth_transport *t = userdata;
>
> @@ -398,6 +511,8 @@ static void rfcomm_io_callback(pa_mainloop_api *io, pa_io_event *e, int fd, pa_i
>              do_reply = true;
>          } else if (sscanf(buf, "AT+CKPD=%d", &dummy) == 1) {
>              do_reply = true;
> +        } else if (t->config) { /* t->config is only non-null for hfp profile */
> +            do_reply = hfp_rfcomm_handle(fd, t, buf);
>          } else {
>              do_reply = false;
>          }
> @@ -540,7 +655,9 @@ static DBusMessage *profile_new_connection(DBusConnection *conn, DBusMessage *m,
>      sender = dbus_message_get_sender(m);
>
>      pathfd = pa_sprintf_malloc ("%s/fd%d", path, fd);
> -    t = pa_bluetooth_transport_new(d, sender, pathfd, p, NULL, 0);
> +    t = pa_bluetooth_transport_new(d, sender, pathfd, p, NULL,
> +                                   p == PA_BLUETOOTH_PROFILE_HFP_HF ?
> +                                   sizeof(struct hfp_config) : 0);
>      pa_xfree(pathfd);
>
>      t->acquire = sco_acquire_cb;
> @@ -558,9 +675,8 @@ static DBusMessage *profile_new_connection(DBusConnection *conn, DBusMessage *m,
>
>      sco_listen(t);
>
> -    pa_bluetooth_transport_put(t);
> -
> -    pa_log_debug("Transport %s available for profile %s", t->path, pa_bluetooth_profile_to_string(t->profile));
> +    if (p != PA_BLUETOOTH_PROFILE_HFP_HF)
> +        transport_put(t);
>
>      pa_assert_se(r = dbus_message_new_method_return(m));
>
> diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
> index 80a025d5..8be8a11d 100644
> --- a/src/modules/bluetooth/bluez5-util.c
> +++ b/src/modules/bluetooth/bluez5-util.c
> @@ -150,7 +150,10 @@ pa_bluetooth_transport *pa_bluetooth_transport_new(pa_bluetooth_device *d, const
>
>      if (size > 0) {
>          t->config = pa_xnew(uint8_t, size);
> -        memcpy(t->config, config, size);
> +        if (config)
> +            memcpy(t->config, config, size);
> +        else
> +            memset(t->config, 0, size);
>      }
>
>      return t;
> diff --git a/src/modules/bluetooth/bluez5-util.h b/src/modules/bluetooth/bluez5-util.h
> index b077ca2c..23f9a798 100644
> --- a/src/modules/bluetooth/bluez5-util.h
> +++ b/src/modules/bluetooth/bluez5-util.h
> @@ -73,7 +73,7 @@ struct pa_bluetooth_transport {
>      pa_bluetooth_profile_t profile;
>
>      uint8_t codec;
> -    uint8_t *config;
> +    void *config;
>      size_t config_size;
>
>      uint16_t microphone_gain;
> --
> 2.12.3
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
On Fri, 2017-09-22 at 21:09 +0300, Luiz Augusto von Dentz wrote:
> Hi James,
> 
> On Thu, Sep 21, 2017 at 10:28 PM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
[...]
> > +static bool hfp_rfcomm_handle(int fd, pa_bluetooth_transport *t,
> > const char *buf)
> > +{
> > +    struct hfp_config *c = t->config;
> > +    int val;
> > +
> 
> Was this code tested against PTS or you just got it working with some
> specific headsets? Im guessing this is for SLC handling but Im afraid
> this was never really qualified properly so perhaps some statement
> here would be nice.
> 
> If you have no idea what Im talking about you please have a look at:
> 
> https://www.bluetooth.org/docman/handlers/DownloadDoc.ashx?doc_id=411
> 65

The site seems to be login protected, but if this is some type of
simulator, it would be useful to run it by that if you can get me
access.

> There may be commands we would need to implement even if the
> indicators are not supported to pass qualification. If this is just a
> 'works for me' solution then we better put a disclaimer about not
> using this code on Bluetooth certified products.

I can ask Marcel what the usual certification route of Linux code is,
if it's important.  I wasn't aware we'd put any pulseaudio code through
bluetooth qualifications before (unless the vendors just did it
quietly).

[...]
> > +    /*
> > +     * once we're fully connected, just reply OK to everything
> > +     * it will just be the headset sending the occasional status
> > +     * update, but we process only the ones we care about
> > +     */
> 
> Im not really sure responding "OK" to everything will do well for
> interoperability, some vendors uses HFP with the its own set of
> vendor commands which may not always accept OK as a response or it
> may trigger some special mode.

I modelled that on the android code

https://android.googlesource.com/platform/packages/apps/Bluetooth/+/master/src/com/android/bluetooth/hfp/HeadsetStateMachine.java

It responds OK to any vendor specific command it doesn't recognise.

James
Hi James,

On Fri, Sep 22, 2017 at 9:29 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Fri, 2017-09-22 at 21:09 +0300, Luiz Augusto von Dentz wrote:
>> Hi James,
>>
>> On Thu, Sep 21, 2017 at 10:28 PM, James Bottomley
>> <James.Bottomley@hansenpartnership.com> wrote:
> [...]
>> > +static bool hfp_rfcomm_handle(int fd, pa_bluetooth_transport *t,
>> > const char *buf)
>> > +{
>> > +    struct hfp_config *c = t->config;
>> > +    int val;
>> > +
>>
>> Was this code tested against PTS or you just got it working with some
>> specific headsets? Im guessing this is for SLC handling but Im afraid
>> this was never really qualified properly so perhaps some statement
>> here would be nice.
>>
>> If you have no idea what Im talking about you please have a look at:
>>
>> https://www.bluetooth.org/docman/handlers/DownloadDoc.ashx?doc_id=411
>> 65
>
> The site seems to be login protected, but if this is some type of
> simulator, it would be useful to run it by that if you can get me
> access.

Hmm I thought this would be publicly accessible, it seems it requires
a Bluetooth account, anyway this is the testing spec for HFP.

>> There may be commands we would need to implement even if the
>> indicators are not supported to pass qualification. If this is just a
>> 'works for me' solution then we better put a disclaimer about not
>> using this code on Bluetooth certified products.
>
> I can ask Marcel what the usual certification route of Linux code is,
> if it's important.  I wasn't aware we'd put any pulseaudio code through
> bluetooth qualifications before (unless the vendors just did it
> quietly).

Up to now PA wouldn't be doing any PDU exchange, well except for HSP
but that is limited to just very few commands and don't really need a
state machine, but with HFP there are quite many things that can go
wrong.

> [...]
>> > +    /*
>> > +     * once we're fully connected, just reply OK to everything
>> > +     * it will just be the headset sending the occasional status
>> > +     * update, but we process only the ones we care about
>> > +     */
>>
>> Im not really sure responding "OK" to everything will do well for
>> interoperability, some vendors uses HFP with the its own set of
>> vendor commands which may not always accept OK as a response or it
>> may trigger some special mode.
>
> I modelled that on the android code
>
> https://android.googlesource.com/platform/packages/apps/Bluetooth/+/master/src/com/android/bluetooth/hfp/HeadsetStateMachine.java
>
> It responds OK to any vendor specific command it doesn't recognise.

Im not seeing anything like that, anyway it probably has all the
standard AT parsing in place, or the RIL just process them. For
instance, the AT commands to dial, etc, are all valid as well though
headsets normally don't use them carkits or other devices with an UI
probably could.

> James
>
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss