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

[James Bottomley]

On Sun, 2016-08-21 at 23:01 +0300, Tanu Kaskinen wrote:
> 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")?

the ones that need it do: AT+BRSF= expects a +BRSF reply and the
AT+CIND ones expect +CIND: replies, but the rest are informational and
can simply be acknowledged with an 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?

We handle AT+CMER.  The v1.6 version of the standard contains a diagram
which shows the minimum dialog you need.  It's basically BRSF, two
CINDS and a CMER.  Once the CMER is sent and replied to, you can
establish audio.  The other end may optionally send a AT+CHLD=? but we
can just reply OK to that.  I actually theorise that just replying OK
to this entire negotiation sequence is fine because it indicates a
minimal but functional audio gateway, so in theory, the negotiation is
an optional best practice because the original headset code will just
reply OK on its own.

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

Not if they're spec compliant and they have to follow the standards
otherwise they wouldn't talk to phones.  Fortunately bluez was used in
early androids and it still has a headset model (in
blues/android/handsfree*) so I used that to see what android does.  The
trick for us is initialising the bare minimum so we don't get complex
call stuff from the headset.

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

It would still obscure HSP, though, if you're switching headsets
without reloading the modules.

> Tabs are still appearing here.

Sorry, I think I forgot to run the patches through a tab checker.

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

I'll add a comment.

> > +	    switch (hfp_rfcomm_handle(fd, t, buf)) {
> > +	    case -1:
> > +		goto fail;
> 
> hfp_rfcomm_handle() never returns -1, so this looks weird.

I was actually planning to add a negotiation failure state, which
would, but then figured out the timeout did it for me.

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

Yes, I'll excise it.

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

Stray debugging; I already removed it.

> > +/* 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().

Hm, OK that was one name I didn't think of looking for; I'll use it.

James