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

2016-08-28 Thread 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:  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
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss


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

2016-08-26 Thread James Bottomley
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.

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

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

The latest code will just respond OK to this ... it doesn't matter
because we never send event indicators, so whether the headset wants
them on or off is irrelevant to us.

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

Yes, see below ... I'll post the complete set as a v3, but this is what
I currently have.

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

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

2016-08-24 Thread 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:  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
___
pulseaudio-discuss mailing list

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

2016-08-22 Thread 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", ) == 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
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss


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

2016-08-21 Thread James Bottomley
On Sun, 2016-08-21 at 16:16 -0400, James Bottomley wrote:
> The v1.6 version of the standard contains a diagram
> which shows the minimum dialog you need.

For this standard:

https://www.bluetooth.org/docman/handlers/downloaddoc.ashx?doc_id=238193

it's figure 4.1 (service level connection establishment)

James

___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss


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

2016-08-21 Thread 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", ) == 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", ) == 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
> > 

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

2016-08-21 Thread Tanu Kaskinen
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", ) == 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")?

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

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.

Tabs are still appearing here.

> @@ -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", ) == 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.

> + switch (hfp_rfcomm_handle(fd, t, buf)) {
> + case -1:
> + goto fail;

hfp_rfcomm_handle() never returns -1, so this looks weird.

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

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

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

-- 
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss