V4:
Added device name caching in device list
Changes according to Johan comments
V3:
Removed adapter bonding state
Added device list in order to track bonding state per device.
Fix for incoming legacy paring. We act here same as Bluedroid.
V2:
Bonding state added.
V1:
With those patches it is possible to bond from remote device and in addition
you will see the name of remote device on Android pairing request pop up.
Lukasz Rymanowski (4):
android: Update bond state on incoming bonding
android: Cache device name on device list.
android: Update bond state on auth and connect failed
android: Change TODO with explaining comment
android/bluetooth.c | 229 +++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 189 insertions(+), 40 deletions(-)
--
1.8.4
Hi Lukasz,
On Thu, Nov 14, 2013, Lukasz Rymanowski wrote:
> +static void cache_device_name(const bdaddr_t *addr, char *name)
Maybe const char *name here?
> + struct device *dev = NULL;
> + GSList *l;
> +
> + l = g_slist_find_custom(devices, addr, bdaddr_cmp);
> + if (l)
> + dev = l->data;
> +
> + if (!dev) {
> + dev = g_new0(struct device, 1);
> + bacpy(&dev->bdaddr, addr);
> + dev->bond_state = HAL_BOND_STATE_NONE;
> + dev->name = g_malloc0(strlen(name) + 1);
> + memcpy(dev->name, name, strlen(name));
Why this complicated malloc + memcpy when further below you do a much
simpler g_strdup?
In fact I'd just skip the name part here completely, remove the return
statement and let the code continue to the single place where you free
and set the name. Both g_strcmp0 and g_free should do the right thing if
the name is NULL.
> + if (!g_strcmp0(dev->name, name))
> + return;
Minor coding style issue here with indentation.
> +static char* get_device_name(const bdaddr_t *addr)
> +{
Coding style above with spacing, and this should be static const char *
> + GSList *l;
> + struct device *dev;
> +
> + l = g_slist_find_custom(devices, addr, bdaddr_cmp);
> + if (l) {
> + dev = l->data;
You can move the variable declaration here to the more reduced scope.
> +static void send_remote_device_name_prop(const bdaddr_t *bdaddr)
> {
> struct hal_ev_remote_device_props *ev;
> - uint8_t buf[BASELEN_REMOTE_DEV_PROP + strlen(name)];
> + uint8_t *buf;
> + char dst[18];
> + char *name;
const char *name;
> +
> + ba2str(bdaddr, dst);
> +
> + /* Use cached name or bdaddr string */
> + name = get_device_name(bdaddr);
> + if (!name)
> + name = dst;
> +
> + buf = g_malloc0(BASELEN_REMOTE_DEV_PROP + strlen(name));
>
> ev = (void *) buf;
The value of having a separate buf variable is completely lost here now
that you start doing dynamic allocation. What I'd do is remove buf, add
a new size_t ev_len, evaluate ev_len and then do ev = g_malloc0(ev_len);
> memset(buf, 0, sizeof(buf));
This one is both unnecessary and broken. Firstly you already zeroed out
the buffer with g_malloc and secondly sizeof(buf) doesn't do what you'd
want it to do (now that this is pointer instead of an array).
Johan
Hi Lukasz,
On Thu, Nov 14, 2013, Lukasz Rymanowski wrote:
> +static void send_remote_device_name_prop(const bdaddr_t *bdaddr, char *name)
> +{
> + struct hal_ev_remote_device_props *ev;
> + uint8_t buf[BASELEN_REMOTE_DEV_PROP + strlen(name)];
> +
> + ev = (void *) buf;
Minor white space issue here (two spaces instead of one after =.
> @@ -1865,12 +1933,18 @@ static bool cancel_bond(void *buf, uint16_t len)
> {
> struct hal_cmd_cancel_bond *cmd = buf;
> struct mgmt_addr_info cp;
> + bool result;
>
> cp.type = BDADDR_BREDR;
> android2bdaddr(cmd->bdaddr, &cp.bdaddr);
>
> - return mgmt_reply(mgmt_if, MGMT_OP_CANCEL_PAIR_DEVICE, adapter.index,
> + result = mgmt_reply(mgmt_if, MGMT_OP_CANCEL_PAIR_DEVICE, adapter.index,
> sizeof(cp), &cp, NULL, NULL, NULL) > 0;
> + if (result)
> + set_device_bond_state(&cp.bdaddr, HAL_STATUS_SUCCESS,
> + HAL_BOND_STATE_NONE);
Would it not make sense to use the mgmt command complete callback for
sending HAL_BOND_STATE_NONE? (right now you're just passing NULL for
it). Actually, sending this command will generate a command complete to
our pair_device command with a "canceled" status. So aren't we sending
redundant HAL_BOND_STATE_NONE here?
Johan
---
android/bluetooth.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/android/bluetooth.c b/android/bluetooth.c
index e75baef..2e94e1f 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -654,7 +654,10 @@ static void send_ssp_request(const bdaddr_t *addr, uint8_t variant,
{
struct hal_ev_ssp_request ev;
- /* TODO name and CoD of remote devices should probably be cached */
+ /* It is ok to have empty name and CoD of remote devices here since
+ * those information has been already provided on device_connected event
+ * or during device scaning. Android will use that instead.
+ */
memset(&ev, 0, sizeof(ev));
bdaddr2android(addr, ev.bdaddr);
ev.pairing_variant = variant;
--
1.8.4
---
android/bluetooth.c | 57 ++++++++++++++++++++++++++++++++---------------------
1 file changed, 35 insertions(+), 22 deletions(-)
diff --git a/android/bluetooth.c b/android/bluetooth.c
index ce153c2..e75baef 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -996,16 +996,51 @@ static void mgmt_device_disconnected_event(uint16_t index, uint16_t length,
HAL_EV_ACL_STATE_CHANGED, sizeof(hal_ev), &hal_ev, -1);
}
+static uint8_t status_mgmt2hal(uint8_t mgmt)
+{
+ switch (mgmt) {
+ case MGMT_STATUS_SUCCESS:
+ return HAL_STATUS_SUCCESS;
+ case MGMT_STATUS_NO_RESOURCES:
+ return HAL_STATUS_NOMEM;
+ case MGMT_STATUS_BUSY:
+ return HAL_STATUS_BUSY;
+ case MGMT_STATUS_NOT_SUPPORTED:
+ return HAL_STATUS_UNSUPPORTED;
+ case MGMT_STATUS_INVALID_PARAMS:
+ return HAL_STATUS_INVALID;
+ case MGMT_STATUS_AUTH_FAILED:
+ return HAL_STATUS_AUTH_FAILURE;
+ case MGMT_STATUS_NOT_CONNECTED:
+ return HAL_STATUS_REMOTE_DEVICE_DOWN;
+ default:
+ return HAL_STATUS_FAILED;
+ }
+}
+
static void mgmt_connect_failed_event(uint16_t index, uint16_t length,
const void *param, void *user_data)
{
+ const struct mgmt_ev_connect_failed *ev = param;
+
DBG("");
+
+ /* In case security mode 3 pairing we will get connect failed event
+ * in case e.g wrong PIN code entered. Let's check if device is
+ * bonding, if so update bond state */
+ set_device_bond_state(&ev->addr.bdaddr, status_mgmt2hal(ev->status),
+ HAL_BOND_STATE_NONE);
}
static void mgmt_auth_failed_event(uint16_t index, uint16_t length,
const void *param, void *user_data)
{
+ const struct mgmt_ev_auth_failed *ev = param;
+
DBG("");
+
+ set_device_bond_state(&ev->addr.bdaddr, status_mgmt2hal(ev->status),
+ HAL_BOND_STATE_NONE);
}
static void mgmt_device_unpaired_event(uint16_t index, uint16_t length,
@@ -1931,28 +1966,6 @@ static uint8_t set_property(void *buf, uint16_t len)
}
}
-static uint8_t status_mgmt2hal(uint8_t mgmt)
-{
- switch (mgmt) {
- case MGMT_STATUS_SUCCESS:
- return HAL_STATUS_SUCCESS;
- case MGMT_STATUS_NO_RESOURCES:
- return HAL_STATUS_NOMEM;
- case MGMT_STATUS_BUSY:
- return HAL_STATUS_BUSY;
- case MGMT_STATUS_NOT_SUPPORTED:
- return HAL_STATUS_UNSUPPORTED;
- case MGMT_STATUS_INVALID_PARAMS:
- return HAL_STATUS_INVALID;
- case MGMT_STATUS_AUTH_FAILED:
- return HAL_STATUS_AUTH_FAILURE;
- case MGMT_STATUS_NOT_CONNECTED:
- return HAL_STATUS_REMOTE_DEVICE_DOWN;
- default:
- return HAL_STATUS_FAILED;
- }
-}
-
static void pair_device_complete(uint8_t status, uint16_t length,
const void *param, void *user_data)
{
--
1.8.4
---
android/bluetooth.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 65 insertions(+), 6 deletions(-)
diff --git a/android/bluetooth.c b/android/bluetooth.c
index f70db5b..ce153c2 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -96,6 +96,7 @@ static struct {
struct device {
bdaddr_t bdaddr;
int bond_state;
+ char *name;
};
struct browse_req {
@@ -330,6 +331,34 @@ static void send_bond_state_change(const bdaddr_t *addr, uint8_t status,
HAL_EV_BOND_STATE_CHANGED, sizeof(ev), &ev, -1);
}
+static void cache_device_name(const bdaddr_t *addr, char *name)
+{
+ struct device *dev = NULL;
+ GSList *l;
+
+ l = g_slist_find_custom(devices, addr, bdaddr_cmp);
+ if (l)
+ dev = l->data;
+
+ if (!dev) {
+ dev = g_new0(struct device, 1);
+ bacpy(&dev->bdaddr, addr);
+ dev->bond_state = HAL_BOND_STATE_NONE;
+ dev->name = g_malloc0(strlen(name) + 1);
+ memcpy(dev->name, name, strlen(name));
+
+ devices = g_slist_prepend(devices, dev);
+ return;
+ }
+
+ if (!g_strcmp0(dev->name, name))
+ return;
+
+ g_free(dev->name);
+ dev->name = g_strdup(name);
+ /*TODO: Do some real caching here */
+}
+
static void set_device_bond_state(const bdaddr_t *addr, uint8_t status,
int state) {
@@ -542,10 +571,35 @@ static void new_link_key_callback(uint16_t index, uint16_t length,
browse_remote_sdp(&addr->bdaddr);
}
-static void send_remote_device_name_prop(const bdaddr_t *bdaddr, char *name)
+static char* get_device_name(const bdaddr_t *addr)
+{
+ GSList *l;
+ struct device *dev;
+
+ l = g_slist_find_custom(devices, addr, bdaddr_cmp);
+ if (l) {
+ dev = l->data;
+ return dev->name;
+ }
+
+ return NULL;
+}
+
+static void send_remote_device_name_prop(const bdaddr_t *bdaddr)
{
struct hal_ev_remote_device_props *ev;
- uint8_t buf[BASELEN_REMOTE_DEV_PROP + strlen(name)];
+ uint8_t *buf;
+ char dst[18];
+ char *name;
+
+ ba2str(bdaddr, dst);
+
+ /* Use cached name or bdaddr string */
+ name = get_device_name(bdaddr);
+ if (!name)
+ name = dst;
+
+ buf = g_malloc0(BASELEN_REMOTE_DEV_PROP + strlen(name));
ev = (void *) buf;
memset(buf, 0, sizeof(buf));
@@ -559,6 +613,8 @@ static void send_remote_device_name_prop(const bdaddr_t *bdaddr, char *name)
ipc_send(notification_sk, HAL_SERVICE_ID_BLUETOOTH,
HAL_EV_REMOTE_DEVICE_PROPS, sizeof(buf), ev, -1);
+
+ g_free(buf);
}
static void pin_code_request_callback(uint16_t index, uint16_t length,
@@ -576,15 +632,16 @@ static void pin_code_request_callback(uint16_t index, uint16_t length,
ba2str(&ev->addr.bdaddr, dst);
/* Workaround for Android Bluetooth.apk issue: send remote
- * device property. Lets use address as a name for now */
- send_remote_device_name_prop(&ev->addr.bdaddr, dst);
+ * device property */
+ send_remote_device_name_prop(&ev->addr.bdaddr);
set_device_bond_state(&ev->addr.bdaddr, HAL_STATUS_SUCCESS,
HAL_BOND_STATE_BONDING);
DBG("%s type %u secure %u", dst, ev->addr.type, ev->secure);
- /* TODO name and CoD of remote devices should probably be cached */
+ /* TODO CoD of remote devices should probably be cached
+ * Name we already send in remote device prop */
memset(&hal_ev, 0, sizeof(hal_ev));
bdaddr2android(&ev->addr.bdaddr, hal_ev.bdaddr);
@@ -799,8 +856,10 @@ static void update_found_device(const bdaddr_t *bdaddr, uint8_t bdaddr_type,
props_size += sizeof(struct hal_property) + sizeof(eir.class);
props_size += sizeof(struct hal_property) + sizeof(rssi);
- if (eir.name)
+ if (eir.name) {
props_size += sizeof(struct hal_property) + strlen(eir.name);
+ cache_device_name(remote, eir.name);
+ }
if (is_new_dev) {
struct hal_ev_device_found *ev = NULL;
--
1.8.4
Before sending any ssp request or pin code request up to HAL library we
need to send bond state change with bonding state. Otherwise incoming
bonding is not correctly handled by Bluetooth.apk.
In this patch also device list has been added in order to e.g track
bonding state.
Note: For incoming paring (security mode 3) there is a need to send
HAL_EVE_REMOTE_DEVICE_PROPS before HAL_EV_PIN_REQUEST.
It is because Android will crash due to bug in pinRequestCallback
function in java. Android checks if device is already in HashMap and if
not then creates device, but forget to use that one, but instead do
operations on NULL. By sending HAL_BOND_STATE_BONDING event it works
better but we have race issue. It is because new device is added to
HashMap not in callback context but later after BONDING msg will be
received by BondStateMachine. If it happens before pin_request_cb hits
java then we are fine, otherwise not. So for that reason we send
HAL_EV_REMOTE_DEVICE_PROPS so in the java handler class new device will
be added to HashMap in the callback context.
In ssp case we don't have this problem as we send device found once acl
is created.
---
android/bluetooth.c | 104 ++++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 89 insertions(+), 15 deletions(-)
diff --git a/android/bluetooth.c b/android/bluetooth.c
index 39589fa..f70db5b 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -62,6 +62,8 @@ static uint16_t option_index = MGMT_INDEX_NONE;
static int notification_sk = -1;
+#define BASELEN_REMOTE_DEV_PROP (sizeof(struct hal_ev_remote_device_props) \
+ + sizeof(struct hal_property))
/* This list contains addresses which are asked for records */
static GSList *browse_reqs;
@@ -91,6 +93,11 @@ static struct {
.uuids = NULL,
};
+struct device {
+ bdaddr_t bdaddr;
+ int bond_state;
+};
+
struct browse_req {
bdaddr_t bdaddr;
GSList *uuids;
@@ -106,6 +113,7 @@ static const uint16_t uuid_list[] = {
};
static GSList *found_devices = NULL;
+static GSList *devices = NULL;
static void adapter_name_changed(const uint8_t *name)
{
@@ -301,6 +309,14 @@ static void store_link_key(const bdaddr_t *dst, const uint8_t *key,
}
+static int bdaddr_cmp(gconstpointer a, gconstpointer b)
+{
+ const bdaddr_t *bda = a;
+ const bdaddr_t *bdb = b;
+
+ return bacmp(bdb, bda);
+}
+
static void send_bond_state_change(const bdaddr_t *addr, uint8_t status,
uint8_t state)
{
@@ -314,6 +330,29 @@ static void send_bond_state_change(const bdaddr_t *addr, uint8_t status,
HAL_EV_BOND_STATE_CHANGED, sizeof(ev), &ev, -1);
}
+static void set_device_bond_state(const bdaddr_t *addr, uint8_t status,
+ int state) {
+
+ struct device *dev = NULL;
+ GSList *l;
+
+ l = g_slist_find_custom(devices, addr, bdaddr_cmp);
+ if (l)
+ dev = l->data;
+
+ if (!dev) {
+ dev = g_new0(struct device, 1);
+ bacpy(&dev->bdaddr, addr);
+ dev->bond_state = HAL_BOND_STATE_NONE;
+ devices = g_slist_prepend(devices, dev);
+ }
+
+ if (dev->bond_state != state) {
+ dev->bond_state = state;
+ send_bond_state_change(&dev->bdaddr, status, state);
+ }
+}
+
static void browse_req_free(struct browse_req *req)
{
g_slist_free_full(req->uuids, g_free);
@@ -497,12 +536,31 @@ static void new_link_key_callback(uint16_t index, uint16_t length,
key->pin_len);
}
- send_bond_state_change(&addr->bdaddr, HAL_STATUS_SUCCESS,
+ set_device_bond_state(&addr->bdaddr, HAL_STATUS_SUCCESS,
HAL_BOND_STATE_BONDED);
browse_remote_sdp(&addr->bdaddr);
}
+static void send_remote_device_name_prop(const bdaddr_t *bdaddr, char *name)
+{
+ struct hal_ev_remote_device_props *ev;
+ uint8_t buf[BASELEN_REMOTE_DEV_PROP + strlen(name)];
+
+ ev = (void *) buf;
+ memset(buf, 0, sizeof(buf));
+
+ ev->status = HAL_STATUS_SUCCESS;
+ bdaddr2android(bdaddr, ev->bdaddr);
+ ev->num_props = 1;
+ ev->props[0].type = HAL_PROP_DEVICE_NAME;
+ ev->props[0].len = strlen(name);
+ memcpy(&ev->props[0].val, name, strlen(name));
+
+ ipc_send(notification_sk, HAL_SERVICE_ID_BLUETOOTH,
+ HAL_EV_REMOTE_DEVICE_PROPS, sizeof(buf), ev, -1);
+}
+
static void pin_code_request_callback(uint16_t index, uint16_t length,
const void *param, void *user_data)
{
@@ -517,6 +575,13 @@ static void pin_code_request_callback(uint16_t index, uint16_t length,
ba2str(&ev->addr.bdaddr, dst);
+ /* Workaround for Android Bluetooth.apk issue: send remote
+ * device property. Lets use address as a name for now */
+ send_remote_device_name_prop(&ev->addr.bdaddr, dst);
+
+ set_device_bond_state(&ev->addr.bdaddr, HAL_STATUS_SUCCESS,
+ HAL_BOND_STATE_BONDING);
+
DBG("%s type %u secure %u", dst, ev->addr.type, ev->secure);
/* TODO name and CoD of remote devices should probably be cached */
@@ -556,6 +621,9 @@ static void user_confirm_request_callback(uint16_t index, uint16_t length,
ba2str(&ev->addr.bdaddr, dst);
DBG("%s confirm_hint %u", dst, ev->confirm_hint);
+ set_device_bond_state(&ev->addr.bdaddr, HAL_STATUS_SUCCESS,
+ HAL_BOND_STATE_BONDING);
+
if (ev->confirm_hint)
send_ssp_request(&ev->addr.bdaddr, HAL_SSP_VARIANT_CONSENT, 0);
else
@@ -577,6 +645,9 @@ static void user_passkey_request_callback(uint16_t index, uint16_t length,
ba2str(&ev->addr.bdaddr, dst);
DBG("%s", dst);
+ set_device_bond_state(&ev->addr.bdaddr, HAL_STATUS_SUCCESS,
+ HAL_BOND_STATE_BONDING);
+
send_ssp_request(&ev->addr.bdaddr, HAL_SSP_VARIANT_ENTRY, 0);
}
@@ -595,8 +666,13 @@ static void user_passkey_notify_callback(uint16_t index, uint16_t length,
DBG("%s entered %u", dst, ev->entered);
/* HAL seems to not support entered characters */
- if (!ev->entered)
- send_ssp_request(&ev->addr.bdaddr, HAL_SSP_VARIANT_NOTIF,
+ if (ev->entered)
+ return;
+
+ set_device_bond_state(&ev->addr.bdaddr, HAL_STATUS_SUCCESS,
+ HAL_BOND_STATE_BONDING);
+
+ send_ssp_request(&ev->addr.bdaddr, HAL_SSP_VARIANT_NOTIF,
ev->passkey);
}
@@ -647,14 +723,6 @@ static void confirm_device_name(const bdaddr_t *addr, uint8_t addr_type)
error("Failed to send confirm name request");
}
-static int bdaddr_cmp(gconstpointer a, gconstpointer b)
-{
- const bdaddr_t *bda = a;
- const bdaddr_t *bdb = b;
-
- return bacmp(bdb, bda);
-}
-
static int fill_device_props(struct hal_property *prop, bdaddr_t *addr,
uint32_t cod, int8_t rssi, char *name)
{
@@ -1838,7 +1906,7 @@ static void pair_device_complete(uint8_t status, uint16_t length,
if (status == MGMT_STATUS_SUCCESS)
return;
- send_bond_state_change(&rp->addr.bdaddr, status_mgmt2hal(status),
+ set_device_bond_state(&rp->addr.bdaddr, status_mgmt2hal(status),
HAL_BOND_STATE_NONE);
}
@@ -1855,7 +1923,7 @@ static bool create_bond(void *buf, uint16_t len)
&cp, pair_device_complete, NULL, NULL) == 0)
return false;
- send_bond_state_change(&cp.addr.bdaddr, HAL_STATUS_SUCCESS,
+ set_device_bond_state(&cp.addr.bdaddr, HAL_STATUS_SUCCESS,
HAL_BOND_STATE_BONDING);
return true;
@@ -1865,12 +1933,18 @@ static bool cancel_bond(void *buf, uint16_t len)
{
struct hal_cmd_cancel_bond *cmd = buf;
struct mgmt_addr_info cp;
+ bool result;
cp.type = BDADDR_BREDR;
android2bdaddr(cmd->bdaddr, &cp.bdaddr);
- return mgmt_reply(mgmt_if, MGMT_OP_CANCEL_PAIR_DEVICE, adapter.index,
+ result = mgmt_reply(mgmt_if, MGMT_OP_CANCEL_PAIR_DEVICE, adapter.index,
sizeof(cp), &cp, NULL, NULL, NULL) > 0;
+ if (result)
+ set_device_bond_state(&cp.bdaddr, HAL_STATUS_SUCCESS,
+ HAL_BOND_STATE_NONE);
+
+ return result;
}
static void unpair_device_complete(uint8_t status, uint16_t length,
@@ -1883,7 +1957,7 @@ static void unpair_device_complete(uint8_t status, uint16_t length,
if (status != MGMT_STATUS_SUCCESS)
return;
- send_bond_state_change(&rp->addr.bdaddr, HAL_STATUS_SUCCESS,
+ set_device_bond_state(&rp->addr.bdaddr, HAL_STATUS_SUCCESS,
HAL_BOND_STATE_NONE);
}
--
1.8.4