Skip to content

Bluetooth: Classic: HID: Implement HID Device profile - #94012

Merged
nashif merged 4 commits into
zephyrproject-rtos:mainfrom
chengkai15:hid_dev
Aug 3, 2026
Merged

Bluetooth: Classic: HID: Implement HID Device profile#94012
nashif merged 4 commits into
zephyrproject-rtos:mainfrom
chengkai15:hid_dev

Conversation

@chengkai15

@chengkai15 chengkai15 commented Aug 2, 2025

Copy link
Copy Markdown
Member

Implementation Summary
This PR adds full HID Device protocol support to Zephyr's Bluetooth stack, enabling Zephyr devices to function as Bluetooth HID peripherals (e.g., keyboards, mice). The implementation includes:

Core Protocol Infrastructure

  • Dual L2CAP channel management (Control PSM=0x11, Interrupt PSM=0x13)
  • Connection manager
  • Resource initialization system (bt_hid_device_init)
  • Standardized API for device registration and callbacks

Data Handling

  • Control channel command processing (SET_REPORT/GET_REPORT)
  • Interrupt channel report transmission
  • Input report reception with validation
  • Support for all HID report types (Input/Output/Feature)

Shell Tool

shell

Connection management

br connect DC:41:A9:XX:XX:XX
hid_device register
hid_device connect
hid_device disconnect

Data operations

hid_device send # Send mouse report

Security & Compliance

HID Spec v1.1.1 compliance
Bonding and pairing integration

Comment thread subsys/bluetooth/host/classic/Kconfig
Comment thread subsys/bluetooth/host/classic/Kconfig
Comment thread subsys/bluetooth/host/classic/Kconfig Outdated
Comment thread include/zephyr/bluetooth/classic/hid_device.h
Comment thread subsys/bluetooth/host/classic/hid_device.c Outdated
Comment thread subsys/bluetooth/host/classic/hid_device.c Outdated
Comment thread subsys/bluetooth/host/classic/hid_device.c Outdated
Comment thread subsys/bluetooth/host/classic/hid_device.c Outdated
Comment thread subsys/bluetooth/host/classic/hid_device.c Outdated
Comment thread subsys/bluetooth/host/classic/hid_device.c Outdated
Comment thread subsys/bluetooth/host/classic/hid_device.c Outdated
Comment thread subsys/bluetooth/host/classic/hid_device.c Outdated
Comment thread subsys/bluetooth/host/classic/hid_device.c Outdated
@sonarqubecloud

Copy link
Copy Markdown

@chengkai15

Copy link
Copy Markdown
Member Author

@lylezhu2012 please help to review

gzh-terry
gzh-terry previously approved these changes Oct 21, 2025
@sonarqubecloud

Copy link
Copy Markdown

gzh-terry
gzh-terry previously approved these changes Nov 3, 2025
@github-actions

github-actions Bot commented Jan 3, 2026

Copy link
Copy Markdown

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 13 comments.

Comment on lines +359 to +361
if (hid_boot_mode || req->len > 0) {
report_id = net_buf_pull_u8(req);
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment on lines +332 to +343
uint8_t report_id;

if (buf->len < 1) {
return -EINVAL;
}

report_id = net_buf_pull_u8(buf);
bt_shell_print("HID: set report type %u id %u len %u", type, report_id, buf->len);

if (!hid_boot_mode && report_id != SHELL_MOUSE_REPORT_ID) {
return -ENOENT;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simple it done

Comment on lines +407 to +409
if (buf->len >= 1) {
report_id = net_buf_pull_u8(buf);
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simple it done

Comment on lines +576 to +586
if (err) {
shell_error(sh, "HID: invalid parameter");
return -EINVAL;
}

buf = bt_hid_device_create_pdu(&pool);
if (!buf) {
shell_error(sh, "HID: failed to create PDU");
return -ENOEXEC;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

over desgin for tool

Comment on lines +35 to +46
#define HID_DEVICE_BY_CTRL_CHAN(ch) \
CONTAINER_OF(ch, struct bt_hid_device, ctrl_session.br_chan.chan)

/* Get the HID device from INTR L2CAP channel */
#define HID_DEVICE_BY_INTR_CHAN(ch) \
CONTAINER_OF(ch, struct bt_hid_device, intr_session.br_chan.chan)

/* Get the HID session from L2CAP channel */
#define HID_SESSION_BY_CHAN(ch) CONTAINER_OF(ch, struct bt_hid_session, br_chan.chan)

#define HID_CHAN_TYPE(ch) \
(((ch) == NULL) ? BT_HID_CHANNEL_UNKNOWN : HID_SESSION_BY_CHAN((ch))->type)
Comment on lines +865 to +872
int bt_hid_device_get_report_rsp(struct bt_hid_device *hid, uint8_t type, struct net_buf *buf)
{
struct bt_hid_hdr *hdr;
int err;

__ASSERT_NO_MSG(hid);

if (hid->state != BT_HID_STATE_CONNECTED) {
Comment on lines +903 to +910
int bt_hid_device_input_report(struct bt_hid_device *hid, struct net_buf *buf)
{
struct bt_hid_hdr *hdr;
int err;

__ASSERT_NO_MSG(hid);

if (hid->state != BT_HID_STATE_CONNECTED) {
Comment on lines +943 to +947
LOG_DBG("virtual cable unplug");

__ASSERT_NO_MSG(hid);

if (hid->state != BT_HID_STATE_CONNECTED) {
Comment on lines +180 to +185
* This clears the callbacks, disconnects any active HID session, and
* unregisters the L2CAP servers. After this call, no further callbacks
* will be delivered, even for in-progress disconnections.
*
* @return 0 on success or a negative errno on failure.
*/
Comment on lines +7 to +16
#include <errno.h>
#include <zephyr/types.h>
#include <stddef.h>
#include <stdlib.h>
#include <string.h>
#include <zephyr/sys/byteorder.h>
#include <zephyr/kernel.h>

#include <zephyr/settings/settings.h>

@chengkai15
chengkai15 force-pushed the hid_dev branch 3 times, most recently from 43dd944 to e093f69 Compare May 29, 2026 02:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Comment on lines +11 to +12
#include <zephyr/bluetooth/classic/hid_device.h>
#include <zephyr/bluetooth/l2cap.h>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need

#include "host/shell/bt.h"
#include "common/bt_shell_private.h"

#define HELP_NONE "[none]"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

/* Shell mouse descriptor declares a single Report ID */
#define SHELL_MOUSE_REPORT_ID 2

static uint8_t mouse_descriptor[] = {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment on lines +329 to +340
uint8_t report_id;

if (buf->len < 1U) {
return -EINVAL;
}

report_id = net_buf_pull_u8(buf);
bt_shell_print("HID: set report type %u id %u len %u", type, report_id, buf->len);

if (report_id != SHELL_MOUSE_REPORT_ID) {
return -ENOENT;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refer to #94012 (comment)

Comment on lines +352 to +376
if (req->len < 1U) {
return -EINVAL;
}

report_id = net_buf_pull_u8(req);
bt_shell_print("HID: get report type %u id %u", type, report_id);

if (report_id != SHELL_MOUSE_REPORT_ID) {
return -ENOENT;
}

buf = bt_hid_device_create_pdu(&pool);
if (buf == NULL) {
bt_shell_error("HID: failed to create PDU");
return -ENOMEM;
}

net_buf_add_u8(buf, SHELL_MOUSE_REPORT_ID); /* Report ID */
net_buf_add_u8(buf, 0x00); /* buttons */
net_buf_add_u8(buf, 0x00); /* X displacement */
net_buf_add_u8(buf, 0x00); /* Y displacement */

if (!hid_boot_mode) {
net_buf_add_u8(buf, 0x00); /* wheel (report mode only) */
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggestion is incorrect per HID spec v1.1.2.

Section 3.1.2.3 (Page 30):

"In Boot Protocol Mode, there is no report descriptor, but the reports do have Report IDs as defined in Section 3.3.2 and thus do require the ReportID field of the GET_REPORT Request octet."

Section 3.3.2 (Page 43):

"All reports sent by a Bluetooth HID device in Boot Protocol Mode in response to a GET_REPORT command or sent asynchronously over the HID Interrupt Channel shall include the Report ID."

"Bluetooth HID Boot Protocol devices require a 1-octet Report ID prepended to the standard HID Boot Protocol report... mouse boot reports are 4 octets (1-octet Report ID + standard 3-octet mouse boot report)."

This is a key difference between USB HID Boot Protocol (no Report ID) and Bluetooth HID Boot Protocol (Report ID required). The current implementation is correct.

Comment on lines +562 to +572
button = shell_strtol(argv[1], 0, &err);
dx = shell_strtol(argv[2], 0, &err);
dy = shell_strtol(argv[3], 0, &err);
if (argc >= 5U) {
wheel = shell_strtol(argv[4], 0, &err);
}

if (err != 0) {
shell_error(sh, "HID: invalid parameter");
return -EINVAL;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment on lines +127 to +128
/** True when the HID host has sent SUSPEND. */
bool suspended;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the suspend state is not properly managed. The field suspended is not only changed when the HID_CONTROL(SUSPENDED/EXIT_SUSPENDED) or VC_UNPLUG message received, but also changed if the user activity occurs.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. In the Bluetooth HID Profile, SUSPEND/EXIT_SUSPEND are unidirectional control messages owned by the HID Host (spec v1.1.1 §7.4.2).

The suspended flag on the Device side reflects the Host's last control message and is therefore only driven by HID_CONTROL(SUSPEND/EXIT_SUSPEND) and cleared on VC_UNPLUG. On local user activity, the Device does not flip this flag itself — per the profile, it wakes the Host by reconnecting/role-switching, after which the Host issues EXIT_SUSPEND.

Flipping suspended on local activity would desync Device/Host state and break DRE/BV-09-C (suspend persists across reconnect). I'll clarify the doc comment to make the ownership explicit, but the state-management logic is intentional.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying that this flag represents the host's suspend state?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless of who triggers it, it resets to false.

image

hid_l2cap_registered = true;
}

hid_cb = cb;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that the variable hid_cb can be modified arbitrarily without any restrictions.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add -EALREADY handle for Duplicate Registration

return 0;
}

int bt_hid_device_connect(struct bt_conn *conn, struct bt_hid_device **hid_out)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function is not aligned with the prototype definition.

Suggested change
int bt_hid_device_connect(struct bt_conn *conn, struct bt_hid_device **hid_out)
int bt_hid_device_connect(struct bt_conn *conn, struct bt_hid_device **hid)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment on lines +819 to +823
if ((hid->ctrl_session.br_chan.chan.conn != NULL) &&
(hid->ctrl_session.br_chan.chan.conn != conn)) {
LOG_ERR("CTRL channel bound to another connection");
return -EALREADY;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same logic has been guaranteed by function hid_get_connection(). It is a duplicated code block, and also useless.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

Comment on lines +825 to +828
if (hid->intr_session.br_chan.chan.conn != NULL) {
LOG_ERR("INTR channel already connected");
return -EALREADY;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary? The check if (hid->state != BT_HID_STATE_DISCONNECTED) is enough I think.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The INTR check guards a different condition than the state check, and they're not equivalent. state is managed by the HID layer; intr_session...conn is cleared by the L2CAP stack after the disconnected callback returns (l2cap.c:296).

In bt_hid_l2cap_intr_disconnected() we call bt_hid_device_cleanup() — which sets state = DISCONNECTED — while still inside the INTR disconnected callback, so there is a window where state == DISCONNECTED but the INTR conn is still non-NULL.

Unlike the CTRL check I just removed (which hid_get_connection()/hid_conn_busy() already guarantee), nothing upstream checks the INTR conn. So this is a real defensive check against an L2CAP/HID state mismatch, not redundant with state. Suggest keep it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update comment:
/* HID spec v1.1.2 Section 5.2.1: an HID connection establishes the
* CTRL channel first, then the INTR channel; a live INTR channel
* therefore implies a fully connected HID session. Reaching here
* with a non-NULL INTR conn means a previous session's INTR channel
* is still being torn down (link already down, but the L2CAP
* chan->conn field is cleared only after bt_l2cap_br_chan_del()'s
* disconnected callback returns). This guards that residual window,
* which the state check above does not cover.
*/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In bt_hid_l2cap_intr_disconnected() we call bt_hid_device_cleanup() — which sets state = DISCONNECTED — while still inside the INTR disconnected callback, so there is a window where state == DISCONNECTED but the INTR conn is still non-NULL.

The hid device state can be changed to DISCONNECTED only if the control and interrupt channels are disconnected.

So it is hid device disconnection workflow issue. Just adding a patch here is not a good idea. We need to fix it at its root.

Comment on lines +592 to +597
if (type != BT_HID_MSG_TYPE_DATA) {
LOG_WRN("INTR type %u not supported", type);
return 0;
}

return hid_intr_handle(hid, buf);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the report type discarded?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on the Interrupt channel, host→device DATA is always an Output report so the report-type is constant (OUTPUT) and unactionable

I'd keep it as (hid, buf) — but can add it if you prefer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the report type should be checked before calling hid_intr_handle() to ensure it is Output.

Comment on lines +923 to +926
if (buf->len > BT_HID_TX_MTU) {
LOG_ERR("intr payload %u > MTU %u", buf->len, BT_HID_TX_MTU);
return -EMSGSIZE;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same TX MTU issue here. I think the check is not needed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already removed along with the control-channel one — BT_HID_TX_MTU is gone;

}

hdr = net_buf_push(buf, sizeof(struct bt_hid_hdr));
memset(hdr, 0, sizeof(struct bt_hid_hdr));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The memory clear is not needed here. The header will be set in the following.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

* destroy all bonding and virtual cable information.
*/
hid->suspended = false;
bt_br_unpair(bt_conn_get_dst_br(bt_hid_device_get_conn(hid)));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function bt_br_unpair() will cause the ACL to disconnect. All TX pending data/message/packet will be unable to be sent.

@chengkai15 chengkai15 Jun 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as #94012 (comment) discuss

and the device-initiated VCU sends the PDU first, then schedules a delayed disconnect (vcu_timeout) so the PDU is transmitted before teardown — no pending TX is lost.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion is that more comments need to be added to instruct applications to clear bonding information when appropriate.

if (hid->role == BT_HID_ROLE_ACCEPTOR) {
/* Wait for INTR channel connection from remote */
LOG_DBG("wait for INTR connection");
k_work_schedule(&hid->intr_timeout, HID_INTR_CONN_TIMEOUT);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the timer should be started when creating the control channel or receiving control channel connection request. After the timeout, any one of control and interrupt channel is not established, drop all connected l2cap channel connections.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the timeout now drops all HID channels via bt_hid_device_disconnect() whenever the connection isn't CONNECTED

@chengkai15

Copy link
Copy Markdown
Member Author

@lylezhu2012 Thanks for the thorough review — really helpful. I've pushed a force-update addressing all of your comments

Please take another review when you have a chance — thanks again!

Comment on lines +772 to +779
if (hid == NULL) {
return -EINVAL;
}

inst = hid_get_connection(conn);
if (inst == NULL) {
return (conn == NULL) ? -EINVAL : -EBUSY;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (hid == NULL) {
return -EINVAL;
}
inst = hid_get_connection(conn);
if (inst == NULL) {
return (conn == NULL) ? -EINVAL : -EBUSY;
}
if (conn == NULL || hid == NULL) {
return -EINVAL;
}
inst = hid_get_connection(conn);
if (inst == NULL) {
return -EBUSY;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment on lines +825 to +828
if (hid->intr_session.br_chan.chan.conn != NULL) {
LOG_ERR("INTR channel already connected");
return -EALREADY;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In bt_hid_l2cap_intr_disconnected() we call bt_hid_device_cleanup() — which sets state = DISCONNECTED — while still inside the INTR disconnected callback, so there is a window where state == DISCONNECTED but the INTR conn is still non-NULL.

The hid device state can be changed to DISCONNECTED only if the control and interrupt channels are disconnected.

So it is hid device disconnection workflow issue. Just adding a patch here is not a good idea. We need to fix it at its root.

*/
hid->suspended = false;

bt_br_unpair(bt_conn_get_dst_br(bt_hid_device_get_conn(hid)));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to add more comments to the doxygen of the callback vc_unplug(). It is used to instruct applications to clear the bonding information when appropriate.

Comment on lines +201 to +205
err = hid_cb->get_report(hid, report_type, buf);
if (err < 0) {
LOG_ERR("Get_Report cb err %d", err);
goto err_handshake;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Re-entrant GET_REPORT (a second GET_REPORT arrives while one is still pending): respond with a NOT_READY handshake — §3.2.1.1 allows that and the host may retry — rather than overwriting the pending request.

Do you consider change the behaviour of GET_REPORT from async to sync mode? Since the NOT_READY HANDSHAKE message can be returned, the application can return the NOT_READY if no data can be returned. And then host can retry the request.

It can reduce the complexity of the driver. And also the flag is also not needed.

Comment on lines +209 to +211
err_handshake:
hid_send_handshake(&hid->ctrl_session, hid_err_to_handshake(err));
return err;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For control, ignore errors and do not send handshake.

For other, send error handshake if return error code is not 0. For SUCCESS handshake message, I have two options,

  • Option 1
    a flag can be added to structure hid_ctrl_handlers if the SUCCESS handshake can be sent.
  • Option 2
    the SUCCESS handshake will be sent in each message handlers.

Comment on lines +420 to +423
static void hid_vc_unplug_cb(struct bt_hid_device *hid)
{
bt_shell_print("HID: virtual cable unplug");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the bonding information should be cleaned in application for reference.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

return -ENOEXEC;
}

if (!default_conn) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!default_conn) {
if (default_conn == NULL) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment on lines +580 to +582
if (err != 0) {
shell_error(sh, "HID: invalid parameter");
return -EINVAL;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seams the code block is useless.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block isn't dead — it catches malformed numeric arguments (e.g. send abc 1 2). shell_strtol() only writes *err on failure and leaves it untouched on success, so reusing the same err across the four parses accumulates correctly: if any argument fails to parse, err stays non-zero through to this check.

}

buf = bt_hid_device_create_pdu(&pool);
if (!buf) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!buf) {
if (buf == NULL) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

return -ENOEXEC;
}

if (!default_hid) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!default_hid) {
if (default_hid == NULL) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Comment on lines +786 to +788
if (cb == NULL) {
return -EINVAL;
}

@chengkai15 chengkai15 Jun 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest avoid design like the A2DP and HFP implementations.
get_report cb would check it and return errorcode to remote.

Comment on lines +220 to +230
/** @brief Register HID device callbacks.
*
* The callback pointer must remain valid for the lifetime of the HID
* subsystem or until another registration replaces it.
*
* @param cb Callbacks to register (must not be NULL).
*
* @retval 0 on success.
* @retval -EINVAL if @p cb is NULL.
* @retval -EALREADY if a callback set is already registered.
*/

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment on lines +242 to +247
err = hid_cb->get_report(hid, report_type, size_present, buf, rsp);
if (err < 0) {
LOG_ERR("Get_Report cb err %d", err);
net_buf_unref(rsp);
return err;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep it clear and simple

Comment on lines +74 to +75
#define BT_HID_BUILD_HDR(t, p) \
(uint8_t)(FIELD_PREP(BT_HID_HDR_TYPE_MASK, t) | FIELD_PREP(BT_HID_HDR_PARAM_MASK, p))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment on lines +641 to +647
config BT_HID_DEVICE
bool "Bluetooth HID Device Profile [EXPERIMENTAL]"
select EXPERIMENTAL
select BT_DID
help
This option enables the HID Device profile.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ci and more tester would be pushed in next PR #109124 #109117

@sonarqubecloud

Copy link
Copy Markdown

hid_l2cap_registered = true;
}

hid_cb = cb;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the bt_hid_device_cb::get_report should be checked here because the get_report is mandatory feature.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done add get_report and set_report ( mandatory) as well

Comment on lines +952 to +953
hdr->header = BT_HID_BUILD_HDR(BT_HID_MSG_TYPE_CONTROL,
BT_HID_CONTROL_VIRTUAL_CABLE_UNPLUG);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
hdr->header = BT_HID_BUILD_HDR(BT_HID_MSG_TYPE_CONTROL,
BT_HID_CONTROL_VIRTUAL_CABLE_UNPLUG);
hdr->header = BT_HID_BUILD_HDR(BT_HID_MSG_TYPE_CONTROL,
BT_HID_CONTROL_VIRTUAL_CABLE_UNPLUG);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines +955 to +973
err = bt_l2cap_chan_send(&hid->ctrl_session.br_chan.chan, buf);
if (err < 0) {
net_buf_unref(buf);
return err;
}

/* HID spec v1.1.2 Section 3.1.2.2.3: the VCU initiator shall also
* destroy bonding/Virtual Cable info. This is intentionally left out
* for now for the same reason as in hid_control_handle() — see the
* note there: bt_br_unpair() would drop the shared ACL, and clearing
* the key without an ACL disconnect needs a new host API pending
* community agreement.
*/

/* Delay disconnect to allow the VCU PDU to be transmitted before
* tearing down the L2CAP channel. If the remote host disconnects
* first (spec-correct behavior), cleanup will cancel this timer.
*/
k_work_schedule(&hid->vcu_timeout, HID_VCU_DISCONNECT_TIMEOUT);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe delaying the disconnection is unwise. It should be confirmed that HID_CONTROL message has been sent.
It can be achieved using the function bt_l2cap_br_chan_send_cb().

static void virtual_cable_unplug_tx_cb(struct bt_conn *conn, void *user_data, int err)
{
	struct bt_hid_device *hid;
	
	if (conn == NULL || user_data == NULL) {
		return;
	}

	if (err != 0) {
		LOG_ERR("Failed to send virtual_cable_unplug control message");
		return;
	}

	hid = (struct bt_hid_device *)user_data;
	/* The type of `vcu_timeout` should be changed from `struct k_work_delayable`
	 * to `struct k_work`.
	 */
	k_work_submit(&hid->vcu_timeout.work);
}
Suggested change
err = bt_l2cap_chan_send(&hid->ctrl_session.br_chan.chan, buf);
if (err < 0) {
net_buf_unref(buf);
return err;
}
/* HID spec v1.1.2 Section 3.1.2.2.3: the VCU initiator shall also
* destroy bonding/Virtual Cable info. This is intentionally left out
* for now for the same reason as in hid_control_handle() — see the
* note there: bt_br_unpair() would drop the shared ACL, and clearing
* the key without an ACL disconnect needs a new host API pending
* community agreement.
*/
/* Delay disconnect to allow the VCU PDU to be transmitted before
* tearing down the L2CAP channel. If the remote host disconnects
* first (spec-correct behavior), cleanup will cancel this timer.
*/
k_work_schedule(&hid->vcu_timeout, HID_VCU_DISCONNECT_TIMEOUT);
err = bt_l2cap_br_chan_send_cb(&hid->ctrl_session.br_chan.chan, buf, virtual_cable_unplug_tx_cb, hid);
if (err < 0) {
net_buf_unref(buf);
return err;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replaced the timed delay with bt_l2cap_br_chan_send_cb() so the disconnect only happens after the VCU PDU is actually sent. vcu_timeout (k_work_delayable) is now vcu_disconnect (k_work), and the HID_VCU_DISCONNECT_TIMEOUT magic delay is gone.

One change from your snippet: the tx callback now tears down the link on send failure too (only logging a warning), instead of returning early. VC unplug is a one-way teardown — if we skip the disconnect on error, the device would stay stuck in CONNECTED. The disconnect is still deferred to the workqueue as you suggested.

Comment on lines +552 to +561
/* HID spec v1.1.2 Section 5.2.2: the HID connection is established only
* after both Control and Interrupt channels are open. Report
* transactions other than HID_CONTROL therefore require the fully
* connected state.
*/
if (type != BT_HID_MSG_TYPE_CONTROL && hid->state != BT_HID_STATE_CONNECTED) {
LOG_WRN("transaction 0x%x rejected in state %d", type, hid->state);
hid_send_handshake(hid, BT_HID_HANDSHAKE_RSP_ERR_UNKNOWN);
return 0;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that the description here is inconsistent with the HID v1.1.2 spc. According to the following description,

A Bluetooth HID Host or Bluetooth HID device shall open the Control channel first, then the Interrupt
channel. The Bluetooth HID Host may send Control channel commands once the Control channel has
been successfully opened. The Interrupt channel need not be established in order to send Control
channel commands.

All commands of control channel can be sent after the control channel has been established, regardless of the connection status of the interrupt channel.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove BT_HID_STATE_CONNECTED check

return err;
}

hid_send_handshake(hid, BT_HID_HANDSHAKE_RSP_SUCCESS);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is failed here, shall the connection need to be broken? Or, just report it? Or, ignore it?

Suggested change
hid_send_handshake(hid, BT_HID_HANDSHAKE_RSP_SUCCESS);
err = hid_send_handshake(hid, BT_HID_HANDSHAKE_RSP_SUCCESS);
if (err != 0) {
LOG_ERR("Failed to send handshake (err %d)", err);
return err;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hid_send_handshake() already logs the failure internally via LOG_ERR, so the error is reported. I'd prefer not to break the connection here:

  • A failure is either transient -ENOBUFS (buffer pressure — breaking the link over this would needlessly drop the HID device; the host can retransmit), or -ENOTCONN/-ESHUTDOWN (the link is already going down, so disconnecting again is redundant).
  • This matches how AVRCP/AVCTP handle a failed response send — log it, don't tear down the link.

So my preference is "report it" (already logged), without breaking the connection. I can mark the call sites with (void) to make the intentional ignore explicit if you'd like.

{
uint8_t report_id;

if (buf->len < 1U) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (buf->len < 1U) {
if (buf->len < sizeof(report_id)) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment on lines +398 to +409
static void hid_output_report_cb(struct bt_hid_device *hid, struct net_buf *buf)
{
uint8_t report_id;

if (buf->len < 1U) {
return;
}

report_id = net_buf_pull_u8(buf);
bt_shell_print("HID: output report id %u len %u", report_id, buf->len);
bt_shell_hexdump(buf->data, buf->len);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In current hid report descriptor, no output report exists. I think the an warning should be printed here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment on lines +352 to +354
if (req->len < 1U) {
return -EINVAL;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (req->len < 1U) {
return -EINVAL;
}
if (req->len < sizeof(report_id)) {
return -EINVAL;
}

bt_shell_print("HID: get report type %u id %u", type, report_id);

if (size_present) {
if (req->len < 2U) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (req->len < 2U) {
if (req->len < sizeof(uint16_t)) {

{
uint8_t report_id;

if (buf->len < 1U) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (buf->len < 1U) {
if (buf->len < sizeof(report_id)) {

Comment on lines +36 to +43
/** @brief HID handshake response codes (4-bit param field). */
#define BT_HID_HANDSHAKE_RSP_SUCCESS 0x00
#define BT_HID_HANDSHAKE_RSP_NOT_READY 0x01
#define BT_HID_HANDSHAKE_RSP_ERR_INVALID_REP_ID 0x02
#define BT_HID_HANDSHAKE_RSP_ERR_UNSUPPORTED_REQ 0x03
#define BT_HID_HANDSHAKE_RSP_ERR_INVALID_PARAM 0x04
#define BT_HID_HANDSHAKE_RSP_ERR_UNKNOWN 0x0e
#define BT_HID_HANDSHAKE_RSP_ERR_FATAL 0x0f

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** @brief HID handshake response codes (4-bit param field). */
#define BT_HID_HANDSHAKE_RSP_SUCCESS 0x00
#define BT_HID_HANDSHAKE_RSP_NOT_READY 0x01
#define BT_HID_HANDSHAKE_RSP_ERR_INVALID_REP_ID 0x02
#define BT_HID_HANDSHAKE_RSP_ERR_UNSUPPORTED_REQ 0x03
#define BT_HID_HANDSHAKE_RSP_ERR_INVALID_PARAM 0x04
#define BT_HID_HANDSHAKE_RSP_ERR_UNKNOWN 0x0e
#define BT_HID_HANDSHAKE_RSP_ERR_FATAL 0x0f
/** @brief HID handshake result codes (4-bit param field). */
#define BT_HID_HS_RSP_SUCCESS 0x00
#define BT_HID_HS_RSP_NOT_READY 0x01
#define BT_HID_HS_RSP_ERR_INVALID_REPORT_ID 0x02
#define BT_HID_HS_RSP_ERR_UNSUPPORTED_REQ 0x03
#define BT_HID_HS_RSP_ERR_INVALID_PARAM 0x04
#define BT_HID_HS_RSP_ERR_UNKNOWN 0x0e
#define BT_HID_HS_RSP_ERR_FATAL 0x0f

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

udpated

Comment on lines +11 to +12
#ifndef ZEPHYR_INCLUDE_BLUETOOTH_HID_DEVICE_H_
#define ZEPHYR_INCLUDE_BLUETOOTH_HID_DEVICE_H_

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#ifndef ZEPHYR_INCLUDE_BLUETOOTH_HID_DEVICE_H_
#define ZEPHYR_INCLUDE_BLUETOOTH_HID_DEVICE_H_
#ifndef ZEPHYR_INCLUDE_BLUETOOTH_CLASSIC_HID_DEVICE_H_
#define ZEPHYR_INCLUDE_BLUETOOTH_CLASSIC_HID_DEVICE_H_

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment on lines +402 to +410

err = bt_l2cap_chan_connect(hid->ctrl_session.br_chan.chan.conn,
&hid->intr_session.br_chan.chan, BT_L2CAP_PSM_HID_INT);
if (err != 0) {
LOG_ERR("INTR connect failed");
hid->state = BT_HID_STATE_DISCONNECTING;
bt_l2cap_chan_disconnect(&hid->ctrl_session.br_chan.chan);
return;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The status should be updated to BT_HID_STATE_INTR_CONNECTING.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment on lines +452 to +460
/* Clear our own conn up-front so the slot is observably free the moment
* the HID state reaches DISCONNECTED. L2CAP clears chan->conn only after
* this callback returns (l2cap_br.c, bt_l2cap_br_chan_del()), so without
* this the DISCONNECTED state would briefly coexist with a non-NULL CTRL
* conn. Same pattern as AVDTP (avdtp.c) and AVCTP (avctp.c). The INTR
* coordination below reads the *peer* channel's conn, so clearing ours
* here does not affect it.
*/
chan->conn = NULL;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same pattern as AVDTP (avdtp.c) and AVCTP (avctp.c)

I think the conn of chan should not be touched by upper layer. It is maintained by the l2cap.
Everything you mentioned needs to be corrected.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added ctrl_connected / intr_connected flags owned by the profile, set in the L2CAP connected callbacks and cleared in the disconnected callbacks. The CTRL/INTR teardown coordination now reads those flags instead of chan->conn.

static void bt_hid_l2cap_intr_connected(struct bt_l2cap_chan *chan)
{
struct bt_hid_device *hid = HID_DEVICE_BY_INTR_CHAN(chan);
enum bt_hid_channel_type chtype;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
enum bt_hid_channel_type chtype;
__maybe_unused enum bt_hid_channel_type chtype;

static void bt_hid_l2cap_ctrl_disconnected(struct bt_l2cap_chan *chan)
{
struct bt_hid_device *hid = HID_DEVICE_BY_CTRL_CHAN(chan);
enum bt_hid_channel_type chtype;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
enum bt_hid_channel_type chtype;
__maybe_unused enum bt_hid_channel_type chtype;

static void bt_hid_l2cap_intr_disconnected(struct bt_l2cap_chan *chan)
{
struct bt_hid_device *hid = HID_DEVICE_BY_INTR_CHAN(chan);
enum bt_hid_channel_type chtype;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix it in similar places.

Suggested change
enum bt_hid_channel_type chtype;
__maybe_unused enum bt_hid_channel_type chtype;


if (buf->len < sizeof(*hdr)) {
LOG_ERR("CTRL buf len %u invalid", buf->len);
hid_send_handshake(hid, BT_HID_HANDSHAKE_RSP_ERR_INVALID_PARAM);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return code of the function hid_send_handshake() should be checked.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same rationale as the Set_Report handshake above: hid_send_handshake() already logs the failure internally, and I'd prefer not to act on it here.
A failure is either transient -ENOBUFS or -ENOTCONN/-ESHUTDOWN (link already going down) — neither warrants tearing down the connection, and this is an error path already (sending an error handshake), so there's no meaningful recovery.

Comment on lines +530 to +551
static int cmd_hid_reconnect(const struct shell *sh, size_t argc, char *argv[])
{
int err;

if (!hid_registered) {
shell_error(sh, "HID: not registered");
return -ENOEXEC;
}

if (default_conn == NULL) {
shell_error(sh, "HID: not connected");
return -ENOEXEC;
}

err = bt_hid_device_connect(default_conn, &default_hid);
if (err != 0) {
shell_error(sh, "HID: reconnect failed (%d)", err);
return -ENOEXEC;
}

return 0;
}

@lylezhu2012 lylezhu2012 Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is different between this commander and connect?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove. this represents a device-initiated reconnect; in the current sample, it is simplified to reuse the connect operation.


rsp = bt_hid_device_create_pdu(NULL);
if (rsp == NULL) {
return -ENOBUFS;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return -ENOBUFS;
return -EAGAIN;

return err;
}

hid_send_handshake(hid, BT_HID_HS_RSP_SUCCESS);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
hid_send_handshake(hid, BT_HID_HS_RSP_SUCCESS);
err = hid_send_handshake(hid, BT_HID_HS_RSP_SUCCESS);
if (err != 0) {
LOG_ERR("Failed to send handshake (err %d)", err);
}

Or if you want to ignore the error code of the function hid_send_handshake(), it is better to change the prototype of the function hid_send_handshake() to:

static void hid_send_handshake(struct bt_hid_device *hid, uint8_t result_code);

Besides, consider adding a dedicated buffer pool for handshake packets to avoid buffer allocation failures.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Changed hid_send_handshake() to void and added a dedicated 2-buffer pool (hid_hs_pool) for handshake PDUs, matching the pattern used by AVDTP/SDP/L2CAP signaling. This ensures handshake responses are
never starved by data traffic.

}

hid->boot_mode = (protocol == BT_HID_PROTOCOL_BOOT_MODE);
hid_send_handshake(hid, BT_HID_HS_RSP_SUCCESS);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

if (hid->state != BT_HID_STATE_CONNECTED &&
hid->state != BT_HID_STATE_DISCONNECTED &&
hid->state != BT_HID_STATE_DISCONNECTING) {
bt_hid_device_disconnect(hid);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto. The return code should be handled.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines +357 to +358
hid->state != BT_HID_STATE_DISCONNECTED &&
hid->state != BT_HID_STATE_DISCONNECTING) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the state is changed to BT_HID_STATE_CONNECTED or BT_HID_STATE_DISCONNECTED, the delayable worker should be cancelled. So the function hid_intr_timeout_handler() is called, the control channel should be disconnected directly.

Why is it necessary to check the state here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplified both handlers: flipped the condition to an early-return on terminal states (DISCONNECTED || DISCONNECTING) which is clearer, and now log the bt_hid_device_disconnect() return value.

Comment on lines +369 to +372
if (hid->state != BT_HID_STATE_DISCONNECTED &&
hid->state != BT_HID_STATE_DISCONNECTING) {
bt_hid_device_disconnect(hid);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it necessary to perform such a state check?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guard is needed because this work item runs asynchronously — between k_work_submit() in the TX callback and actual execution on the workqueue, the remote may have already disconnected (triggering cleanup via the L2CAP disconnected callbacks).

Without the check, bt_hid_device_disconnect() would be called on an already-torn-down instance and return -ENOTCONN. Now simplified to an early-return on terminal states and the return value is logged.

Comment on lines +375 to +396
static void virtual_cable_unplug_tx_cb(struct bt_conn *conn, void *user_data, int err)
{
struct bt_hid_device *hid;

if (conn == NULL || user_data == NULL) {
return;
}

hid = (struct bt_hid_device *)user_data;

/* VC unplug is a one-way teardown of the HID connection: disconnect
* regardless of the send result. If the PDU failed to reach the
* controller the host may not see the unplug, but the local link must
* still be torn down. Defer the disconnect to the system workqueue
* because this callback runs in the TX context.
*/
if (err != 0) {
LOG_WRN("VCU control message send failed (%d)", err);
}

k_work_submit(&hid->vcu_disconnect);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function virtual_cable_unplug_tx_cb() should ideally be placed close to where it is called.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines +32 to +33
#define BT_HID_PROTO_CONTROL 0x0011
#define BT_HID_PROTO_INTERRUPT 0x0013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#define BT_HID_PROTO_CONTROL 0x0011
#define BT_HID_PROTO_INTERRUPT 0x0013
#define BT_L2CAP_PSM_HID_CONTROL 0x0011
#define BT_L2CAP_PSM_HID_INTERRUPT 0x0013

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kept PSM defines local to the shell file (renamed to BT_L2CAP_PSM_HID_CONTROL/INTERRUPT). These are only needed for SDP record construction in the shell sample. Following the same pattern as other profiles
(AVDTP, AVRCP, SDP) where PSM values live in *_internal.h or locally in .c files — not in public API headers, since applications use profile-level connect APIs rather than raw PSM values.

Comment on lines +308 to +320
protocol = FIELD_GET(BT_HID_PROTOCOL_MASK, param);

if ((hid_cb != NULL) && (hid_cb->set_protocol != NULL)) {
err = hid_cb->set_protocol(hid, protocol);
if (err != 0) {
return err;
}
}

hid->boot_mode = (protocol == BT_HID_PROTOCOL_BOOT_MODE);
hid_send_handshake(hid, BT_HID_HS_RSP_SUCCESS);

return 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
protocol = FIELD_GET(BT_HID_PROTOCOL_MASK, param);
if ((hid_cb != NULL) && (hid_cb->set_protocol != NULL)) {
err = hid_cb->set_protocol(hid, protocol);
if (err != 0) {
return err;
}
}
hid->boot_mode = (protocol == BT_HID_PROTOCOL_BOOT_MODE);
hid_send_handshake(hid, BT_HID_HS_RSP_SUCCESS);
return 0;
protocol = FIELD_GET(BT_HID_PROTOCOL_MASK, param);
if ((hid_cb == NULL) || (hid_cb->set_protocol == NULL)) {
return -EOPNOTSUPP;
}
err = hid_cb->set_protocol(hid, protocol);
if (err != 0) {
return err;
}
hid->boot_mode = (protocol == BT_HID_PROTOCOL_BOOT_MODE);
hid_send_handshake(hid, BT_HID_HS_RSP_SUCCESS);
return 0;

Comment on lines +334 to +338
if (err != 0) {
hid_send_handshake(&hid->ctrl_session,
BT_HID_HANDSHAKE_RSP_ERR_INVALID_PARAM);
return err;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not find it.

@lylezhu2012 lylezhu2012 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that the sign off Assisted-by: Claude Opus 4.6 (1M context) has been removed from the message of all commits.

static void bt_hid_session_init(struct bt_hid_device *hid, enum bt_hid_role role)
{
hid->ctrl_session.br_chan.chan.ops = &ctrl_ops;
hid->ctrl_session.br_chan.rx.mtu = BT_HID_RX_MTU;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HID header should be also a part of MTU.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove BT_HID_RX_MTU

return -ENOEXEC;
}

bt_hid_device_disconnect(default_hid);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return code of the function bt_hid_device_disconnect() should be checked here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment on lines +519 to +523
/* Wait for remote to disconnect CTRL. */
if (hid->ctrl_connected) {
LOG_DBG("wait for remote CTRL disconnect");
return;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the interrupt channel disconnection is performed by peer device, and no control channel disconnection request anymore, the state of HID device is always BT_HID_STATE_CONNECTED.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The state should be updated to reflect the actual situation.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add BT_HID_STATE_CTRL_CONNECTED state

Comment on lines +362 to +371
/* Timer is started when CTRL connects and cancelled when INTR connects
* or when cleanup runs. If this fires, the HID connection did not
* complete in time — tear down whatever is up. The state guard handles
* the race where remote disconnected between work queue scheduling and
* execution.
*/
if (hid->state == BT_HID_STATE_DISCONNECTED ||
hid->state == BT_HID_STATE_DISCONNECTING) {
return;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition is also be checked in function bt_hid_device_disconnect().

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

Comment on lines +834 to +841
err = bt_l2cap_chan_connect(conn, &inst->ctrl_session.br_chan.chan, BT_L2CAP_PSM_HID_CTRL);
if (err != 0) {
LOG_ERR("connect failed (%d)", err);
bt_hid_device_cleanup(inst);
return err;
}

inst->state = BT_HID_STATE_CTRL_CONNECTING;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The state will be cleared by function bt_hid_device_cleanup() if the err is not zero.

Suggested change
err = bt_l2cap_chan_connect(conn, &inst->ctrl_session.br_chan.chan, BT_L2CAP_PSM_HID_CTRL);
if (err != 0) {
LOG_ERR("connect failed (%d)", err);
bt_hid_device_cleanup(inst);
return err;
}
inst->state = BT_HID_STATE_CTRL_CONNECTING;
inst->state = BT_HID_STATE_CTRL_CONNECTING;
err = bt_l2cap_chan_connect(conn, &inst->ctrl_session.br_chan.chan, BT_L2CAP_PSM_HID_CTRL);
if (err != 0) {
LOG_ERR("connect failed (%d)", err);
bt_hid_device_cleanup(inst);
return err;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment thread subsys/bluetooth/host/classic/shell/hid_device.c
Comment thread subsys/bluetooth/host/classic/hid_device.c
Comment thread subsys/bluetooth/host/classic/hid_device.c
Comment thread subsys/bluetooth/host/classic/hid_device.c
Comment thread subsys/bluetooth/host/classic/hid_device.c Outdated
@makeshi

makeshi commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

@chengkai15 The PR looks good for me, but the sanity check doesn't seem to pass. Could you rebase it with the latest Zephyr code?

@chengkai15

Copy link
Copy Markdown
Member Author

@chengkai15 The PR looks good for me, but the sanity check doesn't seem to pass. Could you rebase it with the latest Zephyr code?

Rebased onto the latest main @makeshi @lylezhu2012 Could you please take another look when you get a chance?

makeshi
makeshi previously approved these changes Jul 29, 2026

@MarkWangChinese MarkWangChinese left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chengkai15 I just have two questions, please help to clarify them. Apologies if I've missed something.

Comment on lines +277 to +278
* @note The returned buffer may be fragmented. Use net_buf_frag_add()
* to append data.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please help to explain the case that net_buf_frag_add is used? From the codes, I can't see how it is used.

static bool hid_l2cap_registered;

/* HID device connection (single session only) */
static struct bt_hid_device hid_device_conn;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it limited to supporting only one instance? Should it instead be:

Suggested change
static struct bt_hid_device hid_device_conn;
static struct bt_hid_device hid_device_conns[CONFIG_BT_MAX_CONN];

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The single-instance design is intentional for HID Device role:

  1. HID spec requires remembering multiple HID hosts for reconnection, but does NOT require HID device simultaneous active connections to multiple hosts.

  2. Major implementations use single-instance for HID Device side:

    • Android BluetoothHidDevice: single active app registration
    • No BR/EDR HID product ships with simultaneous multi-host
  3. The Virtual Cable abstraction implies one-cable-one-host active connection. Multi-host "switching" keyboards
    store multiple bond keys but only maintain one active L2CAP session.

  4. If multi-instance support is desired in the future, a dedicated CONFIG_BT_HID_DEVICE_MAX_CONN (not CONFIG_BT_MAX_CONN) could be added. and enhance the handle

Add internal header with HID protocol constants, message types,
handshake codes, header encoding macros, and session/device structures
per HID Profile v1.1.2.

Add HID-specific SDP attribute definitions (0x0200-0x0210) to the
public SDP header.

Signed-off-by: Kai Cheng <chengkai@xiaomi.com>
Assisted-by: Claude Opus 4.6 (1M context)
Define the public API for Bluetooth Classic HID Device profile:
- Protocol mode and report type constants
- Opaque bt_hid_device forward declaration
- Callback structure (connected, disconnected, set_report, get_report,
  set_protocol, output_report, vc_unplug, suspend)
- Registration, connect, disconnect, PDU creation, get_report_rsp,
  input_report, and virtual_cable_unplug APIs

Key design decisions:
- Driver does not parse report_id; raw buf passed to callbacks
- set_protocol returns int; driver sends handshake based on return value
- Caller owns buf on error (callee never frees)
- Struct bt_hid_device is opaque (forward declaration only)

Signed-off-by: Kai Cheng <chengkai@xiaomi.com>
Assisted-by: Claude Opus 4.6 (1M context)
Implement the HID Device profile core:
- L2CAP server registration for CTRL (PSM 0x0011) and
  INTR (PSM 0x0013)
- Connection state machine
  (DISCONNECTED -> CTRL -> INTR -> CONNECTED)
- Control channel message dispatch (GET_REPORT, SET_REPORT,
  GET_PROTOCOL, SET_PROTOCOL, HID_CONTROL)
- Interrupt channel output report handling
- Handshake response generation based on handler return codes
- Virtual Cable Unplug, disconnecting once the PDU has been sent
- Buffer ownership: callee never frees caller-provided buf
- unregister returns -EBUSY if still connected

Signed-off-by: Kai Cheng <chengkai@xiaomi.com>
Assisted-by: Claude Opus 4.6 (1M context)
Add shell commands for interactive HID Device testing:
- hid_device register/unregister: register HID callbacks and SDP record
- hid_device connect/disconnect: manage HID association
- hid_device send: send mouse input reports (button, dx, dy, wheel)

The shell module uses HID report descriptor macros from
<zephyr/usb/class/hid.h> for a mouse with 8 buttons, X/Y axes,
and scroll wheel (Report ID 2).

Callbacks parse report_id from buf locally (driver passes raw buf).
Protocol mode tracked via set_protocol callback return value.

Signed-off-by: Kai Cheng <chengkai@xiaomi.com>
Assisted-by: Claude Opus 4.6 (1M context)
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Bluetooth Classic Bluetooth Classic (BR/EDR) area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth area: Tests Issues related to a particular existing or missing test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants