Bluetooth: Classic: AVRCP: Support vendor dependent cmd #96273

makeshi · 2025-09-19T09:39:19Z

This patch introduces support for AVRCP vendor-dependent commands and responses, including full handling of fragmented messages.

MarkWangChinese

Only reviewed part of the codes.

MarkWangChinese · 2025-09-23T09:46:16Z

subsys/bluetooth/host/classic/avrcp_internal.h

 	uint8_t opcode; /**< Unit Info, Subunit Info, Vendor Dependent, or Pass Through */
 } __packed;

 struct bt_avrcp_avc_pdu {


From the spec, the AV/C commands contain UNIT INFO, SUBUNIT INFO, VENDOR DEPENDENT and PASS THROUGH. I think this struct is only for VENDOR DEPENDENT.
The struct name seems not reasonable.

And the follow struct bt_avrcp_avc_brow_pdu, browse cmd seems not belong to AV/C command from spec.

MarkWangChinese · 2025-09-23T09:50:58Z

subsys/bluetooth/host/classic/avrcp.c

+				  sizeof(struct bt_l2cap_hdr) +
+				  sizeof(struct bt_avctp_header_start) +
+				  sizeof(struct bt_avrcp_header) +
+				  sizeof(struct bt_avrcp_avc_pdu));


From I see, the bt_avrcp_avc_pdu is for vendor dependent msg. Is bt_avrcp_create_pdu only for vendor dependent msg buf creation?

Currently all bt_avrcp_avc_pdu for all AV command buffer alloc included browing buffer, the API's message is the AVRCP payload, so I need to reserve space for the maximum possible AVRCP header inside it, the sizeof(struct bt_avrcp_header) + sizeof(struct bt_avrcp_avc_pdu) is maximum possible AVRCP header.

Do you consider to divide bt_avrcp_avc_pdu() into two part. On part for pass through. Another part for vendor independent?

MarkWangChinese · 2025-09-23T12:40:21Z

include/zephyr/bluetooth/classic/avrcp.h

 	 *             @ref bt_avrcp_set_browsed_player_rsp. Note that the data is encoded in
 	 *             big-endian format.
 	 */
+


meaningless blank line.

MarkWangChinese · 2025-09-23T12:46:49Z

include/zephyr/bluetooth/classic/avrcp.h

+	 *  @param tid The transaction label of the request.
+	 *  @param cap_id The capability ID requested.
+	 */
+	void (*get_cap_req)(struct bt_avrcp_tg *tg, uint8_t tid, uint8_t cap_id);


why the get_caps_rsp uses caps?

I want to standardize the naming of this API so that it's easy to trace from the spec document → PDU ID → command/response buffer name → API and callback.
The reason I renamed it to caps is because of the BT_AVRCP_PDU_ID_GET_CAPS.

MarkWangChinese · 2025-09-23T13:13:00Z

subsys/bluetooth/host/classic/avrcp_internal.h

