On 25.09.2017 15:53, James Bottomley wrote:
> 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).
LGTM
> [...]
>>> Â 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.
I see your point, but then you should not change the name of
PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY either.
If you start distinguishing the profiles, you should do it fully.
But maybe this could be an additional patch after all?
1) rename PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT
2) separate native HFP/HSP
3) RFCOMM patch
4) Change default backend patch
5) separate PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY
I guess the original reason for using the names was that there
was no difference from the PA perspective between HFP and HSP,
so calling them "HEAD_UNIT" or "AUDIO_GATEWAY" was correct
for both.
>
> 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.
When you use the ofono backend, enable_native_hfp_hf will not
be set, so you will show HSP for the ofono backend, which is
wrong.
>
> [...]
>>> @@ -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?
Yes, ofono only supports HFP. Since enable_native_hfp_hf is not
set when you use the ofono backend, you would log that HFP
will be disabled, which is not the case.
>
>>> Â Â Â Â Â Â Â Â Â Â 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.
Changing the default has for me nothing to do with when to enable
native HFP. The main point is that without those lines your second
patch breaks the default behavior and HFP headsets don't work as
expected anymore. Therefore I would prefer this in patch 2.
We are changing the default, because that way we are able to support
HFP out-of-the-box instead of needing ofono for it. Before your patches
it was auto, because PA had no way to support HFP directly.
>
> James
