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 =
