On 21.09.2017 16:47, James Bottomley wrote: > 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.) Sorry, was a bit unclear. I am referring to the role of the bluetooth device above, not the role of PA. > 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 not true, the patch supplies an option to enable/disable the HFP implementation. Simple usage of that switch would provide all the required functionality. The default for the option would just be disabled in "auto" mode and enabled in "native" mode. There is not a big code change needed, only a check if the headset mode is "auto" or "native" and changing the default accordingly. The above is the essence of what I proposed to solve the issue with headset=auto and I really don't understand why this is causing such discussions. Leaving it as is definitely breaks "headset=auto". > 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 > _______________________________________________ I think you misunderstood Tanu's reply. Changing the default to "native" is fine for me, but that still does not solve the problem that "auto" does not work any more as expected. Tanu also says "... then "headset=auto" should not enable the native HFP AG implementation ..." which basically still means you have to disable HFP for "headset=auto". And when you disable HFP for "auto" mode, it is not a big deal to not disable it if this is specified on the command line. (Or as said above just change the default to disabled with "headset=auto". If the option is specified, it will overwrite the default.) Or do you propose to remove the auto switch completely? I think that would be a bad thing because we have all the infrastructure to support it. Regarding terminology, I meanwhile prefer to add the side to which the role refers, so "PA in headset role" is the same as "bluetooth device in AG role" and vice versa. I guess that's why it is so often confused and I forgot to specify what I meant above. Regards              Georg
