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 =
_______________________________________________
