To optimize BLE HID devices reconnection response, we can cache the
report map so that the subsequent reconnections do not need round trip
time to read the report map.
Reviewed-by: Alain Michaud <[email protected]>
---
profiles/input/hog-lib.c | 142 ++++++++++++++++++++++++++++++++-------
1 file changed, 118 insertions(+), 24 deletions(-)
diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
index ee811d301..e5fef4c7c 100644
--- a/profiles/input/hog-lib.c
+++ b/profiles/input/hog-lib.c
@@ -95,6 +95,13 @@ struct bt_hog {
struct queue *bas;
GSList *instances;
struct queue *gatt_op;
+ struct gatt_db *gatt_db;
+ struct gatt_db_attribute *report_map_attr;
+};
+
+struct report_map {
+ uint8_t value[HOG_REPORT_MAP_MAX_SIZE];
+ size_t length;
};
struct report {
@@ -924,33 +931,16 @@ static char *item2string(char *str, uint8_t *buf, uint8_t len)
return str;
}
-static void report_map_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
- gpointer user_data)
+static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
+ ssize_t report_map_len)
{
- struct gatt_request *req = user_data;
- struct bt_hog *hog = req->user_data;
- uint8_t value[HOG_REPORT_MAP_MAX_SIZE];
+ uint8_t *value = report_map;
struct uhid_event ev;
- ssize_t vlen;
+ ssize_t vlen = report_map_len;
char itemstr[20]; /* 5x3 (data) + 4 (continuation) + 1 (null) */
int i, err;
GError *gerr = NULL;
- destroy_gatt_req(req);
-
- DBG("HoG inspecting report map");
-
- if (status != 0) {
- error("Report Map read failed: %s", att_ecode2str(status));
- return;
- }
-
- vlen = dec_read_resp(pdu, plen, value, sizeof(value));
- if (vlen < 0) {
- error("ATT protocol error");
- return;
- }
-
DBG("Report MAP:");
for (i = 0; i < vlen;) {
ssize_t ilen = 0;
@@ -1022,6 +1012,43 @@ static void report_map_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
DBG("HoG created uHID device");
}
+static void db_report_map_write_value_cb(struct gatt_db_attribute *attr,
+ int err, void *user_data)
+{
+ if (err)
+ error("Error writing report map value to gatt db");
+}
+
+static void report_map_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
+ gpointer user_data)
+{
+ struct gatt_request *req = user_data;
+ struct bt_hog *hog = req->user_data;
+ uint8_t value[HOG_REPORT_MAP_MAX_SIZE];
+ ssize_t vlen;
+
+ destroy_gatt_req(req);
+
+ DBG("HoG inspecting report map");
+
+ if (status != 0) {
+ error("Report Map read failed: %s", att_ecode2str(status));
+ return;
+ }
+
+ vlen = dec_read_resp(pdu, plen, value, sizeof(value));
+ if (vlen < 0) {
+ error("ATT protocol error");
+ return;
+ }
+
+ uhid_create(hog, value, vlen);
+
+ /* Cache the report map to optimize reconnection */
+ gatt_db_attribute_write(hog->report_map_attr, 0, value, vlen, 0, NULL,
+ db_report_map_write_value_cb, NULL);
+}
+
static void info_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
gpointer user_data)
{
@@ -1269,12 +1296,32 @@ static void foreach_hog_external(struct gatt_db_attribute *attr,
external_report_reference_cb, hog);
}
+static void db_report_map_read_value_cb(struct gatt_db_attribute *attrib,
+ int err, const uint8_t *value,
+ size_t length, void *user_data)
+{
+ struct report_map *map = user_data;
+
+ if (err) {
+ error("Error reading report map from gatt db %s",
+ strerror(-err));
+ return;
+ }
+
+ if (!length)
+ return;
+
+ map->length = length < sizeof(map->value) ? length : sizeof(map->value);
+ memcpy(map->value, value, map->length);
+}
+
static void foreach_hog_chrc(struct gatt_db_attribute *attr, void *user_data)
{
struct bt_hog *hog = user_data;
bt_uuid_t uuid, report_uuid, report_map_uuid, info_uuid;
bt_uuid_t proto_mode_uuid, ctrlpt_uuid;
uint16_t handle, value_handle;
+ struct report_map report_map = {0};
gatt_db_attribute_get_char_data(attr, &handle, &value_handle, NULL,
NULL, &uuid);
@@ -1288,8 +1335,24 @@ static void foreach_hog_chrc(struct gatt_db_attribute *attr, void *user_data)
bt_uuid16_create(&report_map_uuid, HOG_REPORT_MAP_UUID);
if (!bt_uuid_cmp(&report_map_uuid, &uuid)) {
- read_char(hog, hog->attrib, value_handle, report_map_read_cb,
- hog);
+
+ hog->report_map_attr = gatt_db_get_attribute(hog->gatt_db,
+ value_handle);
+ gatt_db_attribute_read(hog->report_map_attr, 0,
+ BT_ATT_OP_READ_REQ, NULL,
+ db_report_map_read_value_cb,
+ &report_map);
+
+ if (report_map.length) {
+ /* Report map found in the cache, straight to creating
+ * UHID to optimize reconnection.
+ */
+ uhid_create(hog, report_map.value, report_map.length);
+ } else {
+ read_char(hog, hog->attrib, value_handle,
+ report_map_read_cb, hog);
+ }
+
gatt_db_service_foreach_desc(attr, foreach_hog_external, hog);
return;
}
@@ -1417,6 +1480,8 @@ struct bt_hog *bt_hog_new(int fd, const char *name, uint16_t vendor,
hog->dis = bt_dis_new(db);
bt_dis_set_notification(hog->dis, dis_notify, hog);
}
+
+ hog->gatt_db = db;
}
return bt_hog_ref(hog);
@@ -1612,9 +1677,14 @@ bool bt_hog_attach(struct bt_hog *hog, void *gatt)
hog->primary->range.start,
hog->primary->range.end, NULL,
char_discovered_cb, hog);
- return true;
}
+ if (!hog->uhid_created)
+ return true;
+
+ /* If UHID is already created, set up the report value handlers to
+ * optimize reconnection.
+ */
for (l = hog->reports; l; l = l->next) {
struct report *r = l->data;
@@ -1627,6 +1697,29 @@ bool bt_hog_attach(struct bt_hog *hog, void *gatt)
return true;
}
+static void uhid_destroy(struct bt_hog *hog)
+{
+ int err;
+ struct uhid_event ev;
+
+ if (!hog->uhid_created)
+ return;
+
+ bt_uhid_unregister_all(hog->uhid);
+
+ memset(&ev, 0, sizeof(ev));
+ ev.type = UHID_DESTROY;
+
+ err = bt_uhid_send(hog->uhid, &ev);
+
+ if (err < 0) {
+ error("bt_uhid_send: %s", strerror(-err));
+ return;
+ }
+
+ hog->uhid_created = false;
+}
+
void bt_hog_detach(struct bt_hog *hog)
{
GSList *l;
@@ -1660,6 +1753,7 @@ void bt_hog_detach(struct bt_hog *hog)
queue_foreach(hog->gatt_op, (void *) cancel_gatt_req, NULL);
g_attrib_unref(hog->attrib);
hog->attrib = NULL;
+ uhid_destroy(hog);
}
int bt_hog_set_control_point(struct bt_hog *hog, bool suspend)
--
2.26.2
Hi Sonny,
On Sat, Dec 12, 2020 at 9:57 PM Sonny Sasaka <[email protected]> wrote:
>
> To optimize BLE HID devices reconnection response, we can cache the
> report map so that the subsequent reconnections do not need round trip
> time to read the report map.
>
> Reviewed-by: Alain Michaud <[email protected]>
>
> ---
> profiles/input/hog-lib.c | 142 ++++++++++++++++++++++++++++++++-------
> 1 file changed, 118 insertions(+), 24 deletions(-)
>
> diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> index ee811d301..e5fef4c7c 100644
> --- a/profiles/input/hog-lib.c
> +++ b/profiles/input/hog-lib.c
> @@ -95,6 +95,13 @@ struct bt_hog {
> struct queue *bas;
> GSList *instances;
> struct queue *gatt_op;
> + struct gatt_db *gatt_db;
> + struct gatt_db_attribute *report_map_attr;
> +};
> +
> +struct report_map {
> + uint8_t value[HOG_REPORT_MAP_MAX_SIZE];
> + size_t length;
> };
>
> struct report {
> @@ -924,33 +931,16 @@ static char *item2string(char *str, uint8_t *buf, uint8_t len)
> return str;
> }
>
> -static void report_map_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
> - gpointer user_data)
> +static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
> + ssize_t report_map_len)
> {
> - struct gatt_request *req = user_data;
> - struct bt_hog *hog = req->user_data;
> - uint8_t value[HOG_REPORT_MAP_MAX_SIZE];
> + uint8_t *value = report_map;
> struct uhid_event ev;
> - ssize_t vlen;
> + ssize_t vlen = report_map_len;
> char itemstr[20]; /* 5x3 (data) + 4 (continuation) + 1 (null) */
> int i, err;
> GError *gerr = NULL;
>
> - destroy_gatt_req(req);
> -
> - DBG("HoG inspecting report map");
> -
> - if (status != 0) {
> - error("Report Map read failed: %s", att_ecode2str(status));
> - return;
> - }
> -
> - vlen = dec_read_resp(pdu, plen, value, sizeof(value));
> - if (vlen < 0) {
> - error("ATT protocol error");
> - return;
> - }
> -
> DBG("Report MAP:");
> for (i = 0; i < vlen;) {
> ssize_t ilen = 0;
> @@ -1022,6 +1012,43 @@ static void report_map_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
> DBG("HoG created uHID device");
> }
>
> +static void db_report_map_write_value_cb(struct gatt_db_attribute *attr,
> + int err, void *user_data)
> +{
> + if (err)
> + error("Error writing report map value to gatt db");
> +}
> +
> +static void report_map_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
> + gpointer user_data)
> +{
> + struct gatt_request *req = user_data;
> + struct bt_hog *hog = req->user_data;
> + uint8_t value[HOG_REPORT_MAP_MAX_SIZE];
> + ssize_t vlen;
> +
> + destroy_gatt_req(req);
> +
> + DBG("HoG inspecting report map");
> +
> + if (status != 0) {
> + error("Report Map read failed: %s", att_ecode2str(status));
> + return;
> + }
> +
> + vlen = dec_read_resp(pdu, plen, value, sizeof(value));
> + if (vlen < 0) {
> + error("ATT protocol error");
> + return;
> + }
> +
> + uhid_create(hog, value, vlen);
> +
> + /* Cache the report map to optimize reconnection */
> + gatt_db_attribute_write(hog->report_map_attr, 0, value, vlen, 0, NULL,
> + db_report_map_write_value_cb, NULL);
> +}
> +
> static void info_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
> gpointer user_data)
> {
> @@ -1269,12 +1296,32 @@ static void foreach_hog_external(struct gatt_db_attribute *attr,
> external_report_reference_cb, hog);
> }
>
> +static void db_report_map_read_value_cb(struct gatt_db_attribute *attrib,
> + int err, const uint8_t *value,
> + size_t length, void *user_data)
> +{
> + struct report_map *map = user_data;
> +
> + if (err) {
> + error("Error reading report map from gatt db %s",
> + strerror(-err));
> + return;
> + }
> +
> + if (!length)
> + return;
> +
> + map->length = length < sizeof(map->value) ? length : sizeof(map->value);
> + memcpy(map->value, value, map->length);
> +}
> +
> static void foreach_hog_chrc(struct gatt_db_attribute *attr, void *user_data)
> {
> struct bt_hog *hog = user_data;
> bt_uuid_t uuid, report_uuid, report_map_uuid, info_uuid;
> bt_uuid_t proto_mode_uuid, ctrlpt_uuid;
> uint16_t handle, value_handle;
> + struct report_map report_map = {0};
>
> gatt_db_attribute_get_char_data(attr, &handle, &value_handle, NULL,
> NULL, &uuid);
> @@ -1288,8 +1335,24 @@ static void foreach_hog_chrc(struct gatt_db_attribute *attr, void *user_data)
>
> bt_uuid16_create(&report_map_uuid, HOG_REPORT_MAP_UUID);
> if (!bt_uuid_cmp(&report_map_uuid, &uuid)) {
> - read_char(hog, hog->attrib, value_handle, report_map_read_cb,
> - hog);
> +
> + hog->report_map_attr = gatt_db_get_attribute(hog->gatt_db,
> + value_handle);
> + gatt_db_attribute_read(hog->report_map_attr, 0,
> + BT_ATT_OP_READ_REQ, NULL,
> + db_report_map_read_value_cb,
> + &report_map);
> +
> + if (report_map.length) {
> + /* Report map found in the cache, straight to creating
> + * UHID to optimize reconnection.
> + */
> + uhid_create(hog, report_map.value, report_map.length);
> + } else {
> + read_char(hog, hog->attrib, value_handle,
> + report_map_read_cb, hog);
> + }
> +
> gatt_db_service_foreach_desc(attr, foreach_hog_external, hog);
> return;
> }
> @@ -1417,6 +1480,8 @@ struct bt_hog *bt_hog_new(int fd, const char *name, uint16_t vendor,
> hog->dis = bt_dis_new(db);
> bt_dis_set_notification(hog->dis, dis_notify, hog);
> }
> +
> + hog->gatt_db = db;
Is this really supposed to be a weak reference?
> }
>
> return bt_hog_ref(hog);
> @@ -1612,9 +1677,14 @@ bool bt_hog_attach(struct bt_hog *hog, void *gatt)
> hog->primary->range.start,
> hog->primary->range.end, NULL,
> char_discovered_cb, hog);
> - return true;
> }
>
> + if (!hog->uhid_created)
> + return true;
> +
> + /* If UHID is already created, set up the report value handlers to
> + * optimize reconnection.
> + */
> for (l = hog->reports; l; l = l->next) {
> struct report *r = l->data;
>
> @@ -1627,6 +1697,29 @@ bool bt_hog_attach(struct bt_hog *hog, void *gatt)
> return true;
> }
>
> +static void uhid_destroy(struct bt_hog *hog)
> +{
> + int err;
> + struct uhid_event ev;
> +
> + if (!hog->uhid_created)
> + return;
> +
> + bt_uhid_unregister_all(hog->uhid);
> +
> + memset(&ev, 0, sizeof(ev));
> + ev.type = UHID_DESTROY;
> +
> + err = bt_uhid_send(hog->uhid, &ev);
> +
> + if (err < 0) {
> + error("bt_uhid_send: %s", strerror(-err));
> + return;
> + }
> +
> + hog->uhid_created = false;
> +}
> +
> void bt_hog_detach(struct bt_hog *hog)
> {
> GSList *l;
> @@ -1660,6 +1753,7 @@ void bt_hog_detach(struct bt_hog *hog)
> queue_foreach(hog->gatt_op, (void *) cancel_gatt_req, NULL);
> g_attrib_unref(hog->attrib);
> hog->attrib = NULL;
> + uhid_destroy(hog);
> }
>
> int bt_hog_set_control_point(struct bt_hog *hog, bool suspend)
> --
> 2.26.2
>
--
Luiz Augusto von Dentz
Hi Luiz,
On Mon, Dec 14, 2020 at 10:20 AM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Sonny,
>
> On Sat, Dec 12, 2020 at 9:57 PM Sonny Sasaka <[email protected]> wrote:
> >
> > To optimize BLE HID devices reconnection response, we can cache the
> > report map so that the subsequent reconnections do not need round trip
> > time to read the report map.
> >
> > Reviewed-by: Alain Michaud <[email protected]>
> >
> > ---
> > profiles/input/hog-lib.c | 142 ++++++++++++++++++++++++++++++++-------
> > 1 file changed, 118 insertions(+), 24 deletions(-)
> >
> > diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> > index ee811d301..e5fef4c7c 100644
> > --- a/profiles/input/hog-lib.c
> > +++ b/profiles/input/hog-lib.c
> > @@ -95,6 +95,13 @@ struct bt_hog {
> > struct queue *bas;
> > GSList *instances;
> > struct queue *gatt_op;
> > + struct gatt_db *gatt_db;
> > + struct gatt_db_attribute *report_map_attr;
> > +};
> > +
> > +struct report_map {
> > + uint8_t value[HOG_REPORT_MAP_MAX_SIZE];
> > + size_t length;
> > };
> >
> > struct report {
> > @@ -924,33 +931,16 @@ static char *item2string(char *str, uint8_t *buf, uint8_t len)
> > return str;
> > }
> >
> > -static void report_map_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
> > - gpointer user_data)
> > +static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
> > + ssize_t report_map_len)
> > {
> > - struct gatt_request *req = user_data;
> > - struct bt_hog *hog = req->user_data;
> > - uint8_t value[HOG_REPORT_MAP_MAX_SIZE];
> > + uint8_t *value = report_map;
> > struct uhid_event ev;
> > - ssize_t vlen;
> > + ssize_t vlen = report_map_len;
> > char itemstr[20]; /* 5x3 (data) + 4 (continuation) + 1 (null) */
> > int i, err;
> > GError *gerr = NULL;
> >
> > - destroy_gatt_req(req);
> > -
> > - DBG("HoG inspecting report map");
> > -
> > - if (status != 0) {
> > - error("Report Map read failed: %s", att_ecode2str(status));
> > - return;
> > - }
> > -
> > - vlen = dec_read_resp(pdu, plen, value, sizeof(value));
> > - if (vlen < 0) {
> > - error("ATT protocol error");
> > - return;
> > - }
> > -
> > DBG("Report MAP:");
> > for (i = 0; i < vlen;) {
> > ssize_t ilen = 0;
> > @@ -1022,6 +1012,43 @@ static void report_map_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
> > DBG("HoG created uHID device");
> > }
> >
> > +static void db_report_map_write_value_cb(struct gatt_db_attribute *attr,
> > + int err, void *user_data)
> > +{
> > + if (err)
> > + error("Error writing report map value to gatt db");
> > +}
> > +
> > +static void report_map_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
> > + gpointer user_data)
> > +{
> > + struct gatt_request *req = user_data;
> > + struct bt_hog *hog = req->user_data;
> > + uint8_t value[HOG_REPORT_MAP_MAX_SIZE];
> > + ssize_t vlen;
> > +
> > + destroy_gatt_req(req);
> > +
> > + DBG("HoG inspecting report map");
> > +
> > + if (status != 0) {
> > + error("Report Map read failed: %s", att_ecode2str(status));
> > + return;
> > + }
> > +
> > + vlen = dec_read_resp(pdu, plen, value, sizeof(value));
> > + if (vlen < 0) {
> > + error("ATT protocol error");
> > + return;
> > + }
> > +
> > + uhid_create(hog, value, vlen);
> > +
> > + /* Cache the report map to optimize reconnection */
> > + gatt_db_attribute_write(hog->report_map_attr, 0, value, vlen, 0, NULL,
> > + db_report_map_write_value_cb, NULL);
> > +}
> > +
> > static void info_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
> > gpointer user_data)
> > {
> > @@ -1269,12 +1296,32 @@ static void foreach_hog_external(struct gatt_db_attribute *attr,
> > external_report_reference_cb, hog);
> > }
> >
> > +static void db_report_map_read_value_cb(struct gatt_db_attribute *attrib,
> > + int err, const uint8_t *value,
> > + size_t length, void *user_data)
> > +{
> > + struct report_map *map = user_data;
> > +
> > + if (err) {
> > + error("Error reading report map from gatt db %s",
> > + strerror(-err));
> > + return;
> > + }
> > +
> > + if (!length)
> > + return;
> > +
> > + map->length = length < sizeof(map->value) ? length : sizeof(map->value);
> > + memcpy(map->value, value, map->length);
> > +}
> > +
> > static void foreach_hog_chrc(struct gatt_db_attribute *attr, void *user_data)
> > {
> > struct bt_hog *hog = user_data;
> > bt_uuid_t uuid, report_uuid, report_map_uuid, info_uuid;
> > bt_uuid_t proto_mode_uuid, ctrlpt_uuid;
> > uint16_t handle, value_handle;
> > + struct report_map report_map = {0};
> >
> > gatt_db_attribute_get_char_data(attr, &handle, &value_handle, NULL,
> > NULL, &uuid);
> > @@ -1288,8 +1335,24 @@ static void foreach_hog_chrc(struct gatt_db_attribute *attr, void *user_data)
> >
> > bt_uuid16_create(&report_map_uuid, HOG_REPORT_MAP_UUID);
> > if (!bt_uuid_cmp(&report_map_uuid, &uuid)) {
> > - read_char(hog, hog->attrib, value_handle, report_map_read_cb,
> > - hog);
> > +
> > + hog->report_map_attr = gatt_db_get_attribute(hog->gatt_db,
> > + value_handle);
> > + gatt_db_attribute_read(hog->report_map_attr, 0,
> > + BT_ATT_OP_READ_REQ, NULL,
> > + db_report_map_read_value_cb,
> > + &report_map);
> > +
> > + if (report_map.length) {
> > + /* Report map found in the cache, straight to creating
> > + * UHID to optimize reconnection.
> > + */
> > + uhid_create(hog, report_map.value, report_map.length);
> > + } else {
> > + read_char(hog, hog->attrib, value_handle,
> > + report_map_read_cb, hog);
> > + }
> > +
> > gatt_db_service_foreach_desc(attr, foreach_hog_external, hog);
> > return;
> > }
> > @@ -1417,6 +1480,8 @@ struct bt_hog *bt_hog_new(int fd, const char *name, uint16_t vendor,
> > hog->dis = bt_dis_new(db);
> > bt_dis_set_notification(hog->dis, dis_notify, hog);
> > }
> > +
> > + hog->gatt_db = db;
>
> Is this really supposed to be a weak reference?
Yes, I intended it to be a weak reference, I was assuming that gatt_db
is alive as long as hog is alive. Is this assumption not true? Is it
safer to think otherwise?
>
> > }
> >
> > return bt_hog_ref(hog);
> > @@ -1612,9 +1677,14 @@ bool bt_hog_attach(struct bt_hog *hog, void *gatt)
> > hog->primary->range.start,
> > hog->primary->range.end, NULL,
> > char_discovered_cb, hog);
> > - return true;
> > }
> >
> > + if (!hog->uhid_created)
> > + return true;
> > +
> > + /* If UHID is already created, set up the report value handlers to
> > + * optimize reconnection.
> > + */
> > for (l = hog->reports; l; l = l->next) {
> > struct report *r = l->data;
> >
> > @@ -1627,6 +1697,29 @@ bool bt_hog_attach(struct bt_hog *hog, void *gatt)
> > return true;
> > }
> >
> > +static void uhid_destroy(struct bt_hog *hog)
> > +{
> > + int err;
> > + struct uhid_event ev;
> > +
> > + if (!hog->uhid_created)
> > + return;
> > +
> > + bt_uhid_unregister_all(hog->uhid);
> > +
> > + memset(&ev, 0, sizeof(ev));
> > + ev.type = UHID_DESTROY;
> > +
> > + err = bt_uhid_send(hog->uhid, &ev);
> > +
> > + if (err < 0) {
> > + error("bt_uhid_send: %s", strerror(-err));
> > + return;
> > + }
> > +
> > + hog->uhid_created = false;
> > +}
> > +
> > void bt_hog_detach(struct bt_hog *hog)
> > {
> > GSList *l;
> > @@ -1660,6 +1753,7 @@ void bt_hog_detach(struct bt_hog *hog)
> > queue_foreach(hog->gatt_op, (void *) cancel_gatt_req, NULL);
> > g_attrib_unref(hog->attrib);
> > hog->attrib = NULL;
> > + uhid_destroy(hog);
> > }
> >
> > int bt_hog_set_control_point(struct bt_hog *hog, bool suspend)
> > --
> > 2.26.2
> >
>
>
> --
> Luiz Augusto von Dentz
Hi Luiz,
On Mon, Dec 14, 2020 at 10:22 AM Sonny Sasaka <[email protected]> wrote:
>
> Hi Luiz,
>
> On Mon, Dec 14, 2020 at 10:20 AM Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > Hi Sonny,
> >
> > On Sat, Dec 12, 2020 at 9:57 PM Sonny Sasaka <[email protected]> wrote:
> > >
> > > To optimize BLE HID devices reconnection response, we can cache the
> > > report map so that the subsequent reconnections do not need round trip
> > > time to read the report map.
> > >
> > > Reviewed-by: Alain Michaud <[email protected]>
> > >
> > > ---
> > > profiles/input/hog-lib.c | 142 ++++++++++++++++++++++++++++++++-------
> > > 1 file changed, 118 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> > > index ee811d301..e5fef4c7c 100644
> > > --- a/profiles/input/hog-lib.c
> > > +++ b/profiles/input/hog-lib.c
> > > @@ -95,6 +95,13 @@ struct bt_hog {
> > > struct queue *bas;
> > > GSList *instances;
> > > struct queue *gatt_op;
> > > + struct gatt_db *gatt_db;
> > > + struct gatt_db_attribute *report_map_attr;
> > > +};
> > > +
> > > +struct report_map {
> > > + uint8_t value[HOG_REPORT_MAP_MAX_SIZE];
> > > + size_t length;
> > > };
> > >
> > > struct report {
> > > @@ -924,33 +931,16 @@ static char *item2string(char *str, uint8_t *buf, uint8_t len)
> > > return str;
> > > }
> > >
> > > -static void report_map_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
> > > - gpointer user_data)
> > > +static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
> > > + ssize_t report_map_len)
> > > {
> > > - struct gatt_request *req = user_data;
> > > - struct bt_hog *hog = req->user_data;
> > > - uint8_t value[HOG_REPORT_MAP_MAX_SIZE];
> > > + uint8_t *value = report_map;
> > > struct uhid_event ev;
> > > - ssize_t vlen;
> > > + ssize_t vlen = report_map_len;
> > > char itemstr[20]; /* 5x3 (data) + 4 (continuation) + 1 (null) */
> > > int i, err;
> > > GError *gerr = NULL;
> > >
> > > - destroy_gatt_req(req);
> > > -
> > > - DBG("HoG inspecting report map");
> > > -
> > > - if (status != 0) {
> > > - error("Report Map read failed: %s", att_ecode2str(status));
> > > - return;
> > > - }
> > > -
> > > - vlen = dec_read_resp(pdu, plen, value, sizeof(value));
> > > - if (vlen < 0) {
> > > - error("ATT protocol error");
> > > - return;
> > > - }
> > > -
> > > DBG("Report MAP:");
> > > for (i = 0; i < vlen;) {
> > > ssize_t ilen = 0;
> > > @@ -1022,6 +1012,43 @@ static void report_map_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
> > > DBG("HoG created uHID device");
> > > }
> > >
> > > +static void db_report_map_write_value_cb(struct gatt_db_attribute *attr,
> > > + int err, void *user_data)
> > > +{
> > > + if (err)
> > > + error("Error writing report map value to gatt db");
> > > +}
> > > +
> > > +static void report_map_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
> > > + gpointer user_data)
> > > +{
> > > + struct gatt_request *req = user_data;
> > > + struct bt_hog *hog = req->user_data;
> > > + uint8_t value[HOG_REPORT_MAP_MAX_SIZE];
> > > + ssize_t vlen;
> > > +
> > > + destroy_gatt_req(req);
> > > +
> > > + DBG("HoG inspecting report map");
> > > +
> > > + if (status != 0) {
> > > + error("Report Map read failed: %s", att_ecode2str(status));
> > > + return;
> > > + }
> > > +
> > > + vlen = dec_read_resp(pdu, plen, value, sizeof(value));
> > > + if (vlen < 0) {
> > > + error("ATT protocol error");
> > > + return;
> > > + }
> > > +
> > > + uhid_create(hog, value, vlen);
> > > +
> > > + /* Cache the report map to optimize reconnection */
> > > + gatt_db_attribute_write(hog->report_map_attr, 0, value, vlen, 0, NULL,
> > > + db_report_map_write_value_cb, NULL);
> > > +}
> > > +
> > > static void info_read_cb(guint8 status, const guint8 *pdu, guint16 plen,
> > > gpointer user_data)
> > > {
> > > @@ -1269,12 +1296,32 @@ static void foreach_hog_external(struct gatt_db_attribute *attr,
> > > external_report_reference_cb, hog);
> > > }
> > >
> > > +static void db_report_map_read_value_cb(struct gatt_db_attribute *attrib,
> > > + int err, const uint8_t *value,
> > > + size_t length, void *user_data)
> > > +{
> > > + struct report_map *map = user_data;
> > > +
> > > + if (err) {
> > > + error("Error reading report map from gatt db %s",
> > > + strerror(-err));
> > > + return;
> > > + }
> > > +
> > > + if (!length)
> > > + return;
> > > +
> > > + map->length = length < sizeof(map->value) ? length : sizeof(map->value);
> > > + memcpy(map->value, value, map->length);
> > > +}
> > > +
> > > static void foreach_hog_chrc(struct gatt_db_attribute *attr, void *user_data)
> > > {
> > > struct bt_hog *hog = user_data;
> > > bt_uuid_t uuid, report_uuid, report_map_uuid, info_uuid;
> > > bt_uuid_t proto_mode_uuid, ctrlpt_uuid;
> > > uint16_t handle, value_handle;
> > > + struct report_map report_map = {0};
> > >
> > > gatt_db_attribute_get_char_data(attr, &handle, &value_handle, NULL,
> > > NULL, &uuid);
> > > @@ -1288,8 +1335,24 @@ static void foreach_hog_chrc(struct gatt_db_attribute *attr, void *user_data)
> > >
> > > bt_uuid16_create(&report_map_uuid, HOG_REPORT_MAP_UUID);
> > > if (!bt_uuid_cmp(&report_map_uuid, &uuid)) {
> > > - read_char(hog, hog->attrib, value_handle, report_map_read_cb,
> > > - hog);
> > > +
> > > + hog->report_map_attr = gatt_db_get_attribute(hog->gatt_db,
> > > + value_handle);
> > > + gatt_db_attribute_read(hog->report_map_attr, 0,
> > > + BT_ATT_OP_READ_REQ, NULL,
> > > + db_report_map_read_value_cb,
> > > + &report_map);
> > > +
> > > + if (report_map.length) {
> > > + /* Report map found in the cache, straight to creating
> > > + * UHID to optimize reconnection.
> > > + */
> > > + uhid_create(hog, report_map.value, report_map.length);
> > > + } else {
> > > + read_char(hog, hog->attrib, value_handle,
> > > + report_map_read_cb, hog);
> > > + }
> > > +
> > > gatt_db_service_foreach_desc(attr, foreach_hog_external, hog);
> > > return;
> > > }
> > > @@ -1417,6 +1480,8 @@ struct bt_hog *bt_hog_new(int fd, const char *name, uint16_t vendor,
> > > hog->dis = bt_dis_new(db);
> > > bt_dis_set_notification(hog->dis, dis_notify, hog);
> > > }
> > > +
> > > + hog->gatt_db = db;
> >
> > Is this really supposed to be a weak reference?
> Yes, I intended it to be a weak reference, I was assuming that gatt_db
> is alive as long as hog is alive. Is this assumption not true? Is it
> safer to think otherwise?
Hi Luiz, as we agreed in an offline thread, I added ref counting to
this field so it is guaranteed that we don't use an already freed
gatt_db. Please take another look at v3. Thanks!
> >
> > > }
> > >
> > > return bt_hog_ref(hog);
> > > @@ -1612,9 +1677,14 @@ bool bt_hog_attach(struct bt_hog *hog, void *gatt)
> > > hog->primary->range.start,
> > > hog->primary->range.end, NULL,
> > > char_discovered_cb, hog);
> > > - return true;
> > > }
> > >
> > > + if (!hog->uhid_created)
> > > + return true;
> > > +
> > > + /* If UHID is already created, set up the report value handlers to
> > > + * optimize reconnection.
> > > + */
> > > for (l = hog->reports; l; l = l->next) {
> > > struct report *r = l->data;
> > >
> > > @@ -1627,6 +1697,29 @@ bool bt_hog_attach(struct bt_hog *hog, void *gatt)
> > > return true;
> > > }
> > >
> > > +static void uhid_destroy(struct bt_hog *hog)
> > > +{
> > > + int err;
> > > + struct uhid_event ev;
> > > +
> > > + if (!hog->uhid_created)
> > > + return;
> > > +
> > > + bt_uhid_unregister_all(hog->uhid);
> > > +
> > > + memset(&ev, 0, sizeof(ev));
> > > + ev.type = UHID_DESTROY;
> > > +
> > > + err = bt_uhid_send(hog->uhid, &ev);
> > > +
> > > + if (err < 0) {
> > > + error("bt_uhid_send: %s", strerror(-err));
> > > + return;
> > > + }
> > > +
> > > + hog->uhid_created = false;
> > > +}
> > > +
> > > void bt_hog_detach(struct bt_hog *hog)
> > > {
> > > GSList *l;
> > > @@ -1660,6 +1753,7 @@ void bt_hog_detach(struct bt_hog *hog)
> > > queue_foreach(hog->gatt_op, (void *) cancel_gatt_req, NULL);
> > > g_attrib_unref(hog->attrib);
> > > hog->attrib = NULL;
> > > + uhid_destroy(hog);
> > > }
> > >
> > > int bt_hog_set_control_point(struct bt_hog *hog, bool suspend)
> > > --
> > > 2.26.2
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz