On Thu, 2017-09-21 at 17:28 +0300, Tanu Kaskinen wrote: > On Thu, 2017-09-21 at 08:13 +0200, Georg Chini wrote: > > > > On 21.09.2017 06:45, James Bottomley wrote: > > > > > > On Thu, 2017-09-21 at 06:27 +0200, Georg Chini wrote: > > > > > > > > On 20.09.2017 23:12, James Bottomley wrote: > > > > > > > > > > On Wed, 2017-09-20 at 20:41 +0200, Georg Chini wrote: > > > > > > > > > > > > On 20.09.2017 20:10, James Bottomley wrote: > > > > > > > > > > > > > > On Wed, 2017-09-20 at 18:11 +0200, Georg Chini wrote: > > > > > > > > > > > > > > > > On 20.09.2017 01:27, James Bottomley wrote: > > > > > > > > > > > > > > > > > > This is round 4 of the initial bluetooth: separate > > > > > > > > > HSP and HFP patch. > > > > > > > > >     It includes the review feedback and a global > > > > > > > > > on/off switch just in case there's a problem headset > > > > > > > > > with dual HFP/HSP but non-working HFP.  This one now > > > > > > > > > includes a proper rfcomm negotiation (see patch > > > > > > > > > 3).  I've finally figured out a bug in the rfcomm > > > > > > > > > negotiation that was causing issues with my LG 900 > > > > > > > > > headset, so I think it's now working for everything > > > > > > > > > (but testing would be welcome). > > > > > > > > > > > > > > > > > > James Bottomley (3): > > > > > > > > >       bluetooth: use consistent profile names > > > > > > > > >       bluetooth: separate HSP and HFP > > > > > > > > >       bluetooth: add correct HFP rfcomm negotiation > > > > > > > > > > > > > > > > > >     src/modules/bluetooth/backend- > > > > > > > > > native.c          | 168 > > > > > > > > > +++++++++++++++++++++--- > > > > > > > > >     src/modules/bluetooth/backend- > > > > > > > > > ofono.c           |   4 > > > > > > > > > +- > > > > > > > > >     src/modules/bluetooth/bluez5- > > > > > > > > > util.c             |  46 > > > > > > > > > +++++-- > > > > > > > > >     src/modules/bluetooth/bluez5- > > > > > > > > > util.h             |  10 > > > > > > > > > +- > > > > > > > > >     src/modules/bluetooth/module-bluetooth-policy.c > > > > > > > > > |   3 > > > > > > > > > +- > > > > > > > > >     src/modules/bluetooth/module-bluez5- > > > > > > > > > device.c    | 102 > > > > > > > > > ++++++++++---- > > > > > > > > >     src/modules/bluetooth/module-bluez5- > > > > > > > > > discover.c  |   6 > > > > > > > > > +- > > > > > > > > >     7 files changed, 274 insertions(+), 65 > > > > > > > > > deletions(-) > > > > > > > > > > > > > > > > > > > > > > > > > Hello James, > > > > > > > > > > > > > > > > thank you for continuing your work. Unfortunately your > > > > > > > > patch set collides with running ofono. Currently, with > > > > > > > > the default of "headest=auto", the native and the ofono > > > > > > > > backends are active in parallel. This is possible > > > > > > > > because ofono only supports HFP while PA only supports > > > > > > > > HSP. > > > > > > > > > > > > > > > > If PA starts supporting HFP headsets, this breaks above > > > > > > > > assumption and ofono and PA both try to register the > > > > > > > > corresponding HFP UUID. > > > > > > > > > > > > > > > > To work around the problem, I suggest to disable native > > > > > > > > HFP support if headset_backend == HEADSET_BACKEND_AUTO, > > > > > > > > unless configured otherwise on the command line. > > > > > > > > > > > > > > Actually, Pulseaudio already includes an ofono is running > > > > > > > check, so the enable should be set to no for backend > > > > > > > ofono or backend auto and ofono running, which would > > > > > > > enable it in the widest possible set of scenarios. > > > > > > > > > > > > > > I can cook up a patch for that, hang on. > > > > > > > > > > > > > > James > > > > > > > > > > > > This does only work for the special case when PA acts in > > > > > > the HSP headset role. In this case, no duplicate UUIDs are > > > > > > registered and disabling the profile only needs to be done > > > > > > because otherwise PA and ofono would both be listening for > > > > > > incoming SCO connections. > > > > > > > > > > It seems to me that pulse checks the dbus connection to ofono > > > > > before deciding on the backend (and therefore deciding what > > > > > to register), so as long as pulse finds ofono running, there > > > > > won't be any duplicate registrations. > > > > > > > > > > > > > > > > > The case here is different in that a duplicate UUID is > > > > > > registered. This means, by the time PA recognizes that > > > > > > ofono is running, ofono already tried to register the UUID > > > > > > and failed. That's why you have to disable HFP even if > > > > > > ofono is currently not running. > > > > > > > > > > I don't think that can be true if ofono is already running, > > > > > can it? > > > > >    Either ofono registered the HFP UUIDs way earlier or pulse > > > > > sees ofono isn't running and registers them. > > > > > > > > > > I don't think requiring the ofono daemon to be running before > > > > > pulseaudio is insurmountable.  On my openSUSE system, ofono > > > > > is started from systemd and pulse from user login, so this is > > > > > automatically satisfied.  I think where pulse is used in > > > > > system mode, you just have to tell systemd to start ofono > > > > > first, which is certainly doable. > > > > > > > > I don't think it is a good idea to rely on the order ofono and > > > > pulse are started. Also. if pulse is running and has already > > > > registered the UUID, what will happen if ofono starts? ofono > > > > has no checks for pulse and will simply try to register the > > > > UUID. I fear that this happens before pulse recognizes that > > > > ofono is running. > > > > > > This is an initialisation issue for how you want everything to > > > work.  The current distro setup is ofono from systemd and pulse > > > from login, so I can't actually see what the problem is ... > > > unless there's some reason why what the distros are doing is > > > wrong? > > > > > > If pulse is already running when you start ofono, that's caveat > > > emptor ... plus, as far as I can tell, it still all works because > > > ofono doesn't seem to get the uuid. > > > > > > The point is to get a working default configuration the distros > > > can use.  That means that we need a good way of distinguishing > > > the non-ofono case for backend = auto.  Ofono not running seems > > > to be the definitive test unless there's another you can propose? > > > > People may start/stop ofono/PA manually (or PA may crash > > and be re-spawned). The main point is that currently there is > > no restriction on the order you start things and your patch set > > should not introduce it. > > But maybe using the same approach like for the HSP headset > > role of PA works if PA sees ofono before ofono tries to register > > the UUID. I guess the best idea is to test what happens. > > > > > > > > > > > > > > > > > > > > > > The current fly in the ointment is that for a dual HFP/HSP > > > > > device, we need to fall back to the HSP profile not simply > > > > > disable the HFP one, which requires extra jiggerypokery. > > > > > > > > No, you don't need to fall back to HSP. The HFP connection will > > > > be handled by ofono in that case. > > > > > > Well, yes you do.  There's no current easy way to use HFP_HF with > > > ofono, so you want the HSP profile to be exposed if it exists, so > > > there's an easy connection to the headset. > > > > If ofono exposes HFP, you won't get your headset to connect to HSP, > > even if the device supports both, unless you could switch off HFP > > on the device side. Since PA 11.0 HFP_HF is working quite well with > > ofono, though it is clumsy to set up. And if you run ofono you > > might want headset support through ofono because ofono enables the > > HFP control features which PA cannot support. > > > > My suggestion was to disable HFP in "headset=auto" mode only if you > > don't overwrite it on the command line. If you run ofono with > > --noplugin=hfp_ag_bluez5 and enable HFP support in PA explicitly, > > you will still have HFP headset support through PA if you want it, > > only the default would be to go through ofono like it is now. I > > think you should have the choice of three options: > > > > 1) No ofono at all, everything goes through the native backend > > 2) HFP_HF support through PA, HFP_AG support through ofono > > 3) HFP_HF and HFP_AG support through ofono > > (Note about terminology: to me "HFP HF support through PA" means that > PA implements the HF role, but you seem to understand "HFP HF support > through PA" so that PA implements the AG role. In the following text > I use my definition.) Yes, that's my definition too.  The patch series only establishes a binding for HFP_HF ... HFP_AG is left exclusively for ofono. > I think we should use the native backend for the HFP AG role by > default. If the native HFP AG implementation causes a conflict with > ofono in its default configuration (which seems likely to be the > case), then "headset=auto" should not enable the native HFP AG > implementation, which then means that we should use "headset=native" > by default. > > Using "headset=native" by default means that we lose HFP HF support > in the default configuration, but I don't think that's a big loss. Setting the default to native works for me: it's basically what all distros get today anyway because they don't install ofono by default.  That probably means we don't need the elaborate switching for HSP_AG either, but it would become harmless, so could be removed later. > If we want to support the "HFP AG support through PA, HFP HF support > through ofono" case, then some new configuration option is needed, > and I think it would be clearest if that would be a separate patch > after this series. This is going to make added complexity (and added work) for users, so I prefer the default backend approach. The proposed patch is below. James --- diff --git a/src/modules/bluetooth/module-bluez5-discover.c b/src/modules/bluetooth/module-bluez5-discover.c index 52d848f0..3a62f28c 100644 --- a/src/modules/bluetooth/module-bluez5-discover.c +++ b/src/modules/bluetooth/module-bluez5-discover.c @@ -93,7 +93,7 @@ static pa_hook_result_t device_connection_changed_cb(pa_bluetooth_discovery *y, } #ifdef HAVE_BLUEZ_5_NATIVE_HEADSET -const char *default_headset_backend = "auto"; +const char *default_headset_backend = "native"; #else const char *default_headset_backend = "ofono"; #endif
