Bluetooth: A2DP: Improve a2dp interface

[MarkWangChinese]

Implement the A2DP and AVDTP. It doesn't contain the SBC yet, SBC is split as #70531.
There are two samples: #69512 and #69513

[MarkWangChinese]

Hi @pitankar I see the previous a2dp is implemented by you, please help review this PR. Thanks.

[MarkWangChinese]

Hi @asbjornsabo could you please help review it? Thanks.

[MaureenHelm]

@carlescufi can you recommend appropriate reviewers please?

[streetdogg]

@MarkWangChinese - Really sorry for a delayed response. I checked this today. I had only briefly (in 2016) worked on Zephry BT stack while I was at Intel. I don't know who the current maintainer of bluetooth stack is. I would recomend checking the zephyr, bluez IRC channels.

@jhedberg - Can you help recommend a reviewer?

[Vudentz]

This should probably be made generic so other codecs can be initialized with the macro e.g:

#define BT_A2DP_ENDPOINT(name, type, codec, caps)
{
...(fill bt_a2dp_endpoint)
}
#define BT_A2DP_SINK(name, codec, caps) BT_A2DP_ENDPOINT(name, BT_A2DP_SINK, codec, caps)
#define BT_A2DP_SOURCE(name, codec, caps) BT_A2DP_ENDPOINT(name, BT_A2DP_SOURCE, codec, caps)

The for have SBC specific macros:
#define BT_A2DP_SBC_SOURCE(name, caps) BT_A2DP_SOURCE(name, BT_A2DP_SBC, caps)
#define BT_A2DP_SBC_SINK(name, caps) BT_A2DP_SINK(name, BT_A2DP_SBC, caps)

Note if we want to really declare it statically we could use Z_STRUCT_SECTION_ITERABLE to build it as part of the rom, but that can be done later on if we find it really worth adding another section in the binary.

[MarkWangChinese]

Has updated the codes, please check it again Thanks.
I think we don't need to put it in section. The usage is as follow:
case1: sbc source endpoint.
BT_A2DP_SBC_SOURCE_ENDPOINT(sbcEndpoint, A2DP_SBC_SAMP_FREQ_44100);
bt_a2dp_register_endpoint(&sbcEndpoint, BT_A2DP_AUDIO, BT_A2DP_SOURCE);
case2: sbc sink endpoint.
BT_A2DP_SBC_SINK_ENDPOINT(sbcEndpoint);
bt_a2dp_register_endpoint(&sbcEndpoint, BT_A2DP_AUDIO, BT_A2DP_SINK);

[Abildgren]

In LE Audio implementation we are going away from "cap" for capabilities as it is getting an overloaded term in Bluetooth Audio. I think @Vudentz ended on using "capability" instead. I suggest we streamline this across Zephyr

[MarkWangChinese]

Thanks, Has updated it.

[MarkWangChinese]

Hi All, do you have more comments? Can I merge to Zephyr?

[MaureenHelm]

Hi All, do you have more comments? Can I merge to Zephyr?

Please fix the failed compliance and documentation checks.

[MarkWangChinese]

@MaureenHelm I have fixed the document checks, Thanks. What's the next step?

[hongshui3000]

@MarkWangChinese Are there any plans to add avrcp&avctp interface？

This is also the missing part of the classic Bluetooth audio in zephyrrtos

[MaureenHelm]

@MaureenHelm I have fixed the document checks, Thanks. What's the next step?

Bluetooth maintainers need to approve. The commit message could also be improved to explain the motivation for this change.

[carlescufi]

@MarkWangChinese could you move this out of draft, please?

[dleach02]

@MarkWangChinese, Please move this out of draft. Also update the commit message per request from @MaureenHelm. Then we can get this PR moved along.

[MarkWangChinese]

@MarkWangChinese Are there any plans to add avrcp&avctp interface？

This is also the missing part of the classic Bluetooth audio in zephyrrtos

@hongshui3000 we have plan to implement the avrcp&avctp interface in future, then will upstream to zephyr.

[MarkWangChinese]

@MaureenHelm I have fixed the document checks, Thanks. What's the next step?

Bluetooth maintainers need to approve. The commit message could also be improved to explain the motivation for this change.

Hi @MaureenHelm Have updated the commit message.

[MarkWangChinese]

@MarkWangChinese, Please move this out of draft. Also update the commit message per request from @MaureenHelm. Then we can get this PR moved along.

Hi @dleach02 Have move it out of draft. Thanks

[joerchan]

There is just header file changes in this PR. Where is the source file changes?
If the implementation is out of the zephyr source is there any point in having the header file defined in the zephyr source?

If it is not implemented yet shouldn't we just wait for the implementation? No need to define APIs if we don't know if they are needed, or will work.

[MarkWangChinese]

There is just header file changes in this PR. Where is the source file changes?
If the implementation is out of the zephyr source is there any point in having the header file defined in the zephyr source?

If it is not implemented yet shouldn't we just wait for the implementation? No need to define APIs if we don't know if they are needed, or will work.

Hi @joerchan , I am implementing the source codes. When it is ready, I will update the PR.

[jori-nordic]

Just to add to Emil, I will also approve the changes when the current Classic maintainer @lylezhu2012 approves it.
I will only be reviewing parts that touch the shared LE/classic code.

[lylezhu2012]

It is better to return an error code if the feature is unsupported.

[lylezhu2012]

Same for this

[lylezhu2012]

You can consider using a macro to represent this value and passing the macro to this variable, instead of using the number directly. It's easier to modify it.

[dleach02]

@MarkWangChinese, please change the commit title from:

"subsys: bluetooth: host: add classic/Kconfig"

to:

"bluetooth: host: add classic/Kconfig"

This should clean up the compliance issue.

[MarkWangChinese]

Hi All, Thanks for the comments. Update from my side: I am working on aligning the A2DP interface with LE audio interface. For example: (1) add stream (2) add cbs that are like the bt_bap_unicast_client_cb, bt_bap_unicast_server_cb and bt_bap_stream_ops.

[MarkWangChinese]

Hi All, Thanks for the comments. Update from my side: I am working on aligning the A2DP interface with LE audio interface. For example: (1) add stream (2) add cbs that are like the bt_bap_unicast_client_cb, bt_bap_unicast_server_cb and bt_bap_stream_ops.

Done, please help review.

[Thalley]

Hi All, Thanks for the comments. Update from my side: I am working on aligning the A2DP interface with LE audio interface. For example: (1) add stream (2) add cbs that are like the bt_bap_unicast_client_cb, bt_bap_unicast_server_cb and bt_bap_stream_ops.

It would be nice that they were similar.

At some point we may want to add a bt_audio API that is profile agnostic, so that an application can use a single API for both LE and legacy audio devices, but that's far in the future at the moment :)

[hongshui3000]

Would it be better to make these functions static? I don't seem to find that it is a function that needs to be used globally.

[MarkWangChinese]

Thanks @hongshui3000 Fixed.

[hongshui3000]

In function naming, is it better to use a2dp prefix?

[MarkWangChinese]

All the three similar comments are fixed and add some comments in function.

[hongshui3000]

In function naming, is it better to use a2dp prefix?

[hongshui3000]

In function naming, is it better to use a2dp prefix? This function seems to do nothing??

[hongshui3000]

This function seems to do nothing？？

[MarkWangChinese]

It is AVDTP interface callback, A2DP return success/accept directly to let avdtp layer to response the AVDTP_DISCOVER cmd directly. I think A2DP don't need to call back this event to app because the codec endpoints are registered from application, the AVDTP_DISCOVER cmd can be responded directly.

[lylezhu2012]

There are two return error code of this callback. This is an interface exposed to the application, which may make it difficult for users to process. Is there a possibility to combine these two types? Such as, return uint8_t instead of int, and remove parameter uint8_t *rsp_err_code from callback prototype. Or another way is that remove parameter uint8_t *rsp_err_code, and keep return value int, and then convert negative POSIX int error code to uint8_t internally.

