* [PATCH BlueZ v2 1/2] Revert "input/hog: Remove HID device after HoG device disconnects"
@ 2020-12-11 23:30 Sonny Sasaka
2020-12-11 23:30 ` [PATCH BlueZ v2 2/2] input/hog: Cache the HID report map Sonny Sasaka
2020-12-12 1:13 ` [BlueZ,v2,1/2] Revert "input/hog: Remove HID device after HoG device disconnects" bluez.test.bot
0 siblings, 2 replies; 6+ messages in thread
From: Sonny Sasaka @ 2020-12-11 23:30 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Sonny Sasaka, Alain Michaud
This reverts commit d6cafa1f0c3ea1989f8a580e52f418b0998a3552.
In commit d6cafa1f0c3e ("input/hog: Remove HID device after HoG device
disconnects"), the bt_hog structure is destroyed in order to fix a bug
where the UHID connection is not destroyed. This fix has the cost of
increasing reconnection time because every reconnection would need to
re-read the report map again. An improvement to this fix is, instead of
removing the bt_hog structure, we can just destroy the UHID with
UHID_DESTROY event and use the existing bt_hog structure to keep the
cache of the report map to avoid re-reading the report map at
reconnection.
Reviewed-by: Alain Michaud <alainm@chromium.org>
---
profiles/input/hog.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/profiles/input/hog.c b/profiles/input/hog.c
index 91de4c70f..d50b82321 100644
--- a/profiles/input/hog.c
+++ b/profiles/input/hog.c
@@ -207,8 +207,6 @@ static int hog_disconnect(struct btd_service *service)
struct hog_device *dev = btd_service_get_user_data(service);
bt_hog_detach(dev->hog);
- bt_hog_unref(dev->hog);
- dev->hog = NULL;
btd_service_disconnecting_complete(service, 0);
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH BlueZ v2 2/2] input/hog: Cache the HID report map
2020-12-11 23:30 [PATCH BlueZ v2 1/2] Revert "input/hog: Remove HID device after HoG device disconnects" Sonny Sasaka
@ 2020-12-11 23:30 ` Sonny Sasaka
2020-12-14 18:20 ` Luiz Augusto von Dentz
2020-12-12 1:13 ` [BlueZ,v2,1/2] Revert "input/hog: Remove HID device after HoG device disconnects" bluez.test.bot
1 sibling, 1 reply; 6+ messages in thread
From: Sonny Sasaka @ 2020-12-11 23:30 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Sonny Sasaka, Alain Michaud
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 <alainm@chromium.org>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [BlueZ,v2,1/2] Revert "input/hog: Remove HID device after HoG device disconnects"
2020-12-11 23:30 [PATCH BlueZ v2 1/2] Revert "input/hog: Remove HID device after HoG device disconnects" Sonny Sasaka
2020-12-11 23:30 ` [PATCH BlueZ v2 2/2] input/hog: Cache the HID report map Sonny Sasaka
@ 2020-12-12 1:13 ` bluez.test.bot
1 sibling, 0 replies; 6+ messages in thread
From: bluez.test.bot @ 2020-12-12 1:13 UTC (permalink / raw)
To: linux-bluetooth, sonnysasaka
[-- Attachment #1: Type: text/plain, Size: 557 bytes --]
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=400877
---Test result---
##############################
Test: CheckPatch - PASS
##############################
Test: CheckGitLint - PASS
##############################
Test: CheckBuild - PASS
##############################
Test: MakeCheck - PASS
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH BlueZ v2 2/2] input/hog: Cache the HID report map
2020-12-11 23:30 ` [PATCH BlueZ v2 2/2] input/hog: Cache the HID report map Sonny Sasaka
@ 2020-12-14 18:20 ` Luiz Augusto von Dentz
2020-12-14 18:22 ` Sonny Sasaka
0 siblings, 1 reply; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2020-12-14 18:20 UTC (permalink / raw)
To: Sonny Sasaka; +Cc: linux-bluetooth, Alain Michaud
Hi Sonny,
On Sat, Dec 12, 2020 at 9:57 PM Sonny Sasaka <sonnysasaka@chromium.org> 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 <alainm@chromium.org>
>
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH BlueZ v2 2/2] input/hog: Cache the HID report map
2020-12-14 18:20 ` Luiz Augusto von Dentz
@ 2020-12-14 18:22 ` Sonny Sasaka
2020-12-14 22:16 ` Sonny Sasaka
0 siblings, 1 reply; 6+ messages in thread
From: Sonny Sasaka @ 2020-12-14 18:22 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, Alain Michaud
Hi Luiz,
On Mon, Dec 14, 2020 at 10:20 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Sonny,
>
> On Sat, Dec 12, 2020 at 9:57 PM Sonny Sasaka <sonnysasaka@chromium.org> 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 <alainm@chromium.org>
> >
> > ---
> > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH BlueZ v2 2/2] input/hog: Cache the HID report map
2020-12-14 18:22 ` Sonny Sasaka
@ 2020-12-14 22:16 ` Sonny Sasaka
0 siblings, 0 replies; 6+ messages in thread
From: Sonny Sasaka @ 2020-12-14 22:16 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth, Alain Michaud
Hi Luiz,
On Mon, Dec 14, 2020 at 10:22 AM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>
> Hi Luiz,
>
> On Mon, Dec 14, 2020 at 10:20 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Sonny,
> >
> > On Sat, Dec 12, 2020 at 9:57 PM Sonny Sasaka <sonnysasaka@chromium.org> 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 <alainm@chromium.org>
> > >
> > > ---
> > > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-12-14 22:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-11 23:30 [PATCH BlueZ v2 1/2] Revert "input/hog: Remove HID device after HoG device disconnects" Sonny Sasaka
2020-12-11 23:30 ` [PATCH BlueZ v2 2/2] input/hog: Cache the HID report map Sonny Sasaka
2020-12-14 18:20 ` Luiz Augusto von Dentz
2020-12-14 18:22 ` Sonny Sasaka
2020-12-14 22:16 ` Sonny Sasaka
2020-12-12 1:13 ` [BlueZ,v2,1/2] Revert "input/hog: Remove HID device after HoG device disconnects" bluez.test.bot
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.