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

[James Bottomley]

On Mon, 2016-08-22 at 15:12 +0300, Tanu Kaskinen wrote:
> 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?

BTRH is the headset asking about on-hold statement.  A simple OK means
nothing is on-hold (that's 4.29.1)

>  and AT+CHLD=?.

This one is a bit weird: in theory the response should be call hold
states, but we didn't show any call held indicators, so none (i.e. just
respond OK) is the correct response.  I actually experimented with
showing some and the headset crashed.

>  Is it really correct to reply "OK" without providing the answer to 
> the query first?

A plain OK is the correct response to a lot of these per spec.

> 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: ...".)

Not unless we support held call indicators.  I could add them, but I
was trying to make the initial state as simple as possible.

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

We should only ever get one.  Actually 3,0,0,1 to enable event
reporting.  However, I've no objection to responding OK to all of them,
since we don't really do anything.

> What makes AT+BTRH? special in that we should reply "OK" to it and
> "ERROR" to all other commands?

Actually, I think we should just reply OK to all commands (a bit like
HFP).  There's no harm and we don't alter any state.

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

Yes, I agree, lets just do it and see if there's a problem.  I suspect
all phones negotiate for HFP first (android certainly does), so we'd
just be doing what every headset expects.

James