Re: [pulseaudio-discuss] [PATCH v2 2/3] bluetooth: separate HSP and HFP

2016-08-22 Thread Tanu Kaskinen 
On Sun, 2016-08-21 at 19:30 -0400, James Bottomley wrote:
> 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.

Oops! I added this code recently, but I couldn't test it properly,
because my headset most of the time connects only A2DP for some reason,
and on the one occasion when it did connect HSP, A2DP got connected
only much later.

> I think the fix is this.

Yes, that should do the trick. It will fire the CONNECTION_CHANGED hook
more often than necessary, though. I'll make a bit different patch.

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

2016-08-21 Thread James Bottomley 
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] [PATCH v2 2/3] bluetooth: separate HSP and HFP

2016-08-21 Thread James Bottomley 
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 2/3] bluetooth: separate HSP and HFP

2016-08-21 Thread Tanu Kaskinen 
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

2016-08-21 Thread Tanu Kaskinen 
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