* [PATCH] adapter: Handle MGMT_SETTING_LE change
@ 2014-02-13 1:45 Petri Gynther
2014-02-13 12:41 ` Johan Hedberg
0 siblings, 1 reply; 3+ messages in thread
From: Petri Gynther @ 2014-02-13 1:45 UTC (permalink / raw)
To: linux-bluetooth
On some BLE-capable adapters, BLE is not enabled by default. When BLE is
enabled after the adapter is already powered up, we need to call
trigger_passive_scanning() so that paired BLE devices (e.g. HoG devices)
are able to reconnect back to host.
bluetoothd[1087]: src/adapter.c:adapter_start() adapter /org/bluez/hci0 has been enabled
bluetoothd[1087]: src/adapter.c:load_link_keys_complete() link keys loaded for hci0
bluetoothd[1087]: src/adapter.c:load_ltks_complete() LTKs loaded for hci0
bluetoothd[1087]: src/adapter.c:get_connections_complete() Connection count: 0
bluetoothd[1087]: src/adapter.c:new_settings_callback() Settings: 0x000000c1
bluetoothd[1087]: src/adapter.c:settings_changed() Changed settings: 0x00000040
bluetoothd[1087]: src/adapter.c:new_settings_callback() Settings: 0x000002c1
bluetoothd[1087]: src/adapter.c:settings_changed() Changed settings: 0x00000200
=> BLE got enabled, adapter already up, need to call trigger_passive_scanning()
bluetoothd[1087]: src/adapter.c:new_settings_callback() Settings: 0x000002d1
bluetoothd[1087]: src/adapter.c:settings_changed() Changed settings: 0x00000010
bluetoothd[1087]: src/adapter.c:new_settings_callback() Settings: 0x000002d3
bluetoothd[1087]: src/adapter.c:settings_changed() Changed settings: 0x00000002
---
src/adapter.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/adapter.c b/src/adapter.c
index 18601ea..01006f2 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -405,6 +405,7 @@ static void store_adapter_info(struct btd_adapter *adapter)
static void trigger_pairable_timeout(struct btd_adapter *adapter);
static void adapter_start(struct btd_adapter *adapter);
static void adapter_stop(struct btd_adapter *adapter);
+static void trigger_passive_scanning(struct btd_adapter *adapter);
static void settings_changed(struct btd_adapter *adapter, uint32_t settings)
{
@@ -434,6 +435,13 @@ static void settings_changed(struct btd_adapter *adapter, uint32_t settings)
}
}
+ if (changed_mask & MGMT_SETTING_LE) {
+ if ((adapter->current_settings & MGMT_SETTING_POWERED) &&
+ (adapter->current_settings & MGMT_SETTING_LE)) {
+ trigger_passive_scanning(adapter);
+ }
+ }
+
if (changed_mask & MGMT_SETTING_CONNECTABLE)
g_dbus_emit_property_changed(dbus_conn, adapter->path,
ADAPTER_INTERFACE, "Connectable");
--
1.9.0.rc1.175.g0b1dcb5
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] adapter: Handle MGMT_SETTING_LE change
2014-02-13 1:45 [PATCH] adapter: Handle MGMT_SETTING_LE change Petri Gynther
@ 2014-02-13 12:41 ` Johan Hedberg
2014-02-13 20:45 ` Petri Gynther
0 siblings, 1 reply; 3+ messages in thread
From: Johan Hedberg @ 2014-02-13 12:41 UTC (permalink / raw)
To: Petri Gynther; +Cc: linux-bluetooth
Hi Petri,
On Wed, Feb 12, 2014, Petri Gynther wrote:
> On some BLE-capable adapters, BLE is not enabled by default. When BLE is
> enabled after the adapter is already powered up, we need to call
> trigger_passive_scanning() so that paired BLE devices (e.g. HoG devices)
> are able to reconnect back to host.
No dual-mode adapter should have LE enabled by default and we do have
the following piece of code that gets run for any newly discovered
adapter:
if ((adapter->supported_settings & MGMT_SETTING_LE) &&
!(adapter->current_settings & MGMT_SETTING_LE))
set_mode(adapter, MGMT_OP_SET_LE, 0x01);
So the commit message is a bit confusing to me. How exactly do you end
up reproducing this scenario? I could imagine this maybe happening if
bluetoothd crashes and gets restarted when the adapter state is already
powered on. Either way you should explain this in the commit message.
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -405,6 +405,7 @@ static void store_adapter_info(struct btd_adapter *adapter)
> static void trigger_pairable_timeout(struct btd_adapter *adapter);
> static void adapter_start(struct btd_adapter *adapter);
> static void adapter_stop(struct btd_adapter *adapter);
> +static void trigger_passive_scanning(struct btd_adapter *adapter);
Since we try to avoid forward declarations like this whenever possible, did you
investigate what would be needed to not need it here. I.e. is it really
a lot of dependent functions that would need to be moved further up
together with trigger_passive_scanning() or is there even a circular
dependency somewhere that makes the forward declaration inevitable?
> + if (changed_mask & MGMT_SETTING_LE) {
> + if ((adapter->current_settings & MGMT_SETTING_POWERED) &&
> + (adapter->current_settings & MGMT_SETTING_LE)) {
> + trigger_passive_scanning(adapter);
> + }
> + }
The { } isn't needed for the internal if-statement.
Johan
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] adapter: Handle MGMT_SETTING_LE change
2014-02-13 12:41 ` Johan Hedberg
@ 2014-02-13 20:45 ` Petri Gynther
0 siblings, 0 replies; 3+ messages in thread
From: Petri Gynther @ 2014-02-13 20:45 UTC (permalink / raw)
To: Johan Hedberg, linux-bluetooth
Hi Johan,
On Thu, Feb 13, 2014 at 4:41 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Petri,
>
> On Wed, Feb 12, 2014, Petri Gynther wrote:
>> On some BLE-capable adapters, BLE is not enabled by default. When BLE is
>> enabled after the adapter is already powered up, we need to call
>> trigger_passive_scanning() so that paired BLE devices (e.g. HoG devices)
>> are able to reconnect back to host.
>
> No dual-mode adapter should have LE enabled by default and we do have
> the following piece of code that gets run for any newly discovered
> adapter:
>
> if ((adapter->supported_settings & MGMT_SETTING_LE) &&
> !(adapter->current_settings & MGMT_SETTING_LE))
> set_mode(adapter, MGMT_OP_SET_LE, 0x01);
>
> So the commit message is a bit confusing to me. How exactly do you end
> up reproducing this scenario? I could imagine this maybe happening if
> bluetoothd crashes and gets restarted when the adapter state is already
> powered on. Either way you should explain this in the commit message.
>
I have two boards with two different dual-mode adapters that exhibit
this behavior at every boot.
The init sequence is:
1. /etc/init.d/S09drivers
modprobe all BlueZ kernel drivers
2. /etc/init.d/S31bluez
hciconfig hci0 reset
hciconfig hci0 up
bdaddr -i hci0 <new BD address for adapter>
hciconfig hci0 reset
hciconfig hci0 up
...
bluetoothd -n -d 2>&1 | <log-collector> &
...
bluez-agent 2>&1 | <log-collector> &
So, bluetoothd starts with hci0 already up. With this init sequence,
bluetoothd log always shows BLE getting enabled (setting 0x200)
*after* the adapter is already powered up (setting 0x1). So, it is
necessary to call trigger_passive_scanning() at that point, since it
didn't get called at adapter_start() when BLE wasn't yet enabled.
bluetoothd[917]: src/adapter.c:adapter_register() Adapter
/org/bluez/hci0 registered
bluetoothd[917]: src/adapter.c:set_dev_class() sending set device
class command for index 0
bluetoothd[917]: src/adapter.c:set_name() sending set local name
command for index 0
bluetoothd[917]: src/adapter.c:set_mode() sending set mode command for index 0
bluetoothd[917]: src/adapter.c:set_mode() sending set mode command for index 0
bluetoothd[917]: src/adapter.c:set_mode() sending set mode command for index 0
bluetoothd[917]: src/adapter.c:set_mode() sending set mode command for index 0
bluetoothd[917]: src/adapter.c:adapter_start() adapter /org/bluez/hci0
has been enabled
bluetoothd[917]: src/adapter.c:load_link_keys_complete() link keys
loaded for hci0
bluetoothd[917]: src/adapter.c:load_ltks_complete() LTKs loaded for hci0
bluetoothd[917]: src/adapter.c:get_connections_complete() Connection count: 0
bluetoothd[917]: src/adapter.c:local_name_changed_callback() Name: system-name
bluetoothd[917]: src/adapter.c:local_name_changed_callback() Short name:
bluetoothd[917]: src/adapter.c:local_name_changed_callback() Current
alias: system-name
bluetoothd[917]: src/attrib-server.c:attrib_db_update() handle=0x0006
bluetoothd[917]: src/adapter.c:new_settings_callback() Settings: 0x000000c1
bluetoothd[917]: src/adapter.c:settings_changed() Changed settings: 0x00000040
bluetoothd[917]: src/adapter.c:new_settings_callback() Settings:
0x000002c1 <=== MGMT_SETTING_LE=1
bluetoothd[917]: src/adapter.c:settings_changed() Changed settings: 0x00000200
===> need to call trigger_passive_scanning() here
bluetoothd[917]: src/adapter.c:new_settings_callback() Settings: 0x000002d1
bluetoothd[917]: src/adapter.c:settings_changed() Changed settings: 0x00000010
bluetoothd[917]: src/adapter.c:new_settings_callback() Settings: 0x000002d3
bluetoothd[917]: src/adapter.c:settings_changed() Changed settings: 0x00000002
>> --- a/src/adapter.c
>> +++ b/src/adapter.c
>> @@ -405,6 +405,7 @@ static void store_adapter_info(struct btd_adapter *adapter)
>> static void trigger_pairable_timeout(struct btd_adapter *adapter);
>> static void adapter_start(struct btd_adapter *adapter);
>> static void adapter_stop(struct btd_adapter *adapter);
>> +static void trigger_passive_scanning(struct btd_adapter *adapter);
>
> Since we try to avoid forward declarations like this whenever possible, did you
> investigate what would be needed to not need it here. I.e. is it really
> a lot of dependent functions that would need to be moved further up
> together with trigger_passive_scanning() or is there even a circular
> dependency somewhere that makes the forward declaration inevitable?
>
I'll investigate this.
>> + if (changed_mask & MGMT_SETTING_LE) {
>> + if ((adapter->current_settings & MGMT_SETTING_POWERED) &&
>> + (adapter->current_settings & MGMT_SETTING_LE)) {
>> + trigger_passive_scanning(adapter);
>> + }
>> + }
>
> The { } isn't needed for the internal if-statement.
I'll fix this.
>
> Johan
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-02-13 20:45 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-13 1:45 [PATCH] adapter: Handle MGMT_SETTING_LE change Petri Gynther
2014-02-13 12:41 ` Johan Hedberg
2014-02-13 20:45 ` Petri Gynther
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.