[MarkWangChinese]

It is like the LE audio callback. Return val is the zephyr error code, the rsp is the bluetooth spec rsp code.

	int (*config)(struct bt_conn *conn, const struct bt_bap_ep *ep, enum bt_audio_dir dir,
		      const struct bt_audio_codec_cfg *codec_cfg, struct bt_bap_stream **stream,
		      struct bt_audio_codec_qos_pref *const pref, struct bt_bap_ascs_rsp *rsp);

[lylezhu2012]

function

[MarkWangChinese]

Updated

[lylezhu2012]

Not aligned

[MarkWangChinese]

Updated

[lylezhu2012]

Is this interface necessary? Can it be assigned directly in application before configuration request or response?

[MarkWangChinese]

It is like BLE audio too.
void bt_bap_stream_cb_register(struct bt_bap_stream *stream, struct bt_bap_stream_ops *ops);

[lylezhu2012]

Suggest change to #if CONFIG_BT_A2DP_SOURCE

[jhedberg]

Suggest change to #if CONFIG_BT_A2DP_SOURCE

Will that compile if CONFIG_BT_A2DP_SOURCE is undefined? (which it will be when you set the Kconfig option to n)

[lylezhu2012]

Yes. It is a header file.
Maybe it could be #if defined(CONFIG_BT_A2DP_SOURCE).

[MarkWangChinese]

Updated

[lylezhu2012]

when the stream has been opened (media transport channel has been established) but the stream is not started, whether this case can be handled by the function if the function is called?

[MarkWangChinese]

Updated

[lylezhu2012]

Change -EINVAL to -ENOTSUP.

[MarkWangChinese]

Updated

[lylezhu2012]

Change -EINVAL to -ENOTSUP.

[MarkWangChinese]

Updated

[lylezhu2012]

Change -EPERM to -EINVAL.

[MarkWangChinese]

Updated

[lylezhu2012]

Change -EPERM to -EINVAL.

For all cases that the parameter is invalid, it is better to return -EINVAL.

[MarkWangChinese]

Updated

[lylezhu2012]

It is better to add more descriptions to introduce how to use this callback, including the usage of parameter info, and ep.
About return value, it also needs more description about which value could be returned and the meaning of the value.

[MarkWangChinese]

Updated

[lylezhu2012]

It's better to explain the meaning of this bit. How users use it.

[MarkWangChinese]

Updated

[lylezhu2012]

Whether needs to ensure the last discover has done before starting a new discover?

[MarkWangChinese]

Updated

[lylezhu2012]

The parameter ep is still needed here? After the callback returns, there is no processing of parameter ep.

[MarkWangChinese]

Updated for all this case.

[lylezhu2012]

same here

[lylezhu2012]

The issue that the parameter ep is useless also occurs here.

[lylezhu2012]

same here.

[lylezhu2012]

Whether the local and remote ep configurations match? Such as, remote ep is a source, local ep should be a sink. Or, local ep is a source, remote is a sink. I think It needs to be checked here.

Besides, I think it needs to check that whether the signalling channel has been connected.

[MarkWangChinese]

Add codes to check endpoint's source/sink role and codec type.
Signalling channel is created in AVDTP, if it is disconnected, the l2cap sending will fail.

[lylezhu2012]

The stream needs to be confirmed that it has been configured firstly.

Besides, I think it needs to check that whether the signalling channel has been connected.

[MarkWangChinese]

Add codes to check it in AVDTP.
Signalling channel is created in AVDTP, if it is disconnected, the l2cap sending will fail.

[lylezhu2012]

The stream needs to be confirmed that it has been opened firstly.

Besides, I think it needs to check that whether the signalling channel has been connected.

[MarkWangChinese]

Add codes to check it in AVDTP.
Signalling channel is created in AVDTP, if it is disconnected, the l2cap sending will fail.

[jhedberg]

This is not safe. The in-memory ordering of these bitfields will depend on the endianness of the architecture that you're building this for. It'd be better to just have a single uint8_t field and then use helper macros to access the individual bits.

[MarkWangChinese]

