Re: [pulseaudio-discuss] [PATCH v2 2/3] bluetooth: separate HSP and HFP
On Sat, 2016-08-20 at 15:03 -0700, James Bottomley wrote:
> When all headsets supported both HSP and HFP, life was good and we
> only needed to implement HSP in the native backend. Unfortunately
> some headsets have started supporting HFP only. Unfortuantely, we
> can't simply switch to HFP only because that might break older HSP
> only headsets meaning we need to support both HSP and HFP separately.
>
> This patch separates them from a joint profile to being two separate
> ones. The older one retains the headset_head_unit name, meaning any
> saved parameters will still select this (keeping us backward
> compatible). It also introduces a new headset_handsfree.
>
> For headsets that support both HSP and HFP, the two profiles will
> become separately visible and selectable. This will only matter once
> we start adding features to HFP that HSP can't support (like wideband
> audio).
>
> Signed-off-by:
One thing that needs fixing is the profile waiting logic - we wait
until all supported profiles are connected (or until a timeout expires)
before loading module-bluez5-device. Since we will now connect only HFP
or HSP, it doesn't make sense to wait for both of them getting
connected. The waiting logic is implemented in
pa_bluetooth_transport_set_state().
> --- a/src/modules/bluetooth/bluez5-util.c
> +++ b/src/modules/bluetooth/bluez5-util.c
> @@ -76,6 +76,8 @@
> "
> " \
> ""
>
> +bool disable_profile_hfp = false;
Global bluetooth stuff should go to the pa_bluetooth_discovery struct.
module-bluez5-discover should pass the value from the module argument
to pa_bluetooth_discovery_get(). module-bluez5-device doesn't need a
module argument, it can read it from the pa_bluetooth_discovery object
(a new getter function is needed for that).
> static const char* const valid_modargs[] = {
> "path",
> +"disable_profile_hfp",
We try to avoid negative option names on the basis that double
negatives are not nice ("disable_foo=no"), so I'd prefer
"enable_profile_hfp".
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v2 2/3] bluetooth: separate HSP and HFP
On Sun, 2016-08-21 at 15:19 +0300, Tanu Kaskinen wrote:
> On Sat, 2016-08-20 at 15:03 -0700, James Bottomley wrote:
> > static const char* const valid_modargs[] = {
> > "path",
> > +"disable_profile_hfp",
>
> We try to avoid negative option names on the basis that double
> negatives are not nice ("disable_foo=no"), so I'd prefer
> "enable_profile_hfp".
Hmm, maybe that's not a good name, if the option only affects HFP HF
support, and not HFP AG. Would "enable_profile_hfp_hf" work? It's
pretty awful from user-friendliness perspective, but I have trouble
coming up with better names.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v2 2/3] bluetooth: separate HSP and HFP
On Sun, 2016-08-21 at 21:03 +0300, Tanu Kaskinen wrote:
> On Sun, 2016-08-21 at 15:19 +0300, Tanu Kaskinen wrote:
> > On Sat, 2016-08-20 at 15:03 -0700, James Bottomley wrote:
> > > static const char* const valid_modargs[] = {
> > > "path",
> > > +"disable_profile_hfp",
> >
> > We try to avoid negative option names on the basis that double
> > negatives are not nice ("disable_foo=no"), so I'd prefer
> > "enable_profile_hfp".
>
> Hmm, maybe that's not a good name, if the option only affects HFP HF
> support, and not HFP AG. Would "enable_profile_hfp_hf" work? It's
> pretty awful from user-friendliness perspective, but I have trouble
> coming up with better names.
As long as I can put a comment saying it was your suggestion ...
However, yes, I can do it. I don't really see it as a huge problem.
descriptive options aren't bad just because they're long (unless
they're 100s of characters long).
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
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")?
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", &gain) == 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
Re: [pulseaudio-discuss] [PATCH v2 3/3] bluetooth: add correct HFP rfcomm negotiation
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.
> 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", &gain) == 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
> > *profile_new_connection(DB
Re: [pulseaudio-discuss] [PATCH v2 3/3] bluetooth: add correct HFP rfcomm negotiation
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 2/3] bluetooth: separate HSP and HFP
On Sun, 2016-08-21 at 15:19 +0300, Tanu Kaskinen wrote:
> One thing that needs fixing is the profile waiting logic - we wait
> until all supported profiles are connected (or until a timeout
> expires) before loading module-bluez5-device. Since we will now
> connect only HFP or HSP, it doesn't make sense to wait for both of
> them getting connected. The waiting logic is implemented in
> pa_bluetooth_transport_set_state().
This actually seems to be broken today. I unwound all my patches and I
still see the debug message
Aug 21 19:23:28 jarvis pulseaudio[12479]: [pulseaudio] bluez5-util.c:
Timeout expired, and device /org/bluez/hci0/dev_B8_AD_3E_8E_DE_EF still
has disconnected profiles:
meaning the timer expired even though all profiles were connected. I
think the bug is that pa_bluetooth_transport_set_state() starts the
timer when we go from 0->1 disconnected profiles, but after that, the
test (old_any_connected !=
pa_bluetooth_device_any_transport_connected(t->device) is always true
and so the timer never gets stopped if we're waiting for more than one
transport connection.
I think the fix is this.
James
---
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index 7d63f35..e8a0b3d 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -259,6 +259,7 @@ static void
device_start_waiting_for_profiles(pa_bluetooth_device *device) {
void pa_bluetooth_transport_set_state(pa_bluetooth_transport *t,
pa_bluetooth_transport_state_t state) {
bool old_any_connected;
+unsigned n_disconnected_profiles;
pa_assert(t);
@@ -274,8 +275,9 @@ void
pa_bluetooth_transport_set_state(pa_bluetooth_transport *t, pa_bluetooth_tr
pa_hook_fire(&t->device->discovery->hooks[PA_BLUETOOTH_HOOK_TRANSPORT_STATE_CHANGED],
t);
+n_disconnected_profiles = device_count_disconnected_profiles(t->device);
+
if (old_any_connected !=
pa_bluetooth_device_any_transport_connected(t->device)) {
-unsigned n_disconnected_profiles;
/* If there are profiles that are expected to get connected soon (based
* on the UUID list), we wait for a bit before announcing the new
@@ -285,8 +287,6 @@ void
pa_bluetooth_transport_set_state(pa_bluetooth_transport *t, pa_bluetooth_tr
* which would prevent module-card-restore from restoring the initial
* profile properly. */
-n_disconnected_profiles =
device_count_disconnected_profiles(t->device);
-
if (n_disconnected_profiles == 0)
device_stop_waiting_for_profiles(t->device);
@@ -294,6 +294,9 @@ void
pa_bluetooth_transport_set_state(pa_bluetooth_transport *t, pa_bluetooth_tr
device_start_waiting_for_profiles(t->device);
else
pa_hook_fire(&t->device->discovery->hooks[PA_BLUETOOTH_HOOK_DEVICE_CONNECTION_CHANGED],
t->device);
+} else if (n_disconnected_profiles == 0) {
+device_stop_waiting_for_profiles(t->device);
+
pa_hook_fire(&t->device->discovery->hooks[PA_BLUETOOTH_HOOK_DEVICE_CONNECTION_CHANGED],
t->device);
}
}
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Bluetooth Headset Problem with BlueZ-5.38 and Pulseaudio 8.0
Thanks for your information. Let me try to backport from 3.18. Regards, Hieu Le. On Sat, Aug 20, 2016 at 7:32 PM, Tanu Kaskinen wrote: > On Sat, 2016-08-20 at 14:14 +0200, Georg Chini wrote: > > On 20.08.2016 13:40, Tanu Kaskinen wrote: > > > > > > On Fri, 2016-08-19 at 13:57 +0700, Hieu Le wrote: > > > > > > > > I'm trying to get a SPK-ProHT Bluetooth speaker and headset working > on an > > > > embedded Linux board but when I failed to use pacmd set-card-profile > to > > > > choose headset_head_unit profile. Checking pulseaudio lof file, it > seems > > > > backend-native cannot connect to SCO socket. Sorry this is a very > long mail > > > > due to debug logs attached > > > The problem seems to be that connect() fails with "Protocol not > > > supported". This problem has been reported a few times, but I don't > > > know how to fix it. It seems like missing functionality in the kernel, > > > so my guess is that you're missing some bluetooth related driver. If > > > you get it working somehow, please report back how you did it. > > > > > > The bluez mailing list might be able to help better. > > > > > Hi, > > > > I remember there was a kernel problem which prevented some > > headsets from working. The bug was introduced in 3.12 and was > > fixed in 3.18. > > See also > > https://lists.freedesktop.org/archives/pulseaudio-discuss/ > 2015-March/023298.html > > Thanks! I added this bit of information here: > https://www.freedesktop.org/wiki/Software/PulseAudio/ > Documentation/User/Bluetooth/ > > -- > Tanu > ___ > pulseaudio-discuss mailing list > pulseaudio-discuss@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss > -- Hieu Le --- ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
