[pulseaudio-discuss] [PATCH v5 2/4] bluetooth: separate HSP and HFP

[James Bottomley]

On Sun, 2017-09-24 at 15:35 +0200, Georg Chini wrote:
> On 21.09.2017 21:27, 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).
> 
> This paragraph is a bit misleading because you can't select
> the profiles separately. If HFP is available, it will be used.

OK, so how about

For headsets that support both HSP and HFP, HFP becomes the default
selectable profile unless enable_native_hfp_hf is set to no.  This will
only matter once we start adding features to HFP that HSP can't support
(like wideband audio).

[...]  
> >   static bool device_supports_profile(pa_bluetooth_device *device,
> > pa_bluetooth_profile_t profile) {
> > +    bool show_hfp, show_hsp, enable_native_hfp_hf;
> > +
> > +    enable_native_hfp_hf =
> > pa_bluetooth_discovery_get_enable_native_hfp_hf(device->discovery);
> > +
> > +    if (enable_native_hfp_hf) {
> > +        show_hfp = pa_hashmap_get(device->uuids,
> > PA_BLUETOOTH_UUID_HFP_HF);
> > +        show_hsp = !show_hfp;
> > +    } else {
> > +        show_hfp = false;
> > +        show_hsp = true;
> > +    }
> 
> You have to consider the case that the ofono backend is used. In that
> case show_hfp=true; show_hsp=false;
> What about the AG device roles? If we start distinguishing between
> HFP and HSP, I think we should do it everywhere. If "headset=native",
> we only support HSP_AG devices, while with "auto" and "ofono" we
> support only HFP_AG devices.

The patch only affects HFP_HF and HSP_HS it doesn't affect the HFP_AG
and HSP_AG showings.  Since the patches only really affect the headset
role, I don't think they should add any changes to the AG role.

Right at the moment for the headset role, the ofono backend only wants
one thing show here and it doesn't care what (because it gets the audio
stream from ofono via a dbus socket), however it does strike me that
what we now show is more technically accurate even in the ofono case
because if HFP_HF is supported it will be used.

[...]
> > @@ -2010,9 +2041,22 @@ static int add_card(struct userdata *u) {
> >   
> >       create_card_ports(u, data.ports);
> >   
> > +    enable_native_hfp_hf =
> > pa_bluetooth_discovery_get_enable_native_hfp_hf(u->discovery);
> > +
> > +    has_both = enable_native_hfp_hf && pa_hashmap_get(d->uuids,
> > PA_BLUETOOTH_UUID_HFP_HF) && pa_hashmap_get(d->uuids,
> > PA_BLUETOOTH_UUID_HSP_HS);
> >       PA_HASHMAP_FOREACH(uuid, d->uuids, state) {
> >           pa_bluetooth_profile_t profile;
> >   
> > +        if (!enable_native_hfp_hf && pa_streq(uuid,
> > PA_BLUETOOTH_UUID_HFP_HF)) {
> > +            pa_log_info("device supports HFP but disabling profile
> > as requested");
> > +            continue;
> > +        }
> > +
> > +        if (has_both && pa_streq(uuid, PA_BLUETOOTH_UUID_HSP_HS))
> > {
> > +            pa_log_info("device support HSP and HFP, selecting HFP
> > only");
> > +            continue;
> > +        }
> > +
> 
> You need to skip this (or maybe log another message) if the
> ofono backend is used and enable_native_hfp_hf is not set.

But isn't it accurate for ofono as well?  If you use the ofono backend
and you have HFP_HF support it will be selected?

> > 
> >           if (uuid_to_profile(uuid, &profile) < 0)
> >               continue;
> >   
> > diff --git a/src/modules/bluetooth/module-bluez5-discover.c
> > b/src/modules/bluetooth/module-bluez5-discover.c
> > index c535ead4..bfb361ae 100644
> > --- a/src/modules/bluetooth/module-bluez5-discover.c
> > +++ b/src/modules/bluetooth/module-bluez5-discover.c
> > @@ -104,6 +104,7 @@ int pa__init(pa_module *m) {
> >       const char *headset_str;
> >       int headset_backend;
> >       bool autodetect_mtu;
> > +    bool enable_native_hfp_hf = true;
> >   
> >       pa_assert(m);
> >   
> > @@ -127,6 +128,9 @@ int pa__init(pa_module *m) {
> >       autodetect_mtu = false;
> >       if (pa_modargs_get_value_boolean(ma, "autodetect_mtu",
> > &autodetect_mtu) < 0) {
> >           pa_log("Invalid boolean value for autodetect_mtu
> > parameter");
> > +    }
> 
> The two following lines from your patch 4 should go in here:
> 
> + /* default value if no module parameter */
> + enable_native_hfp_hf = (headset_backend == HEADSET_BACKEND_NATIVE);
> 
> Otherwise the patch will break the "headset=auto" case.
> (Please keep changing the default backend to native as a separate
> patch, even if it is reduced to a one-line change)

I honestly don't care that much, so I'll do what the maintaners want.
 From the logical patch development point of view, though, this line to
me seems to belong to the fourth patch, because that's where the
resolution of the native vs auto stuff is done.

James