+	uint16_t received_len;			/**< Length already received */
+};
+
+struct bt_avrcp_tg_tx {


From my see, the struct is only used for sending the vendor dependent response, suggest to change as bt_avrcp_tg_vd_rsp_tx

MarkWangChinese · 2025-09-23T13:14:30Z

subsys/bluetooth/host/classic/avrcp.c

+	struct k_sem lock;
+
+	/* TX work */
+	struct k_work_delayable tx_work;


From my see, it is only used for the vendor dependent response. Suggest to change the name to indicate the usage.

gzh-terry · 2025-09-24T04:15:26Z

include/zephyr/bluetooth/classic/avrcp.h

+	 *
+	 * @note This is an AV/C Response semantics (not an AVRCP Error Status).
+	 *       When the CT-side callback reports REJECTED, the application MUST parse
+	 *       the net buffer to extract the 1-octet Error Status from the PDU payload.


Can we follow the spec definitions?
BTW, the range of these responses should be in 0x8-0xf

I'm trying to extend the status field. I want to append the response codes (ranging from 0x8 to 0xF) to the end of the status, starting from 0xFF bt_avrcp_rsp_t.
The response definitions will still remain as they are. pls see bt_avrcp_rsp_t
Now I'm facing a situation where a vendor-dependent Transaction Group (TG) reply requires both a response code and a status. For example, when the TG replies with an AV/C REJECTED response, it also needs to include a reason in the status.
At the same time, I feel that keeping both the response code and the status for the application might introduce redundant information, making the function somewhat awkward.
take example, if not extend the status field, we need
int bt_avrcp_tg_send_add_to_now_playing_rsp(struct bt_avrcp_tg *tg, uint8_t tid, uint8_t rsp_code, uint8_t status);
If we the status field, application only need take care the status, then stack will parse it to rsp_code and status.

detail see spec:
6.15.3 Status and error codes
The responses of AVRCP Specific Browsing Commands contain a status field to indicate success/failure
and an error status code is added to the AV/C REJECTED response if the TG rejected an AVRCP Specific AV/C command.
It is useful for CT to know why the command was rejected by the TG. Table 6.49 shows the status and error codes to
be used with both AVRCP Specific Browsing Commands and AVRCP Specific AV/C Commands.

lylezhu2012 · 2025-09-29T04:05:29Z

include/zephyr/bluetooth/classic/avrcp.h

+/** @brief AVRCP Media Attribute structure */
+struct bt_avrcp_media_attr {
+	uint32_t attr_id;    /**< Media attribute ID, see @ref bt_avrcp_media_attr_t */
+	uint16_t charset_id; /**< Character set ID, see @ref bt_avrcp_charset_t */
+	uint16_t attr_len;   /**< Length of attribute value */
+	uint8_t attr_val[];  /**< Attribute value data */
+} __packed;
+
+/** @brief GetElementAttributes command request structure */
+struct bt_avrcp_get_element_attrs_cmd {
+	uint8_t identifier[8]; /**< Element identifier (0x0 for currently playing) */
+	uint8_t num_attrs;     /**< Number of attributes requested (0 = all) */
+	uint32_t attr_ids[];   /**< Array of requested attribute IDs */
+} __packed;


Suggested change

/** @brief AVRCP Media Attribute structure */

struct bt_avrcp_media_attr {

uint32_t attr_id; /**< Media attribute ID, see @ref bt_avrcp_media_attr_t */

uint16_t charset_id; /**< Character set ID, see @ref bt_avrcp_charset_t */

uint16_t attr_len; /**< Length of attribute value */

uint8_t attr_val[]; /**< Attribute value data */

} __packed;

/** @brief GetElementAttributes command request structure */

struct bt_avrcp_get_element_attrs_cmd {

uint8_t identifier[8]; /**< Element identifier (0x0 for currently playing) */

uint8_t num_attrs; /**< Number of attributes requested (0 = all) */

uint32_t attr_ids[]; /**< Array of requested attribute IDs */

} __packed;

/** @brief GetElementAttributes command request structure */

struct bt_avrcp_get_element_attrs_cmd {

uint8_t identifier[8]; /**< Element identifier (0x0 for currently playing) */

uint8_t num_attrs; /**< Number of attributes requested (0 = all) */

uint32_t attr_ids[]; /**< Array of requested attribute IDs */

} __packed

/** @brief AVRCP Media Attribute structure */

struct bt_avrcp_media_attr {

uint32_t attr_id; /**< Media attribute ID, see @ref bt_avrcp_media_attr_t */

uint16_t charset_id; /**< Character set ID, see @ref bt_avrcp_charset_t */

uint16_t attr_len; /**< Length of attribute value */

uint8_t attr_val[]; /**< Attribute value data */

} __packed;

lylezhu2012 · 2025-09-29T04:06:00Z

include/zephyr/bluetooth/classic/avrcp.h

+struct bt_avrcp_get_play_status_rsp {
+	uint32_t song_length;    /**< Total length of the song in milliseconds */
+	uint32_t song_position;  /**< Current position in the song in milliseconds */
+	uint8_t play_status;     /**< Play status: @ref bt_avrcp_playback_status_t*/


Suggested change

uint8_t play_status; /**< Play status: @ref bt_avrcp_playback_status_t*/

uint8_t play_status; /**< Play status: @ref bt_avrcp_playback_status_t */

lylezhu2012 · 2025-09-29T04:07:14Z

include/zephyr/bluetooth/classic/avrcp.h

+
+/** @brief RegisterNotification command request */
+struct bt_avrcp_register_notification_cmd {
+    uint8_t event_id;        /**< Event ID to register for */


Where is the event ID defined?

lylezhu2012 · 2025-09-29T05:38:51Z

subsys/bluetooth/host/classic/avrcp.c

+	{  BT_AVRCP_PDU_ID_GET_CAPS                          , BT_AVRCP_CTYPE_STATUS },
+	{  BT_AVRCP_PDU_ID_LIST_PLAYER_APP_SETTING_ATTRS     , BT_AVRCP_CTYPE_STATUS },
+	{  BT_AVRCP_PDU_ID_LIST_PLAYER_APP_SETTING_VALS      , BT_AVRCP_CTYPE_STATUS},
+	{  BT_AVRCP_PDU_ID_GET_CURR_PLAYER_APP_SETTING_VAL   , BT_AVRCP_CTYPE_STATUS},
+	{  BT_AVRCP_PDU_ID_SET_PLAYER_APP_SETTING_VAL        , BT_AVRCP_CTYPE_CONTROL},
+	{  BT_AVRCP_PDU_ID_GET_PLAYER_APP_SETTING_ATTR_TEXT  , BT_AVRCP_CTYPE_STATUS},
+	{  BT_AVRCP_PDU_ID_GET_PLAYER_APP_SETTING_VAL_TEXT   , BT_AVRCP_CTYPE_STATUS},
+	{  BT_AVRCP_PDU_ID_INFORM_DISPLAYABLE_CHAR_SET	     , BT_AVRCP_CTYPE_CONTROL},
+	{  BT_AVRCP_PDU_ID_INFORM_BATT_STATUS_OF_CT	     , BT_AVRCP_CTYPE_CONTROL},
+	{  BT_AVRCP_PDU_ID_GET_ELEMENT_ATTRS                 , BT_AVRCP_CTYPE_STATUS},
+	{  BT_AVRCP_PDU_ID_GET_PLAY_STATUS                   , BT_AVRCP_CTYPE_STATUS},
+	{  BT_AVRCP_PDU_ID_REGISTER_NOTIFICATION	     , BT_AVRCP_CTYPE_NOTIFY},
+	{  BT_AVRCP_PDU_ID_SET_ABSOLUTE_VOLUME               , BT_AVRCP_CTYPE_CONTROL},
+	{  BT_AVRCP_PDU_ID_SET_ADDRESSED_PLAYER              , BT_AVRCP_CTYPE_CONTROL},
+	{  BT_AVRCP_PDU_ID_PLAY_ITEM                         , BT_AVRCP_CTYPE_CONTROL},
+	{  BT_AVRCP_PDU_ID_ADD_TO_NOW_PLAYING                , BT_AVRCP_CTYPE_CONTROL},
+	{  BT_AVRCP_PDU_ID_REQ_CONTINUING_RSP                , BT_AVRCP_CTYPE_CONTROL},
+	{  BT_AVRCP_PDU_ID_ABORT_CONTINUING_RSP              , BT_AVRCP_CTYPE_CONTROL},


Some lines have space before the brace, and others do not.

lylezhu2012 · 2025-09-29T05:48:03Z

subsys/bluetooth/host/classic/avrcp.c

+struct avrcp_pdu_vendor_handler  {
+	bt_avrcp_pdu_id_t pdu_id;
+	uint8_t min_len;
+	void (*func)(struct bt_avrcp *avrcp, uint8_t tid, uint8_t result, struct net_buf *buf);
+};
+
+typedef struct {
+	uint8_t pdu_id;
+	bt_avrcp_ctype_t cmd_type;
+} bt_avrcp_pdu_cmd_map_t;


Do you consider that these two structures can be merged into one?

lylezhu2012 · 2025-09-29T10:51:12Z

include/zephyr/bluetooth/classic/avrcp.h

+	BT_AVRCP_STATUS_REJECTED = 0xf8,
+
+	/** Interim response.
+	 * Indicates a temporary state; typically used for @ref BT_AVRCP_CTYPE_NOTIFY
+	 * (e.g. RegisterNotification) to signal that a future CHANGED/STABLE
+	 * response will follow.
+	 */
+	BT_AVRCP_STATUS_INTERIM = 0xf9,
+
+	/** Changed response.
+	 * Indicates that the notified value has changed and a final result is provided.
+	 * @note AV/C Response code (not an Error Status).
+	 */
+	BT_AVRCP_STATUS_CHANGED = 0xfa,
+
+	/** Implemented response
+	 * - As IMPLEMENTED: used for SPECIFIC_INQUIRY and GENERAL_INQUIRY commands.
+	 * - As STABLE: used for STATUS commands to indicate a stable current state.
+	 */
+	BT_AVRCP_STATUS_IMPLEMENTED = 0xfb,
+
+	/** Stable response.
+	 * Alias of @ref BT_AVRCP_RSP_IMPLEMENTED for STATUS commands.
+	 */
+	BT_AVRCP_STATUS_STABLE = 0xfc,
+
+	/** In transition response.
+	 * The target is currently changing state (e.g., between play/pause).
+	 */
+	BT_AVRCP_STATUS_IN_TRANSITION = 0xfd,
+
+	/** Accepted response.
+	 * The command has been accepted and will be processed; a later response
+	 * (e.g., CHANGED/STABLE) may follow depending on the PDU.
+	 */
+	BT_AVRCP_STATUS_ACCEPTED = 0xfe,
+
+	/** Not implemented response.
+	 * The command/PDU is not supported by the target device.
+	 */
+	BT_AVRCP_STATUS_NOT_IMPLEMENTED = 0xff,


I think there are four types of status,

success -> For control, it means accepted. For status, it means stable.

in transition -> only for status, it means TG is busy.

not implemented -> It means the function is not implemented, such as the callback is not implemented, or the feature is not enabled.

Any other value of bt_avrcp_status_t. It means it is a rejected response. The value of bt_avrcp_status_t is the reason.

So, I think it can be optimised with these four types of response.

lylezhu2012 · 2025-09-29T10:57:32Z

subsys/bluetooth/host/classic/avrcp.c

+	uint8_t status;

-	switch (rsp->cap_id) {
-	case BT_AVRCP_CAP_COMPANY_ID:
-		expected_len = rsp->cap_cnt * BT_AVRCP_COMPANY_ID_SIZE;
-		break;
-	case BT_AVRCP_CAP_EVENTS_SUPPORTED:
-		expected_len = rsp->cap_cnt;
-		break;
-	default:
-		LOG_ERR("Unrecognized capability = 0x%x", rsp->cap_id);
-		return;
-	}
+	status = bt_avrcp_rsp_to_status(rsp_code, BT_AVRCP_STATUS_REJECTED);

-	if (buf->len < sizeof(*rsp) + expected_len) {
-		LOG_ERR("Invalid capability payload length: %d", buf->len);
+	if ((avrcp_ct_cb == NULL) || (avrcp_ct_cb->get_caps_rsp == NULL)) {
 		return;
 	}

-	avrcp_ct_cb->get_caps_rsp(get_avrcp_ct(avrcp), tid, rsp);
+	avrcp_ct_cb->get_caps_rsp(get_avrcp_ct(avrcp), tid, status, buf);


Why the checking of received data length is removed?

lylezhu2012 · 2025-09-29T11:09:04Z

subsys/bluetooth/host/classic/avrcp.c

+	if ((avrcp_tg_cb == NULL) || (avrcp_tg_cb->set_absolute_volume_req == NULL)) {
+		error_code = BT_AVRCP_STATUS_INTERNAL_ERROR;


If the error code should be not implemented?

Same comments for other commands.

lylezhu2012 · 2025-09-29T11:13:49Z

subsys/bluetooth/host/classic/avrcp.c

+	return err;
+}
+
+int bt_avrcp_ct_get_cap(struct bt_avrcp_ct *ct, uint8_t tid, uint8_t cap_id)


If the rsp callback should be checked here? Since the request is not always successful, if the get cap rsp callback is NULL, what is the purpose of the request?

Same comments for other commands.

lylezhu2012 · 2025-09-29T11:16:42Z

subsys/bluetooth/host/classic/avrcp.c

+	buf = bt_avrcp_create_pdu(&avrcp_pool);
+	if (buf == NULL) {
+		LOG_ERR("Failed to allocate buffer");
+		return -ENOMEM;


Suggested change

return -ENOMEM;

return -ENOBUFS;

Please check the same case in other functions.

Updated the check to use buf->len to ensure the buffer contains enough data for the requested chunk size. Signed-off-by: Make Shi <[email protected]>

This patch introduces support for AVRCP vendor-dependent commands and responses, including full handling of fragmented messages. - Adds fragmentation and reassembly logic for AVRCP vendor-dependent - Introduces TX queue management using delayed work for TG - Adds support for GetCapabilities PDUs - Add new Kconfig for vendor-dependent with fragmentation support Signed-off-by: Make Shi <[email protected]>

This patch adds AVRCP notification event handling for both CT and TG roles. Also add Shell command support for testing notification registration and responses. Signed-off-by: Make Shi <[email protected]>

- Added new callback APIs in bt_avrcp_ct_cb and bt_avrcp_tg_cb - Implemented command/response handlers for CT and TG roles - Extended shell commands for testing vendor dependent commands Signed-off-by: Make Shi <[email protected]>

sonarqubecloud · 2025-10-13T10:35:16Z

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
4.1% Duplication on New Code

See analysis details on SonarQube Cloud

zephyrbot added area: Bluetooth Classic Bluetooth Classic (BR/EDR) area: Bluetooth labels Sep 19, 2025

zephyrbot requested review from MarkWangChinese, gzh-terry, jhedberg and lylezhu2012 September 19, 2025 09:40

zephyrbot assigned lylezhu2012 Sep 19, 2025

makeshi force-pushed the br_avrcp_vendor_dependent_cmd branch 10 times, most recently from 7251c67 to 54ff174 Compare September 23, 2025 12:34

MarkWangChinese reviewed Sep 23, 2025

View reviewed changes

gzh-terry reviewed Sep 24, 2025

View reviewed changes

makeshi force-pushed the br_avrcp_vendor_dependent_cmd branch 3 times, most recently from a665fac to dcdbd31 Compare September 26, 2025 05:40

makeshi requested review from MarkWangChinese and gzh-terry September 26, 2025 07:35

lylezhu2012 changed the title ~~Br avrcp vendor dependent cmd~~ Bluetooth: Classic: AVRCP: Br avrcp vendor dependent cmd Sep 29, 2025

lylezhu2012 changed the title ~~Bluetooth: Classic: AVRCP: Br avrcp vendor dependent cmd~~ Bluetooth: Classic: AVRCP: Support vendor dependent cmd Sep 29, 2025

makeshi force-pushed the br_avrcp_vendor_dependent_cmd branch 3 times, most recently from a54dac2 to 08e5c63 Compare September 29, 2025 06:01

lylezhu2012 reviewed Sep 29, 2025

View reviewed changes

lylezhu2012 mentioned this pull request Oct 9, 2025

Planned Bluetooth Classic improvement #76622

Open

26 tasks

Bluetooth: AVCTP: Fix payload length check in avctp_tx_processor

d50ad68

Updated the check to use buf->len to ensure the buffer contains enough data for the requested chunk size. Signed-off-by: Make Shi <[email protected]>

makeshi force-pushed the br_avrcp_vendor_dependent_cmd branch from 08e5c63 to 7f30b30 Compare October 13, 2025 03:50

zephyrbot requested a review from chengkai15 October 13, 2025 03:51

makeshi force-pushed the br_avrcp_vendor_dependent_cmd branch 3 times, most recently from 6f5302a to 3a07ada Compare October 13, 2025 06:59

makeshi added 3 commits October 13, 2025 17:56

Bluetooth: AVRCP: Add support for notification events

52d91a8

This patch adds AVRCP notification event handling for both CT and TG roles. Also add Shell command support for testing notification registration and responses. Signed-off-by: Make Shi <[email protected]>

makeshi force-pushed the br_avrcp_vendor_dependent_cmd branch from 3a07ada to 660cae6 Compare October 13, 2025 10:09

	uint8_t play_status; /*< Play status: @ref bt_avrcp_playback_status_t/
	uint8_t play_status; /*< Play status: @ref bt_avrcp_playback_status_t /

		if ((avrcp_tg_cb == NULL) \|\| (avrcp_tg_cb->set_absolute_volume_req == NULL)) {
		error_code = BT_AVRCP_STATUS_INTERNAL_ERROR;

Bluetooth: Classic: AVRCP: Support vendor dependent cmd #96273

Are you sure you want to change the base?

Bluetooth: Classic: AVRCP: Support vendor dependent cmd #96273

Conversation

makeshi commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarkWangChinese left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Oct 13, 2025

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

makeshi commented Sep 19, 2025 •

edited

Loading