Not turning BT off in time due to actions queued in mgmt makes Android
unstable and locks Bluetooth UI controlls. This patch fixes this issue
by cancelling queued actions.
---
android/bluetooth.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/android/bluetooth.c b/android/bluetooth.c
index 6174b1f..e67864a 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -1982,6 +1982,9 @@ static void pair_device_complete(uint8_t status, uint16_t length,
DBG("status %u", status);
+ /*Data used for bond cancelling can be freed now*/
+ g_free(user_data);
+
/* On success bond state change will be send when new link key event
* is received */
if (status == MGMT_STATUS_SUCCESS)
@@ -1991,19 +1994,34 @@ static void pair_device_complete(uint8_t status, uint16_t length,
HAL_BOND_STATE_NONE);
}
+static void pair_device_cancelled(void *data)
+{
+ bdaddr_t *addr = data;
+
+ set_device_bond_state(addr, HAL_STATUS_FAILED, HAL_BOND_STATE_NONE);
+
+ g_free(data);
+}
+
static void handle_create_bond_cmd(const void *buf, uint16_t len)
{
const struct hal_cmd_create_bond *cmd = buf;
uint8_t status;
struct mgmt_cp_pair_device cp;
+ bdaddr_t *addr;
cp.io_cap = DEFAULT_IO_CAPABILITY;
cp.addr.type = BDADDR_BREDR;
android2bdaddr(cmd->bdaddr, &cp.addr.bdaddr);
+ addr = g_new(bdaddr_t, 1);
+ bacpy(addr, &cp.addr.bdaddr);
+
if (mgmt_send(mgmt_if, MGMT_OP_PAIR_DEVICE, adapter.index, sizeof(cp),
- &cp, pair_device_complete, NULL, NULL) == 0) {
+ &cp, pair_device_complete,
+ addr, pair_device_cancelled) == 0) {
status = HAL_STATUS_FAILED;
+ g_free(addr);
goto fail;
}
@@ -2253,6 +2271,8 @@ static void handle_disable_cmd(const void *buf, uint16_t len)
goto failed;
}
+ mgmt_cancel_index(mgmt_if, adapter.index);
+
if (!set_mode(MGMT_OP_SET_POWERED, 0x00)) {
status = HAL_STATUS_FAILED;
goto failed;
--
1.8.5
Hi Jakub,
On Fri, Dec 6, 2013 at 5:30 PM, Jakub Tyszkowski
<[email protected]> wrote:
> We shouldn't care about pending actions being completed as we want to
> turn BT off. This fixes Android UI controlls being locked when BT off
> action waits in queue for to long.
>
> ---
> android/bluetooth.c | 28 ++++++++++++++++++++--------
> 1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/android/bluetooth.c b/android/bluetooth.c
> index 6174b1f..e5d8b7a 100644
> --- a/android/bluetooth.c
> +++ b/android/bluetooth.c
> @@ -1290,17 +1290,24 @@ static void set_mode_complete(uint8_t status, uint16_t length,
> new_settings_callback(adapter.index, length, param, NULL);
> }
>
> -static bool set_mode(uint16_t opcode, uint8_t mode)
> +static bool set_mode(uint16_t opcode, uint8_t mode, bool unqueued)
> {
> struct mgmt_mode cp;
> + unsigned int mgmt_result;
>
> memset(&cp, 0, sizeof(cp));
> cp.val = mode;
>
> DBG("opcode=0x%x mode=0x%x", opcode, mode);
>
> - if (mgmt_send(mgmt_if, opcode, adapter.index, sizeof(cp), &cp,
> - set_mode_complete, NULL, NULL) > 0)
> + if (unqueued)
> + mgmt_result = mgmt_reply(mgmt_if, opcode, adapter.index,
> + sizeof(cp), &cp, set_mode_complete, NULL, NULL);
> + else
> + mgmt_result = mgmt_send(mgmt_if, opcode, adapter.index,
> + sizeof(cp), &cp, set_mode_complete, NULL, NULL);
> +
> + if (mgmt_result > 0)
> return true;
>
> error("Failed to set mode");
> @@ -1462,10 +1469,10 @@ static void read_info_complete(uint8_t status, uint16_t length,
> missing_settings = adapter.current_settings ^ supported_settings;
>
> if (missing_settings & MGMT_SETTING_SSP)
> - set_mode(MGMT_OP_SET_SSP, 0x01);
> + set_mode(MGMT_OP_SET_SSP, 0x01, false);
>
> if (missing_settings & MGMT_SETTING_PAIRABLE)
> - set_mode(MGMT_OP_SET_PAIRABLE, 0x01);
> + set_mode(MGMT_OP_SET_PAIRABLE, 0x01, false);
>
> return;
>
> @@ -1926,7 +1933,8 @@ static uint8_t set_scan_mode(const void *buf, uint16_t len)
> }
>
> if (cur_conn != conn) {
> - if (!set_mode(MGMT_OP_SET_CONNECTABLE, conn ? 0x01 : 0x00))
> + if (!set_mode(MGMT_OP_SET_CONNECTABLE, conn ? 0x01 : 0x00,
> + false))
> return HAL_STATUS_FAILED;
> }
>
> @@ -2234,7 +2242,7 @@ static void handle_enable_cmd(const void *buf, uint16_t len)
> goto failed;
> }
>
> - if (!set_mode(MGMT_OP_SET_POWERED, 0x01)) {
> + if (!set_mode(MGMT_OP_SET_POWERED, 0x01, false)) {
> status = HAL_STATUS_FAILED;
> goto failed;
> }
> @@ -2253,7 +2261,11 @@ static void handle_disable_cmd(const void *buf, uint16_t len)
> goto failed;
> }
>
> - if (!set_mode(MGMT_OP_SET_POWERED, 0x00)) {
> + /*
> + * Prioritize BT turn off as not doing it in time makes Android
> + * unstable and locks Bluetooth UI controlls.
> + */
> + if (!set_mode(MGMT_OP_SET_POWERED, 0x00, true)) {
> status = HAL_STATUS_FAILED;
> goto failed;
> }
> --
> 1.8.5
Ive applied all the patches from this set except this one, it seems
you are misusing mgmt_reply is for actual replies and this one seems
to be a request. We should probably store pending handles and cancel
them properly if is the case here.
--
Luiz Augusto von Dentz
We shouldn't care about pending actions being completed as we want to
turn BT off. This fixes Android UI controlls being locked when BT off
action waits in queue for to long.
---
android/bluetooth.c | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/android/bluetooth.c b/android/bluetooth.c
index 6174b1f..e5d8b7a 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -1290,17 +1290,24 @@ static void set_mode_complete(uint8_t status, uint16_t length,
new_settings_callback(adapter.index, length, param, NULL);
}
-static bool set_mode(uint16_t opcode, uint8_t mode)
+static bool set_mode(uint16_t opcode, uint8_t mode, bool unqueued)
{
struct mgmt_mode cp;
+ unsigned int mgmt_result;
memset(&cp, 0, sizeof(cp));
cp.val = mode;
DBG("opcode=0x%x mode=0x%x", opcode, mode);
- if (mgmt_send(mgmt_if, opcode, adapter.index, sizeof(cp), &cp,
- set_mode_complete, NULL, NULL) > 0)
+ if (unqueued)
+ mgmt_result = mgmt_reply(mgmt_if, opcode, adapter.index,
+ sizeof(cp), &cp, set_mode_complete, NULL, NULL);
+ else
+ mgmt_result = mgmt_send(mgmt_if, opcode, adapter.index,
+ sizeof(cp), &cp, set_mode_complete, NULL, NULL);
+
+ if (mgmt_result > 0)
return true;
error("Failed to set mode");
@@ -1462,10 +1469,10 @@ static void read_info_complete(uint8_t status, uint16_t length,
missing_settings = adapter.current_settings ^ supported_settings;
if (missing_settings & MGMT_SETTING_SSP)
- set_mode(MGMT_OP_SET_SSP, 0x01);
+ set_mode(MGMT_OP_SET_SSP, 0x01, false);
if (missing_settings & MGMT_SETTING_PAIRABLE)
- set_mode(MGMT_OP_SET_PAIRABLE, 0x01);
+ set_mode(MGMT_OP_SET_PAIRABLE, 0x01, false);
return;
@@ -1926,7 +1933,8 @@ static uint8_t set_scan_mode(const void *buf, uint16_t len)
}
if (cur_conn != conn) {
- if (!set_mode(MGMT_OP_SET_CONNECTABLE, conn ? 0x01 : 0x00))
+ if (!set_mode(MGMT_OP_SET_CONNECTABLE, conn ? 0x01 : 0x00,
+ false))
return HAL_STATUS_FAILED;
}
@@ -2234,7 +2242,7 @@ static void handle_enable_cmd(const void *buf, uint16_t len)
goto failed;
}
- if (!set_mode(MGMT_OP_SET_POWERED, 0x01)) {
+ if (!set_mode(MGMT_OP_SET_POWERED, 0x01, false)) {
status = HAL_STATUS_FAILED;
goto failed;
}
@@ -2253,7 +2261,11 @@ static void handle_disable_cmd(const void *buf, uint16_t len)
goto failed;
}
- if (!set_mode(MGMT_OP_SET_POWERED, 0x00)) {
+ /*
+ * Prioritize BT turn off as not doing it in time makes Android
+ * unstable and locks Bluetooth UI controlls.
+ */
+ if (!set_mode(MGMT_OP_SET_POWERED, 0x00, true)) {
status = HAL_STATUS_FAILED;
goto failed;
}
--
1.8.5
On 12/05/2013 11:37 AM, Jakub Tyszkowski wrote:
> Not turning BT off in time due to actions queued in mgmt makes Android
> unstable and locks Bluetooth UI controlls. This patch fixes this issue
> by cancelling queued actions.
>
> ---
> android/bluetooth.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/android/bluetooth.c b/android/bluetooth.c
> index 6174b1f..e67864a 100644
> --- a/android/bluetooth.c
> +++ b/android/bluetooth.c
> @@ -1982,6 +1982,9 @@ static void pair_device_complete(uint8_t status, uint16_t length,
>
> DBG("status %u", status);
>
> + /*Data used for bond cancelling can be freed now*/
> + g_free(user_data);
> +
> /* On success bond state change will be send when new link key event
> * is received */
> if (status == MGMT_STATUS_SUCCESS)
> @@ -1991,19 +1994,34 @@ static void pair_device_complete(uint8_t status, uint16_t length,
> HAL_BOND_STATE_NONE);
> }
>
> +static void pair_device_cancelled(void *data)
> +{
> + bdaddr_t *addr = data;
> +
> + set_device_bond_state(addr, HAL_STATUS_FAILED, HAL_BOND_STATE_NONE);
> +
> + g_free(data);
> +}
> +
> static void handle_create_bond_cmd(const void *buf, uint16_t len)
> {
> const struct hal_cmd_create_bond *cmd = buf;
> uint8_t status;
> struct mgmt_cp_pair_device cp;
> + bdaddr_t *addr;
>
> cp.io_cap = DEFAULT_IO_CAPABILITY;
> cp.addr.type = BDADDR_BREDR;
> android2bdaddr(cmd->bdaddr, &cp.addr.bdaddr);
>
> + addr = g_new(bdaddr_t, 1);
> + bacpy(addr, &cp.addr.bdaddr);
> +
> if (mgmt_send(mgmt_if, MGMT_OP_PAIR_DEVICE, adapter.index, sizeof(cp),
> - &cp, pair_device_complete, NULL, NULL) == 0) {
> + &cp, pair_device_complete,
> + addr, pair_device_cancelled) == 0) {
> status = HAL_STATUS_FAILED;
> + g_free(addr);
> goto fail;
> }
>
> @@ -2253,6 +2271,8 @@ static void handle_disable_cmd(const void *buf, uint16_t len)
> goto failed;
> }
>
> + mgmt_cancel_index(mgmt_if, adapter.index);
> +
> if (!set_mode(MGMT_OP_SET_POWERED, 0x00)) {
> status = HAL_STATUS_FAILED;
> goto failed;
>
Hi,
Please ignore this one patch as it makes user_data being freed twice.
BR,
Jakub Tyszkowski
HAL should contain as little logic as possible, but we should be doing
these checks on daemon side anyway.
---
android/hal-pan.c | 14 --------------
android/pan.c | 19 +++++++++++++++++++
2 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/android/hal-pan.c b/android/hal-pan.c
index ec52672..8c0f8d8 100644
--- a/android/hal-pan.c
+++ b/android/hal-pan.c
@@ -109,20 +109,6 @@ static bt_status_t pan_connect(const bt_bdaddr_t *bd_addr, int local_role,
if (!interface_ready())
return BT_STATUS_NOT_READY;
- switch (local_role) {
- case BTPAN_ROLE_PANNAP:
- if (remote_role != BTPAN_ROLE_PANU)
- return BT_STATUS_UNSUPPORTED;
- break;
- case BTPAN_ROLE_PANU:
- if (remote_role != BTPAN_ROLE_PANNAP &&
- remote_role != BTPAN_ROLE_PANU)
- return BT_STATUS_UNSUPPORTED;
- break;
- default:
- return BT_STATUS_UNSUPPORTED;
- }
-
memcpy(cmd.bdaddr, bd_addr, sizeof(cmd.bdaddr));
cmd.local_role = local_role;
cmd.remote_role = remote_role;
diff --git a/android/pan.c b/android/pan.c
index f6e0ca9..78a1055 100644
--- a/android/pan.c
+++ b/android/pan.c
@@ -200,6 +200,25 @@ static void bt_pan_connect(const void *buf, uint16_t len)
DBG("");
+ switch (cmd->local_role) {
+ case HAL_PAN_ROLE_NAP:
+ if (cmd->remote_role != HAL_PAN_ROLE_PANU) {
+ status = HAL_STATUS_UNSUPPORTED;
+ goto failed;
+ }
+ break;
+ case HAL_PAN_ROLE_PANU:
+ if (cmd->remote_role != HAL_PAN_ROLE_NAP &&
+ cmd->remote_role != HAL_PAN_ROLE_PANU) {
+ status = HAL_STATUS_UNSUPPORTED;
+ goto failed;
+ }
+ break;
+ default:
+ status = HAL_STATUS_UNSUPPORTED;
+ goto failed;
+ }
+
android2bdaddr(&cmd->bdaddr, &dst);
l = g_slist_find_custom(devices, &dst, device_cmp);
--
1.8.5
HAL should contain as little logic as possible, but we should be doing
these checks on daemon side anyway.
---
android/hal-pan.c | 3 ---
android/pan.c | 16 ++++++++++++++--
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/android/hal-pan.c b/android/hal-pan.c
index 6aaf8af..ec52672 100644
--- a/android/hal-pan.c
+++ b/android/hal-pan.c
@@ -74,9 +74,6 @@ static bt_status_t pan_enable(int local_role)
if (!interface_ready())
return BT_STATUS_NOT_READY;
- if (!(local_role == BTPAN_ROLE_PANU || local_role == BTPAN_ROLE_PANNAP))
- return BT_STATUS_UNSUPPORTED;
-
cmd.local_role = local_role;
return hal_ipc_cmd(HAL_SERVICE_ID_PAN, HAL_OP_PAN_ENABLE,
diff --git a/android/pan.c b/android/pan.c
index fe6ee26..f6e0ca9 100644
--- a/android/pan.c
+++ b/android/pan.c
@@ -280,9 +280,21 @@ failed:
static void bt_pan_enable(const void *buf, uint16_t len)
{
- DBG("Not Implemented");
+ const struct hal_cmd_pan_enable *cmd = buf;
+ uint8_t status;
+
+ switch (cmd->local_role) {
+ case HAL_PAN_ROLE_PANU:
+ case HAL_PAN_ROLE_NAP:
+ DBG("Not Implemented");
+ status = HAL_STATUS_FAILED;
+ break;
+ default:
+ status = HAL_STATUS_UNSUPPORTED;
+ break;
+ }
- ipc_send_rsp(HAL_SERVICE_ID_PAN, HAL_OP_PAN_ENABLE, HAL_STATUS_FAILED);
+ ipc_send_rsp(HAL_SERVICE_ID_PAN, HAL_OP_PAN_ENABLE, status);
}
static void bt_pan_get_role(const void *buf, uint16_t len)
--
1.8.5
Add ipc handlers cleanup if init fails. Send proper status if
already initialized.
---
android/hal-a2dp.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/android/hal-a2dp.c b/android/hal-a2dp.c
index cf39ba2..c898995 100644
--- a/android/hal-a2dp.c
+++ b/android/hal-a2dp.c
@@ -96,9 +96,13 @@ static bt_status_t disconnect(bt_bdaddr_t *bd_addr)
static bt_status_t init(btav_callbacks_t *callbacks)
{
struct hal_cmd_register_module cmd;
+ int ret;
DBG("");
+ if (interface_ready())
+ return BT_STATUS_DONE;
+
cbs = callbacks;
hal_ipc_register(HAL_SERVICE_ID_A2DP, ev_handlers,
@@ -106,8 +110,15 @@ static bt_status_t init(btav_callbacks_t *callbacks)
cmd.service_id = HAL_SERVICE_ID_A2DP;
- return hal_ipc_cmd(HAL_SERVICE_ID_CORE, HAL_OP_REGISTER_MODULE,
+ ret = hal_ipc_cmd(HAL_SERVICE_ID_CORE, HAL_OP_REGISTER_MODULE,
sizeof(cmd), &cmd, 0, NULL, NULL);
+
+ if (ret != BT_STATUS_SUCCESS) {
+ cbs = NULL;
+ hal_ipc_unregister(HAL_SERVICE_ID_A2DP);
+ }
+
+ return ret;
}
static void cleanup()
--
1.8.5
Add ipc handlers cleanup if init fails. Send proper status if
already initialized.
---
android/hal-hidhost.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/android/hal-hidhost.c b/android/hal-hidhost.c
index 0573006..6a6b682 100644
--- a/android/hal-hidhost.c
+++ b/android/hal-hidhost.c
@@ -363,9 +363,13 @@ static bt_status_t send_data(bt_bdaddr_t *bd_addr, char *data)
static bt_status_t init(bthh_callbacks_t *callbacks)
{
struct hal_cmd_register_module cmd;
+ int ret;
DBG("");
+ if (interface_ready())
+ return BT_STATUS_DONE;
+
/* store reference to user callbacks */
cbacks = callbacks;
@@ -374,8 +378,15 @@ static bt_status_t init(bthh_callbacks_t *callbacks)
cmd.service_id = HAL_SERVICE_ID_HIDHOST;
- return hal_ipc_cmd(HAL_SERVICE_ID_CORE, HAL_OP_REGISTER_MODULE,
+ ret = hal_ipc_cmd(HAL_SERVICE_ID_CORE, HAL_OP_REGISTER_MODULE,
sizeof(cmd), &cmd, 0, NULL, NULL);
+
+ if (ret != BT_STATUS_SUCCESS) {
+ cbacks = NULL;
+ hal_ipc_unregister(HAL_SERVICE_ID_HIDHOST);
+ }
+
+ return ret;
}
static void cleanup(void)
--
1.8.5
Add ipc handlers cleanup if init fails. Send proper status if
already initialized.
---
android/hal-pan.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/android/hal-pan.c b/android/hal-pan.c
index e7b8a20..6aaf8af 100644
--- a/android/hal-pan.c
+++ b/android/hal-pan.c
@@ -152,9 +152,13 @@ static bt_status_t pan_disconnect(const bt_bdaddr_t *bd_addr)
static bt_status_t pan_init(const btpan_callbacks_t *callbacks)
{
struct hal_cmd_register_module cmd;
+ int ret;
DBG("");
+ if (interface_ready())
+ return BT_STATUS_DONE;
+
cbs = callbacks;
hal_ipc_register(HAL_SERVICE_ID_PAN, ev_handlers,
@@ -162,8 +166,15 @@ static bt_status_t pan_init(const btpan_callbacks_t *callbacks)
cmd.service_id = HAL_SERVICE_ID_PAN;
- return hal_ipc_cmd(HAL_SERVICE_ID_CORE, HAL_OP_REGISTER_MODULE,
+ ret = hal_ipc_cmd(HAL_SERVICE_ID_CORE, HAL_OP_REGISTER_MODULE,
sizeof(cmd), &cmd, 0, NULL, NULL);
+
+ if (ret != BT_STATUS_SUCCESS) {
+ cbs = NULL;
+ hal_ipc_unregister(HAL_SERVICE_ID_PAN);
+ }
+
+ return ret;
}
static void pan_cleanup()
--
1.8.5
We should be sending BT_STATUS_DONE when calling init on already
initialized interface like Bluedroid does. This indicates that previosly
registered callbacks are still registered, not those passed with second
init call.
---
android/hal-bluetooth.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/android/hal-bluetooth.c b/android/hal-bluetooth.c
index 87d6fc7..7cac15c 100644
--- a/android/hal-bluetooth.c
+++ b/android/hal-bluetooth.c
@@ -373,7 +373,7 @@ static int init(bt_callbacks_t *callbacks)
DBG("");
if (interface_ready())
- return BT_STATUS_SUCCESS;
+ return BT_STATUS_DONE;
bt_hal_cbacks = callbacks;
--
1.8.5
Update informations about 'adapter' interface being renamed to 'bluetooth' and
init being called on haltest startup by default.
---
android/README | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/android/README b/android/README
index 6c2c53f..68c3e9f 100644
--- a/android/README
+++ b/android/README
@@ -82,9 +82,12 @@ Testing tool
============
BT HAL test tools located in android/haltest is provided for HAL level testing
-of both Android daemon and HAL library. Start it and type 'adapter init' in
-prompt to initialize HAL library. On Android required bluetoothd service will
-be started automatically. On Linux it is required to start android/bluetoothd
-manually before init command timeout. To deinitialize HAL library and stop
-daemon type 'adapter cleanup'. Type 'help' for more information. Tab completion
-is also supported.
+of both Android daemon and HAL library. Start it with '-n' parameter and type
+'bluetooth init' in prompt to initialize HAL library. Running without parameter
+will make haltest try to initialize all services after start. On Android
+required bluetoothd service will be started automatically. On Linux it is
+required to start android/bluetoothd manually before init command timeout or
+use provided android/system-emulator, which takes care of launching daemon
+automatically on HAL library initialization. To deinitialize HAL library and
+stop daemon type 'bluetooth cleanup'. Type 'help' for more information. Tab
+completion is also supported.
--
1.8.5