On 21.09.2017 19:08, James Bottomley wrote:
> On Thu, 2017-09-21 at 19:15 +0300, Tanu Kaskinen wrote:
>> On Thu, 2017-09-21 at 17:47 +0200, Georg Chini wrote:
>>> On 21.09.2017 16:47, James Bottomley wrote:
>>>> On Thu, 2017-09-21 at 17:28 +0300, Tanu Kaskinen wrote:
>>>>> 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.
>> Yeah, sorry, I wasn't aware of that option, as I hadn't read the
>> patch. You two just seemed to be choosing between enabling or
>> disabling the native HFP AG implementation by default, and I just
>> wanted to say that I want it to be enabled by default, but without
>> breaking "headset=auto".
>>
>>> The default for the option would just be disabled in "auto" mode
>>> and enabled in "native" mode.
>> The option seems to be named "enable_profile_hfp_hf", which suggests
>> that disabling it will disable the ofono implementation of HFP AG as
>> well, and that's not what we want. Maybe rename the option to
>> "enable_native_hfp_hf"?
> I updated the current patch to rename the option and condition it on
> the backend setting (see below. Â Patch would need integrating into the
> series, since the option rename needs to go back into patch 2 but it
> gives you the idea)
>
>>> 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".
>> Ok, sounds fine, with the added note that we should set
>> "headset=native" by default.
> OK, something like this
>
> James
>
> ---
>
> diff --git a/src/modules/bluetooth/backend-native.c b/src/modules/bluetooth/backend-native.c
> index 16b2aa6c..c3a79d75 100644
> --- a/src/modules/bluetooth/backend-native.c
> +++ b/src/modules/bluetooth/backend-native.c
> @@ -822,7 +822,7 @@ pa_bluetooth_backend *pa_bluetooth_native_backend_new(pa_core *c, pa_bluetooth_d
> if (enable_hs_role)
> profile_init(backend, PA_BLUETOOTH_PROFILE_HFP_AG);
> profile_init(backend, PA_BLUETOOTH_PROFILE_HSP_HS);
> - if (pa_bluetooth_discovery_get_enable_profile_hfp_hf(y))
> + if (pa_bluetooth_discovery_get_enable_native_hfp_hf(y))
> profile_init(backend, PA_BLUETOOTH_PROFILE_HFP_HF);
>
> return backend;
> diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
> index 6c77113e..8be8a11d 100644
> --- a/src/modules/bluetooth/bluez5-util.c
> +++ b/src/modules/bluetooth/bluez5-util.c
> @@ -92,7 +92,7 @@ struct pa_bluetooth_discovery {
> int headset_backend;
> pa_bluetooth_backend *ofono_backend, *native_backend;
> PA_LLIST_HEAD(pa_dbus_pending, pending);
> - bool enable_profile_hfp_hf;
> + bool enable_native_hfp_hf;
> };
>
> static pa_dbus_pending* send_and_add_to_pending(pa_bluetooth_discovery *y, DBusMessage *m,
> @@ -173,11 +173,11 @@ static const char *transport_state_to_string(pa_bluetooth_transport_state_t stat
> }
>
> static bool device_supports_profile(pa_bluetooth_device *device, pa_bluetooth_profile_t profile) {
> - bool show_hfp, show_hsp, enable_profile_hfp_hf;
> + bool show_hfp, show_hsp, enable_native_hfp_hf;
>
> - enable_profile_hfp_hf = pa_bluetooth_discovery_get_enable_profile_hfp_hf(device->discovery);
> + enable_native_hfp_hf = pa_bluetooth_discovery_get_enable_native_hfp_hf(device->discovery);
>
> - if (enable_profile_hfp_hf) {
> + if (enable_native_hfp_hf) {
> show_hfp = pa_hashmap_get(device->uuids, PA_BLUETOOTH_UUID_HFP_HF);
> show_hsp = !show_hfp;
> } else {
> @@ -553,12 +553,12 @@ pa_bluetooth_device* pa_bluetooth_discovery_get_device_by_path(pa_bluetooth_disc
> return NULL;
> }
>
> -bool pa_bluetooth_discovery_get_enable_profile_hfp_hf(pa_bluetooth_discovery *y)
> +bool pa_bluetooth_discovery_get_enable_native_hfp_hf(pa_bluetooth_discovery *y)
> {
> pa_assert(y);
> pa_assert(PA_REFCNT_VALUE(y) > 0);
>
> - return y->enable_profile_hfp_hf;
> + return y->enable_native_hfp_hf;
> }
>
> pa_bluetooth_device* pa_bluetooth_discovery_get_device_by_address(pa_bluetooth_discovery *y, const char *remote, const char *local) {
> @@ -1754,7 +1754,7 @@ static void endpoint_done(pa_bluetooth_discovery *y, pa_bluetooth_profile_t prof
> }
> }
>
> -pa_bluetooth_discovery* pa_bluetooth_discovery_get(pa_core *c, int headset_backend, bool enable_profile_hfp_hf) {
> +pa_bluetooth_discovery* pa_bluetooth_discovery_get(pa_core *c, int headset_backend, bool enable_native_hfp_hf) {
> pa_bluetooth_discovery *y;
> DBusError err;
> DBusConnection *conn;
> @@ -1764,7 +1764,7 @@ pa_bluetooth_discovery* pa_bluetooth_discovery_get(pa_core *c, int headset_backe
> PA_REFCNT_INIT(y);
> y->core = c;
> y->headset_backend = headset_backend;
> - y->enable_profile_hfp_hf = enable_profile_hfp_hf;
> + y->enable_native_hfp_hf = enable_native_hfp_hf;
> y->adapters = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, NULL,
> (pa_free_cb_t) adapter_free);
> y->devices = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, NULL,
> diff --git a/src/modules/bluetooth/bluez5-util.h b/src/modules/bluetooth/bluez5-util.h
> index 78c4f2fa..23f9a798 100644
> --- a/src/modules/bluetooth/bluez5-util.h
> +++ b/src/modules/bluetooth/bluez5-util.h
> @@ -166,5 +166,5 @@ pa_bluetooth_discovery* pa_bluetooth_discovery_get(pa_core *core, int headset_ba
> pa_bluetooth_discovery* pa_bluetooth_discovery_ref(pa_bluetooth_discovery *y);
> void pa_bluetooth_discovery_unref(pa_bluetooth_discovery *y);
> void pa_bluetooth_discovery_set_ofono_running(pa_bluetooth_discovery *y, bool is_running);
> -bool pa_bluetooth_discovery_get_enable_profile_hfp_hf(pa_bluetooth_discovery *y);
> +bool pa_bluetooth_discovery_get_enable_native_hfp_hf(pa_bluetooth_discovery *y);
> #endif
> diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
> index 3d362f4b..d37ce9ce 100644
> --- a/src/modules/bluetooth/module-bluez5-device.c
> +++ b/src/modules/bluetooth/module-bluez5-device.c
> @@ -2010,7 +2010,7 @@ static int add_card(struct userdata *u) {
> pa_bluetooth_profile_t *p;
> const char *uuid;
> void *state;
> - bool enable_profile_hfp_hf, has_both;
> + bool enable_native_hfp_hf, has_both;
>
> pa_assert(u);
> pa_assert(u->device);
> @@ -2041,13 +2041,13 @@ static int add_card(struct userdata *u) {
>
> create_card_ports(u, data.ports);
>
> - enable_profile_hfp_hf = pa_bluetooth_discovery_get_enable_profile_hfp_hf(u->discovery);
> + enable_native_hfp_hf = pa_bluetooth_discovery_get_enable_native_hfp_hf(u->discovery);
>
> - has_both = enable_profile_hfp_hf && pa_hashmap_get(d->uuids, PA_BLUETOOTH_UUID_HFP_HF) && pa_hashmap_get(d->uuids, PA_BLUETOOTH_UUID_HSP_HS);
> + 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_profile_hfp_hf && pa_streq(uuid, PA_BLUETOOTH_UUID_HFP_HF)) {
> + 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;
> }
> diff --git a/src/modules/bluetooth/module-bluez5-discover.c b/src/modules/bluetooth/module-bluez5-discover.c
> index 52d848f0..b6e2803e 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
> @@ -104,7 +104,7 @@ int pa__init(pa_module *m) {
> const char *headset_str;
> int headset_backend;
> bool autodetect_mtu;
> - bool enable_profile_hfp_hf = true;
> + bool enable_native_hfp_hf;
>
> pa_assert(m);
>
> @@ -125,12 +125,14 @@ int pa__init(pa_module *m) {
> goto fail;
> }
>
> + enable_native_hfp_hf = (headset_backend == HEADSET_BACKEND_NATIVE);
> +
> autodetect_mtu = false;
> if (pa_modargs_get_value_boolean(ma, "autodetect_mtu", &autodetect_mtu) < 0) {
> pa_log("Invalid boolean value for autodetect_mtu parameter");
> }
> - if (pa_modargs_get_value_boolean(ma, "enable_profile_hfp_hf", &enable_profile_hfp_hf) < 0) {
> - pa_log("enable_profile_hfp_hf must be true or false");
> + if (pa_modargs_get_value_boolean(ma, "enable_native_hfp_hf", &enable_native_hfp_hf) < 0) {
> + pa_log("enable_native_hfp_hf must be true or false");
> goto fail;
> }
>
> @@ -140,7 +142,7 @@ int pa__init(pa_module *m) {
> u->autodetect_mtu = autodetect_mtu;
> u->loaded_device_paths = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
>
> - if (!(u->discovery = pa_bluetooth_discovery_get(u->core, headset_backend, enable_profile_hfp_hf)))
> + if (!(u->discovery = pa_bluetooth_discovery_get(u->core, headset_backend, enable_native_hfp_hf)))
> goto fail;
>
> u->device_connection_changed_slot =
> _______________________________________________
Yes, that looks OK to me. BTW, I did not yet look deeper into the code
but I noticed that you are calling profile_done() unconditionally. This will
break if the profile was never initialized.
