Bluetooth: AVRCP: Implement unit message reception on the AVRCP target #85917

makeshi · 2025-02-18T09:57:15Z

@gzh-terry Please help review it, thanks.

gzh-terry · 2025-02-18T14:56:43Z

include/zephyr/bluetooth/classic/avrcp.h

@@ -177,6 +177,13 @@ struct bt_avrcp_cb {
 	 *  @param rsp The response for PASS THROUGH command.
 	 */
 	void (*passthrough_rsp)(struct bt_avrcp *avrcp, struct bt_avrcp_passthrough_rsp *rsp);
+	/** @brief Callback function called when received unit info command


Leave one line between 179 and 180 would be better :)

Thanks， I remember there was no space before in these call back function list, and I'd like to keep it consistent. :) Anyway, I'll add a space.

Thanks， I remember there was no space before in these call back function list, and I'd like to keep it consistent. :) Anyway, I'll add a space.

Yes, I added it here #84367 :)

include/zephyr/bluetooth/classic/avrcp.h

gzh-terry · 2025-02-18T15:02:41Z

include/zephyr/bluetooth/classic/avrcp.h

@@ -249,6 +256,17 @@ int bt_avrcp_get_subunit_info(struct bt_avrcp *avrcp);
 int bt_avrcp_passthrough(struct bt_avrcp *avrcp, bt_avrcp_opid_t operation_id,
 			 bt_avrcp_button_state_t state, const uint8_t *payload, uint8_t len);

+/** @brief Response unit info command.
+ *
+ *  Send the response unit info command when the receiver will be notified by the registered


response unit info command -> unit info response

include/zephyr/bluetooth/classic/avrcp.h

gzh-terry · 2025-02-18T15:07:16Z

subsys/bluetooth/host/classic/avrcp.c

+int bt_avrcp_response_unit_info(struct bt_avrcp *avrcp, struct bt_avrcp_unit_info_rsp *rsp)
+{
+	struct net_buf *buf;
+	uint8_t data[5];


Try to keep style consistent, e.g., use param[5].

subsys/bluetooth/host/classic/avrcp.c

gzh-terry · 2025-02-18T15:17:25Z

subsys/bluetooth/host/classic/shell/avrcp.c

+	rsp.unit_type = 0x9;
+	rsp.company_id = 0x001958; /*BT SIG registered Company ID*/
+
+	err = bt_avrcp_response_unit_info(avrcp, &rsp);


Caution: this is in the callback thread, and might lead to assertion in net_buf.

Thanks, let me check it

subsys/bluetooth/host/classic/shell/avrcp.c

gzh-terry · 2025-02-20T07:16:52Z

include/zephyr/bluetooth/classic/avrcp.h

@@ -249,6 +257,16 @@ int bt_avrcp_get_subunit_info(struct bt_avrcp *avrcp);
 int bt_avrcp_passthrough(struct bt_avrcp *avrcp, bt_avrcp_opid_t operation_id,
 			 bt_avrcp_button_state_t state, const uint8_t *payload, uint8_t len);


Set -> Send?

It's also okay to use "send". I use set because I want it to correspond to the above API bt_avrcp_get_unit_info.

It's also okay to use "send". I use set because I want it to correspond to the above API bt_avrcp_get_unit_info.

I think set usualy means to provide a value and save at the local device, or set a configuration to the remote.
However, this interface is to inform the remote device, rather than a configuration.

Agree, Here using "Send" here can indeed better express the actual meaning than "Set" for this function. I'll update the comments and the function name.

gzh-terry

Generally fine to me.

lylezhu2012 · 2025-02-21T03:14:32Z

subsys/bluetooth/host/classic/avrcp.c

-		/* ToDo */
+		if ((avrcp_cb != NULL) && (avrcp_cb->unit_info_req != NULL)) {
+			avrcp_hdr = (void *)buf->data;
+			net_buf_pull(buf, sizeof(*avrcp_hdr));


The buf->len needs to be checked before pulling the net buffer.

After checking the whole receiving data path, I found an issue that the buf->len is also not checked before accessing the buf->data in function avrcp_recv.

zephyr/subsys/bluetooth/host/classic/avrcp.c

Lines 354 to 366 in 7ee4011

avctp_hdr = (void *)buf->data;

net_buf_pull(buf, sizeof(*avctp_hdr));

if (buf->len < sizeof(*avrcp_hdr)) {

LOG_ERR("invalid AVRCP header received");

return -EINVAL;

}

avrcp_hdr = (void *)buf->data;

tid = BT_AVCTP_HDR_GET_TRANSACTION_LABLE(avctp_hdr);

cr = BT_AVCTP_HDR_GET_CR(avctp_hdr);

rsp = BT_AVRCP_HDR_GET_CTYPE_OR_RSP(avrcp_hdr);

subunit_id = BT_AVRCP_HDR_GET_SUBUNIT_ID(avrcp_hdr);

subunit_type = BT_AVRCP_HDR_GET_SUBUNIT_TYPE(avrcp_hdr);

lylezhu2012 · 2025-02-21T03:15:58Z

subsys/bluetooth/host/classic/avrcp.c

@@ -253,9 +253,26 @@ static void avrcp_unit_info_handler(struct bt_avrcp *avrcp, struct net_buf *buf,
 {
 	struct bt_avrcp_header *avrcp_hdr;
 	struct bt_avrcp_unit_info_rsp rsp;
+	bt_avrcp_subunit_type_t subunit_type;

 	if (cr == BT_AVCTP_CMD) {


I think it is better to split the handler for request and response.

I will split the AVRCP code to handle with different role CT and CG.

lylezhu2012 · 2025-02-21T03:18:09Z

subsys/bluetooth/host/classic/avrcp.c

+			if ((subunit_type != BT_AVRCP_SUBUNIT_TYPE_UNIT) ||
+				(avrcp_hdr->opcode != BT_AVRCP_OPC_UNIT_INFO)) {
+				LOG_ERR("Invalid unit info command");
+				return;


For the command, if the command is invalid, the response should be sent internally instead of ignoring it.

lylezhu2012 · 2025-02-21T03:19:50Z

subsys/bluetooth/host/classic/avrcp.c


 	if (cr == BT_AVCTP_CMD) {
-		/* ToDo */
+		if ((avrcp_cb != NULL) && (avrcp_cb->unit_info_req != NULL)) {


The command handling should be controlled by the registered callback. Such as, if the command is not valid or the command is unsupported, the error response should be sent directly.

lylezhu2012 · 2025-02-21T03:38:44Z

subsys/bluetooth/host/classic/avrcp.c

+{
+	struct net_buf *buf;
+
+	buf = avrcp_create_unit_pdu(avrcp, BT_AVCTP_RESPONSE);


The TID should be same with the TID of the command.

TID should added as all function calls and callback parameter, handled in application code side.

But the TID cannot be set by application in your PR.

And I think if the TID need be set by application, the TID of command should also be notified to application.

As you comments as in #85910, these is only one command is ongoing , so also need the TID sent should be same with the TID of the command.

lylezhu2012 · 2025-02-21T03:41:35Z

subsys/bluetooth/host/classic/avrcp.c

+	}
+
+	/* Add additional info */
+	net_buf_add_u8(buf, 0x07);


What does the mean of 7? I think the hard code should be defined as a MICRO.

I did not find the real mean of the 7,

and The protocol analysis tool recognizes as the "Const byte"

Hi @makeshi @lylezhu2012

0x07 means page = 0 and extension_code = 7
page shall only be greater than 0 when the number of extended units exceeds 4, which is not possible at unit info response, where the number of extended units is 0.
extension_code is not defined currently, so all three bits are set to 1.

UNIT INFO response generally has the same structure as SUBUNIT INFO response.

@makeshi can use

FIELD_PREP(GENMASK(6, 4), AVRCP_SUBUNIT_PAGE) | FIELD_PREP(GENMASK(2, 0), AVRCP_SUBUNIT_EXTENSION_COED);

Which exactly is 0x7

Hi @makeshi @lylezhu2012

0x07 means page = 0 and extension_code = 7 page shall only be greater than 0 when the number of extended units exceeds 4, which is not possible at unit info response, where the number of extended units is 0. extension_code is not defined currently, so all three bits are set to 1.

The command is unit info instead of subunit info.
From UNIT INFO status command response format, it is a hardcode without any comments. It seams the field has not yet explained of what it means.

Hi @makeshi @lylezhu2012
0x07 means page = 0 and extension_code = 7 page shall only be greater than 0 when the number of extended units exceeds 4, which is not possible at unit info response, where the number of extended units is 0. extension_code is not defined currently, so all three bits are set to 1.

The command is unit info instead of subunit info. From UNIT INFO status command response format, it is a hardcode without any comments. It seams the field has not yet explained of what it means.

Then just put 0x7 is fine, to exactly match the spec.

It's good to have at least a code comment pointing out that the constant is hard-coded in the spec

lylezhu2012 · 2025-02-21T03:43:31Z

subsys/bluetooth/host/classic/avrcp.c

+	/* Add additional info */
+	net_buf_add_u8(buf, 0x07);
+	/* Add Unit Type info */
+	net_buf_add_u8(buf, FIELD_PREP(GENMASK(7, 3), (rsp->unit_type)));


For AVRCP, the unit_type should be Panel, right?

current , it is right, but in spec, it mentions

So, for the code extensibility, I also used the uint_type as a parameter not hard code for Panel

@makeshi @lylezhu2012
We may consider to expose bt_avrcp_subunit_type_t (BT_AVRCP_SUBUNIT_TYPE_PANEL = 0x9) to users?

Yes, I think we could expose it

nashif · 2025-02-21T13:18:02Z

Please set a readable and useful PR title and add more description the PR body, the PR body should be used for more details about the change and not to ask for reviews.

Add a new callback to support the situation when a unit info command is received, and add an API to respond to the unit info command. Signed-off-by: Make Shi <make.shi@nxp.com>

Add avrcp_unit_info_req callback function in shell and a command to set the unit info response. Signed-off-by: Make Shi <make.shi@nxp.com>

zephyrbot added area: Bluetooth area: Bluetooth Classic Bluetooth Classic (BR/EDR) labels Feb 18, 2025

zephyrbot requested review from jhedberg, lylezhu2012 and MarkWangChinese February 18, 2025 09:57

zephyrbot assigned lylezhu2012 Feb 18, 2025

makeshi force-pushed the improve_br_avrcp branch 2 times, most recently from c4c6aa7 to 3dad734 Compare February 18, 2025 10:30

gzh-terry reviewed Feb 18, 2025

View reviewed changes

include/zephyr/bluetooth/classic/avrcp.h Outdated Show resolved Hide resolved

gzh-terry reviewed Feb 18, 2025

View reviewed changes

include/zephyr/bluetooth/classic/avrcp.h Outdated Show resolved Hide resolved

gzh-terry reviewed Feb 18, 2025

View reviewed changes

include/zephyr/bluetooth/classic/avrcp.h Outdated Show resolved Hide resolved

gzh-terry reviewed Feb 18, 2025

View reviewed changes

include/zephyr/bluetooth/classic/avrcp.h Outdated Show resolved Hide resolved

gzh-terry reviewed Feb 18, 2025

View reviewed changes

subsys/bluetooth/host/classic/avrcp.c Outdated Show resolved Hide resolved

gzh-terry reviewed Feb 18, 2025

View reviewed changes

subsys/bluetooth/host/classic/avrcp.c Outdated Show resolved Hide resolved

gzh-terry reviewed Feb 18, 2025

View reviewed changes

subsys/bluetooth/host/classic/avrcp.c Outdated Show resolved Hide resolved

gzh-terry reviewed Feb 18, 2025

View reviewed changes

subsys/bluetooth/host/classic/shell/avrcp.c Outdated Show resolved Hide resolved

makeshi force-pushed the improve_br_avrcp branch 2 times, most recently from 4f7f13f to 510fc82 Compare February 20, 2025 06:45

makeshi requested a review from gzh-terry February 20, 2025 06:58

gzh-terry reviewed Feb 20, 2025

View reviewed changes

gzh-terry previously approved these changes Feb 20, 2025

View reviewed changes

lylezhu2012 requested changes Feb 21, 2025

View reviewed changes

makeshi changed the title ~~Improve br avrcp~~ Bluetooth: AVRCP: Implement unit message reception on the AVRCP target Feb 25, 2025

makeshi dismissed gzh-terry’s stale review via 54dd129 March 4, 2025 07:18

makeshi force-pushed the improve_br_avrcp branch from 510fc82 to 54dd129 Compare March 4, 2025 07:18

makeshi force-pushed the improve_br_avrcp branch from 54dd129 to e772dc6 Compare March 4, 2025 07:47

makeshi added 2 commits March 4, 2025 16:13

Bluetooth: AVRCP: Implement unit message reception on the AVRCP target

001304f

Add a new callback to support the situation when a unit info command is received, and add an API to respond to the unit info command. Signed-off-by: Make Shi <make.shi@nxp.com>

Bluetooth: Shell: AVRCP: support unit info request and response

865fe77

Add avrcp_unit_info_req callback function in shell and a command to set the unit info response. Signed-off-by: Make Shi <make.shi@nxp.com>

makeshi force-pushed the improve_br_avrcp branch from e772dc6 to 865fe77 Compare March 4, 2025 08:15

makeshi requested review from lylezhu2012 and gzh-terry March 4, 2025 08:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: AVRCP: Implement unit message reception on the AVRCP target #85917

Bluetooth: AVRCP: Implement unit message reception on the AVRCP target #85917

makeshi commented Feb 18, 2025 •

edited

Loading

gzh-terry Feb 18, 2025

makeshi Feb 19, 2025 •

edited

Loading

gzh-terry Feb 19, 2025

gzh-terry Feb 18, 2025

gzh-terry Feb 18, 2025

gzh-terry Feb 18, 2025

makeshi Feb 19, 2025

gzh-terry Feb 20, 2025

makeshi Feb 20, 2025

gzh-terry Feb 21, 2025

makeshi Feb 21, 2025

gzh-terry left a comment

lylezhu2012 Feb 21, 2025

lylezhu2012 Feb 21, 2025

makeshi Feb 21, 2025

lylezhu2012 Feb 21, 2025 •

edited

Loading

lylezhu2012 Feb 21, 2025

lylezhu2012 Feb 21, 2025

makeshi Feb 21, 2025

lylezhu2012 Feb 24, 2025 •

edited

Loading

makeshi Feb 27, 2025

lylezhu2012 Feb 21, 2025

makeshi Feb 21, 2025

gzh-terry Feb 21, 2025 •

edited

Loading

gzh-terry Feb 21, 2025

gzh-terry Feb 21, 2025 •

edited

Loading

lylezhu2012 Feb 24, 2025

gzh-terry Feb 24, 2025

jhedberg Feb 28, 2025

lylezhu2012 Feb 21, 2025

makeshi Feb 21, 2025

gzh-terry Feb 21, 2025

makeshi Feb 24, 2025

nashif commented Feb 21, 2025

		@@ -249,6 +257,16 @@ int bt_avrcp_get_subunit_info(struct bt_avrcp *avrcp);
		int bt_avrcp_passthrough(struct bt_avrcp *avrcp, bt_avrcp_opid_t operation_id,
		bt_avrcp_button_state_t state, const uint8_t *payload, uint8_t len);

	avctp_hdr = (void *)buf->data;
	net_buf_pull(buf, sizeof(*avctp_hdr));
	if (buf->len < sizeof(*avrcp_hdr)) {
	LOG_ERR("invalid AVRCP header received");
	return -EINVAL;
	}

	avrcp_hdr = (void *)buf->data;
	tid = BT_AVCTP_HDR_GET_TRANSACTION_LABLE(avctp_hdr);
	cr = BT_AVCTP_HDR_GET_CR(avctp_hdr);
	rsp = BT_AVRCP_HDR_GET_CTYPE_OR_RSP(avrcp_hdr);
	subunit_id = BT_AVRCP_HDR_GET_SUBUNIT_ID(avrcp_hdr);
	subunit_type = BT_AVRCP_HDR_GET_SUBUNIT_TYPE(avrcp_hdr);

Bluetooth: AVRCP: Implement unit message reception on the AVRCP target #85917

Are you sure you want to change the base?

Bluetooth: AVRCP: Implement unit message reception on the AVRCP target #85917

Conversation

makeshi commented Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

makeshi Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gzh-terry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lylezhu2012 Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lylezhu2012 Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gzh-terry Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gzh-terry Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nashif commented Feb 21, 2025

makeshi commented Feb 18, 2025 •

edited

Loading

makeshi Feb 19, 2025 •

edited

Loading

lylezhu2012 Feb 21, 2025 •

edited

Loading

lylezhu2012 Feb 24, 2025 •

edited

Loading

gzh-terry Feb 21, 2025 •

edited

Loading

gzh-terry Feb 21, 2025 •

edited

Loading