[pulseaudio-discuss] [PATCH v2 3/3] bluetooth: add correct HFP rfcomm negotiation

[Tanu Kaskinen]

On Sat, 2016-08-20 at 15:05 -0700, James Bottomley wrote:
> +static int 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 0;
> +    } else if (c->state == 1 && pa_strcont("AT+CIND=?", buf)) {
> +	  /* we declare minimal indicators */
> +	rfcomm_write(fd, "+CIND: (\"call\",(0,1))");
> +	c->state = 2;
> +	return 0;
> +    } else if (c->state == 2 && pa_strcont("AT+CIND?", buf)) {
> +	rfcomm_write(fd, "+CIND: 0");
> +	c->state = 3;
> +	return 0;
> +    } else if (c->state == 3 && pa_strcont("AT+CMER=", buf)) {
> +	rfcomm_write(fd, "OK");
> +	c->state = 4;
> +	transport_put(t);
> +	return 1;
> +    }
> +
> +
> +    if (c->state != 4)
> +	goto error;
> +
> +    /* misc things to handle in fully connected state */
> +
> +    if (pa_strcont("AT+BTRH?", buf)) {
> +	return 0;
> +    } else if (pa_strcont("AT+CHLD=?", buf)) {
> +	return 0;
> +    }

The commands are queries, so shouldn't they receive some kind of a
reply (in addition to just "OK")?

Now that I've read some of the HFP spec, I believe there are other
commands (possibly many of them) that we may encounter too after the
initial setup, for example AT+CMER. How did these two commands end up
being handled specially? Did your headset not work without this code?
It seems likely that other headsets will have other requirements, which
is why I don't feel comfortable switching the majority of headsets from
HSP to this partial HFP implementation by default.

One possibility for avoiding manual configuration in HFP-only use cases
would be to add some logic to automatically register the HFP profile
when we notice that a HFP-only headset is being used.

Tabs are still appearing here.

> @@ -246,16 +358,18 @@ static void rfcomm_io_callback(pa_mainloop_api *io, pa_io_event *e, int fd, pa_i
>          } else if (sscanf(buf, "AT+VGM=%d", &gain) == 1) {
>            t->microphone_gain = gain;
>            pa_hook_fire(pa_bluetooth_discovery_hook(t->device->discovery, PA_BLUETOOTH_HOOK_TRANSPORT_MICROPHONE_GAIN_CHANGED), t);
> -        }
> -
> -        pa_log_debug("RFCOMM >> OK");
> -
> -        len = write(fd, "\r\nOK\r\n", 6);
> +        } else if (t->config) {

The assumption here is that non-NULL config means that we're using HFP
rather than HSP, right? I think that should be clarified with a
comment, or in some other way.

> +	    switch (hfp_rfcomm_handle(fd, t, buf)) {
> +	    case -1:
> +		goto fail;

hfp_rfcomm_handle() never returns -1, so this looks weird.

> +	    case 1:
> +		return;
> +	    }
> +	}
> +	rfcomm_write(fd, "OK");
>  
>          /* we ignore any errors, it's not critical and real errors should
>           * be caught with the HANGUP and ERROR events handled above */

This comment talks about errors from write(), but since the write()
call moved elsewhere, this comment doesn't make much sense here
anymore.

> @@ -381,9 +495,14 @@ static DBusMessage *profile_new_connection(DBusConnection *conn, DBusMessage *m,
>          rfcomm_io_callback, t);
>      t->userdata =  trfc;
>  
> -    pa_bluetooth_transport_put(t);
> +    if (!is_hfp)
> +	transport_put(t);
> +    else {
> +	pa_log_debug("beginning sleep");
> +	sleep(1);
> +	pa_log_debug("ending sleep");

What's this about?

> +/* see if the string b contains a as a prefix */
> +static inline bool pa_strcont(const char *a, const char *b) {
> +    if (a == NULL || b == NULL)
> +	return a == b;
> +    return !strncmp(a, b, strlen(a));
> +}

We already have pa_startswith().

-- 
Tanu