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

[Tanu Kaskinen]

On Tue, 2016-08-23 at 08:37 -0400, James Bottomley wrote:
> 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:
> > > > 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)

Ok, so my assumption that every "Read" command expects a result in
addition to OK was wrong, and indeed plain OK is the only sensible
result when there are no calls on hold.

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

Ok. I couldn't find a statement from the HFP spec about how "we don't
support any of the +CHLD values" should be reported, but judging from
the 3GPP 27.007 spec, it looks like the response to AT+CHLD=? is
optional.

AT+CIND=? is pretty similar in that we'd rather not pretend to support
any indicators, but the 3GPP spec doesn't mark the reply as optional in
that case, and the list of indicators has to have at least one entry.
However, "if MT is not currently reachable, +CME ERROR: <err> is
returned" - maybe returning +CME ERROR for AT+CIND=? would make more
sense?

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

Why should we only ever get one? Why should the headset never send
3,0,0,0 to disable event reporting? (If you're going to change the code
to respond OK to everything, this question becomes moot, but still I'm
somewhat interested in how we can make assumptions about what commands
headsets will or will not use.)

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

That sounds good, if it removes the need for special handling for a
headset-specific set of commands.

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

I'm not sure if you understood what I meant. I suggested not
registering HFP support until a HFP-only headset appears. Your response
sounds more like you thought that I was suggesting enabling HFP by
default as your patches currently do, but I'm not sure, maybe "let's
just do it" means that you'll happily write the additional logic of
adding HFP support only after pulseaudio encounters an HFP-only
headset.

-- 
Tanu