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

[Tanu Kaskinen]

On Sun, 2016-08-21 at 16:16 -0400, James Bottomley wrote:
> 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.

I was referring specifically to AT+BTRH? and AT+CHLD=?. Is it really
correct to reply "OK" without providing the answer to the query first?
Responding "ERROR" like we do for all other unsupported commands would
seem more logical. (AT+CHLD=? is part of the initial setup sequence,
though, so I think we should support it by responding "+CHLD: ...".)

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

We don't handle AT+CMER, except as part of the initial setup sequence.
Further AT+CMER commands get "ERROR" as reply.

What makes AT+BTRH? special in that we should reply "OK" to it and
"ERROR" to all other commands? It's not part of the initial setup
sequence. If your headsets require us to reply "OK" rather than "ERROR"
to it in order to work, why wouldn't other headsets have similar
requirements for other commands?

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

True. However, that would be a problem only if

- the user actively uses multiple headsets
- one of the headsets supports both HSP and HFP, and one supports only
HFP
- the HSP+HFP headset works only with HSP

Seems very marginal case to me, and enabling HFP by default would
anyway break the HSP+HFP headset.

-- 
Tanu