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

[Tanu Kaskinen]

On Fri, 2016-08-26 at 11:04 -0400, James Bottomley wrote:
> On Wed, 2016-08-24 at 16:17 +0300, Tanu Kaskinen wrote:
> > 
> > On Tue, 2016-08-23 at 08:37 -0400, James Bottomley wrote:
> > > 
> > > On Mon, 2016-08-22 at 15:12 +0300, Tanu Kaskinen wrote:
> > > > 
> > > >  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.
> 
> Actually, if you've found spec support from only responding OK, I'll
> rework the code to do that rather than making up a pointless indicator.
>  The slight problem is what happens to the mandatory AT+CHLD?
> negotiation because I can see some headsets not sending that if we
> don't support any indicators, but I think I can cope with that in the
> sequence.

You seem to be mixing CHLD and CIND. Otherwise this paragraph doesn't
make sense to me. I found spec support for responding plain OK to
AT+CHLD=?, not AT+CIND=?. There's nothing to rework, since your
original code already responds OK to AT+CHLD=?. As for "what happens to
the mandatory AT+CHLD? negotiation" - I don't think AT+CHLD? is even a
valid command.

> > 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?
> 
> I'd really rather make up an indicator than return error, because I bet
> error in the initialisation sequence isn't usual for headsets.

This paragraph seems to be in conflict with your previous paragraph,
but anyway: I'm fine with having a "call" indicator if that seems safer
than returning an error.

> > > > > > 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.
> 
> I can do it either way.  Right at the moment, for HSP/HFP systems it
> doesn't matter.  However, as soon as I get the mSBC code working, it
> will because only HFP can do wideband audio.
> 
> I was really thinking that if it "just works" most people with HFP/HSP
> headsets would want to use HFP to get the coded, so what currently
> happens looks OK in that context.

I agree that enabling HFP by default is preferable if it works reliably
with all the headsets that have so far been working in HSP mode.

Your first version had special handling for BTRH?, which is not part of
the initial setup sequence. That looked like a headset-specific tweak,
which is a bad indication when the goal is to support all headsets. Now
it seems that we won't have any headset-specific tweaks, so I'm a bit
more open to enabling HFP by default for all.

We can also handle this by enabling HFP by default in upstream, but
making the risks clear in the release notes, and telling packagers to
disable HFP by default in their packages if they prefer not to take the
risk.

-- 
Tanu