Thanks @jhedberg , I see Zephyr handle it by using CONFIG_LITTLE_ENDIAN. I can do the same way, is it OK? Because it is easy to use and write codes by using bit-fields.

/* RFC 4191, ch. 2.3 */
struct net_icmpv6_nd_opt_route_info {
	uint8_t prefix_len;
	struct {
#ifdef CONFIG_LITTLE_ENDIAN
		uint8_t reserved_2 :3;
		uint8_t prf        :2;
		uint8_t reserved_1 :3;
#else
		uint8_t reserved_1 :3;
		uint8_t prf        :2;
		uint8_t reserved_2 :3;
#endif
	} flags;
	uint32_t route_lifetime;
	/* Variable-length prefix field follows, can be 0, 8 or 16 bytes
	 * depending on the option length.
	 */
} __packed;

[jhedberg]

@MarkWangChinese I'm fine either way. The needed duplication in struct definitions (or internal subsets of a struct like in the above case) can get a bit messy and hard to follow, but in this case it seems it's fairy limited in scope. Btw, if possible, please double check that you got this correct by analyzing what actually gets generated by the compiler - I think we have at least some big endian architectures supported by Zephyr?

[MarkWangChinese]

Updated. I tried the gr716a_mini board with temporary bit-fields codes. I think the follow compiled assembly is expected/right.

struct test_struc {
	union {
		struct {
			uint8_t bit1:1;
			uint8_t bit2:2;
			uint8_t bit3:5
		} bit_test;
		uint8_t u8data;
	};
};

volatile struct test_struc test_data;

int main(void)
{
	test_data.bit_test.bit1 = 1;
        ......
}

	test_data.bit_test.bit1 = 1;
3100106c:	03 0c 00 01 	sethi  %hi(0x30000400), %g1
31001070:	c4 08 61 d8 	ldub  [ %g1 + 0x1d8 ], %g2	! 300005d8 <test_data>
31001074:	84 10 bf 80 	or  %g2, -128, %g2
31001078:	c4 28 61 d8 	stb  %g2, [ %g1 + 0x1d8 ]

[jhedberg]

I'm not sure you have the struct naming correct here. The SEID is just a numeric value that refers to a SEP, so it seems counterintuitive to me to have a struct (which contains much more than the numeric id) called bt_avdtp_seid. Wouldn't struct bt_avdt_sep be a more appropriate name?

[MarkWangChinese]

Updated

[jhedberg]

A few more comments & questions

[jhedberg]

It really isn't ideal that you're ending up duplicating all of the documentation too. Every time you need to fix or update something you will then need to do it in two places. I also don't think this is particularly intuitive for someone who primarily reads documentation from the code rather than something generated by oxygen. I'd consider refactoring out the documentation to a single block (e.g. everything before the struct), but really I'm starting to regret giving you flexibility to choose this path rather than using helper macros :) Would it really be that bad to have something like this:

#define BT_A2DP_SBC_MEDIA_HDR_NUM_FRAMES(hdr) FIELD_GET(GENMASK(3, 0), (hdr))
#define BT_A2DP_SBC_MEDIA_HDR_L(hdr)          FIELD_GET(BIT(5), (hdr))
#define BT_A2DP_SBC_MEDIA_HDR_S(hdr)          FIELD_GET(BIT(6), (hdr))
#define BT_A2DP_SBC_MEDIA_HDR_F(hdr)          FIELD_GET(BIT(7), (hdr))

[MarkWangChinese]

Appreciate your comments.
My main concern is that the application will use the struct fields too. like the follow codes in shell\a2dp.c.

	sbc_hdr = net_buf_add(buf, sizeof(struct bt_a2dp_codec_sbc_media_packet_hdr));
	memset(sbc_hdr, 0, sizeof(struct bt_a2dp_codec_sbc_media_packet_hdr));
	sbc_hdr->number_of_sbc_frames = 1;

If we use the MACRO, then I think we need to add GET/SET macros as follow, and application need to use the MACRO (from my viewpoint, it is not convenient).

#define BT_A2DP_SBC_MEDIA_HDR_NUM_FRAMES_GET(hdr) FIELD_GET(GENMASK(3, 0), (hdr)) //hdr is value.
#define BT_A2DP_SBC_MEDIA_HDR_NUM_FRAMES_SET(hdr, val) hdr = (hdr & ~GENMASK(3, 0)) | (GENMASK(3, 0) & val)

What do you think?

[jhedberg]

Looks alright, but I think it'd make sense to use FIELD_PREP() for the setting of the value. It might also make sense to encode the entire header in one go, i.e. have the setter macro take all fields as separate parameters and then construct the unified uint8_t out of them, something like:

*hdr = BT_A2DP_SBC_MEDIA_HDR_ENCODE(num_frames, l, s, f);

[MarkWangChinese]

Codes are updated. I add xx_GET, xx_SET and xx_ENCODE.

[jhedberg]

I'd drop the _type from the struct name

[MarkWangChinese]

Updated.

[jhedberg]

Same here regarding the duplicated documentation. Sorry about the back-and-forth with bitfields vs accessor macros solutions, but now that I see the end result the accessor macros (especially with Zephyr's FIELD_GET(), FIELD_PREP(), GENMASK(), etc helpers) feels like a cleaner approach. Feel free to provide counterarguments of course (e.g. if the documentation can be moved into a single place, although then it wouldn't be inline anymore), but the macro-based option is what seems like a slightly better solution to me right now.

[MarkWangChinese]

Codes are updated. The bit-fields structure is only used internally.

[jhedberg]

I guess the variable name should be renamed too (to sep). Same for all other places where you have a struct avdtp_sep pointer.

[MarkWangChinese]

Updated

[jhedberg]

It's impossible for ep to be NULL. Even if seid was NULL you'd get pointer underflow with the CONTAINER_OF() operation usage and some garbage non-NULL value.

Should the ep->stream == NULL be an assert instead? Seems like a local bug rather than a valid runtime error condition to me.

[MarkWangChinese]

I remove the ep == NULL and keep ep->stream. The ep->stream is set when configuring codec that creating the stream (like: LE audio). In right flow, the ep->stream must not be NULL. But it may be NULL if stack overflow or it is set as NULL by application codes wrongly. Do you have any reference/document that explain how to distinguish between "a local bug" with "runtime error"?

[jhedberg]

Why is the input parameter type void *, yet the implementation assumes a specific struct type (bt_a2dp_codec_sbc_params)? How is this API intended to be used in a safe way, since the compiler will not warn if you try to pass a pointer of the wrong type? Same issue with the get_channel_num() API.

[MarkWangChinese]

The different codec has different config fields definition. The a2dp.c/h don't need to know the config fields details. For example: a2dp.c/h just callback the config result buffer, for SBC, application use a2dp_codec_sbc.c/h to parse the config result. In my previous implementation, I do the follow way:

bt_a2dp_sbc_get_sampling_frequency((struct bt_a2dp_codec_sbc_params *)&codec_cfg->codec_config->codec_ie[0]);

I will change it back to this way.

[MarkWangChinese]

Updated.

[jhedberg]

I don't see any public functions exposed by this header file. Only struct definitions. Does the header really need to be public?

[MarkWangChinese]

I will try to put struct bt_avdtp_sep to avdtp_internal.h, others can be used in application and are put in avdtp.h because they are from the avdtp specification.

[MarkWangChinese]

I can't put the struct bt_avdtp_sep to avdtp_internal.h too because it is used in struct bt_a2dp_ep.

[jhedberg]

@MarkWangChinese looks better, but there's a Compliance check failure that you need to fix.

[MarkWangChinese]

@MarkWangChinese looks better, but there's a Compliance check failure that you need to fix.

Resolved

[lylezhu2012]

@MarkWangChinese , I found the change was not added to release notes. Could you please add a mention in release notes? Thanks a lot.

[MarkWangChinese]

@MarkWangChinese , I found the change was not added to release notes. Could you please add a mention in release notes? Thanks a lot.

Sure