implement a2dp remain todo/interfaces

[MarkWangChinese]

Implement the remain A2DP todo/interfaces as follow:

Implement the bt_a2dp_stream_reconfig: Modify reconfig_req callback to
pass codec_cfg to application. Remove reconfig_rsp callback,
config_rsp is used. Remove reconfigured callback, configured callback is used.

Implement the bt_a2dp_stream_release: The release_req,
release_rsp and released callbacks are already defined,
implement them in this change.

Implement the bt_a2dp_stream_suspend: The suspend_req,
suspend_rsp and suspended callbacks are already defined,
implement them in this change.

Add and implement the bt_a2dp_stream_abort: Add abort_req,
abort_rsp and aborted callbacks.

Implement bt_a2dp_disconnect.

Simplify the codes: (1) use the same bt_avdtp_ctrl_params to process open,
start, suspend, close and abort operations. (2) use same a2dp_ctrl_ind to
process open, start, suspend, close and abort operations. (3) use the same
bt_a2dp_ctrl_cb to process open, start, suspend, close and abort operations.
(4) use the same bt_avdtp_ctrl to process open, start, suspend, close and
abort operations. (5) move the status to common struct bt_avdtp_req.

Add shell test.

[jhedberg]

NULL checks for CONTAINER_OF() are almost always wrong. It's not a function, rather just pointer arithmetic. The only way you'd get NULL is if you give it a pointer which isn't NULL but has exactly the value NULL + offsetof(struct bt_a2dp_ep, sep). Checking for something like that seems pretty odd to me.

[MarkWangChinese]

Removed the cases and add some more BT_ASSERT for parameter sep.

[jhedberg]

What's the reason for changing to BT_ASSERT()? My preference would be that we completely remove that API from the Bluetooth subsystem and only use the Zephyr-wide assert API.

[MarkWangChinese]

Updated all assert

[jhedberg]

Sorry, but this is just unreadable. Could you introduce some helper variables which you prepare before the call to a2dp_ctrl_ind(), so that the actual call itself doesn't contain any conditionals. The resulting compiled code will most like be exactly the same since the compiler will optimize this either way. Also, please make sure you're using correct alignment of N tabs + 0-7 spaces, so that the split lines start right after the ( on the first line.

[MarkWangChinese]

Updated all the cases.

[jhedberg]

Same here.

[jhedberg]

And here.

[jhedberg]

And here.

[jhedberg]

And here.

[jhedberg]

And here

[jhedberg]

And here

[jhedberg]

And here

[jhedberg]

Why are you using capital U in some places but lower-case u in others?

[MarkWangChinese]

Change all to U

[jhedberg]

You should really fix this global lock stuff. First of all, a semaphore would be better than a mutex, and second of all it should be per-context rather than a global variable.

[MarkWangChinese]

change to semaphore and move it to the avdtp instance struct.

[jhedberg]

There's still inconsistency here. Just look at the defines above this. They all use lower-case u. Furthermore, there's also a namespace violation: this is a public Bluetooth header file, so all defines and symbols must use a BT_* or bt_* prefix.

[MarkWangChinese]

fixed.

[jhedberg]

fixed.

No, there's still A2DP_MAX_IE_LENGTH which doesn't have a BT_ prefix.

[MarkWangChinese]

Fixed.

[jhedberg]

What's the reason for this formatting change? I believe the convention in existing is to not have a space after :, e.g:

zephyr/subsys/bluetooth/mesh/net.h

Lines 133 to 138 in 34206f5

	uint8_t groups_changed:1, /* Friend Subscription List needs updating */
	pending_poll:1, /* Poll to be sent after subscription */
	disable:1, /* Disable LPN after clearing */
	fsn:1, /* Friend Sequence Number */
	established:1, /* Friendship established */
	clear_success:1; /* Friend Clear Confirm received */

[MarkWangChinese]

clang-format do it. The CI suggest me to do the clang-format.

[jhedberg]

Ok. That's interesting. Fair enough then, but make sure that style changes to existing code stays in separate commits. Also explain the reasoning in the commit message of such commits.

[MarkWangChinese]

OK, I will do as this way next time.

[jhedberg]

You're making this much more complicated than it needs to be. Since you have an array of contexts with CONFIG_BT_MAX_CONN elements, you can simply create a 1:1 mapping from bt_conn to that array with the help of bt_conn_index(): https://docs.zephyrproject.org/latest/doxygen/html/group__bt__conn.html#gad4aed76b80cc815f748ad0e84ae3d87c

I.e. something like:

static struct bt_a2dp *get_new_connection(struct bt_conn_conn)
{
	return &connection[bt_conn_index(conn)];
}

I don't think you'll need the A2DP_LOCK() stuff after that either.

[MarkWangChinese]

Theoretically more than one A2DP L2CAP connections can be created based on one ACL connection. Then they are not 1:1 mapping. Usually there is only one A2DP L2CAP connection for one ACL connection. If we need to support it, then the 1:1 mapping cannot be used.

[jhedberg]

Yes, I know, but can't you just make the general decision (at least for now) that you only support a single A2DP connection per ACL? It seems like you're already doing it, since the code is simply returning the same context if it already exists:

	if (connection[i].used == 1U && connection[i].session.br_chan.chan.conn == conn) {
		LOG_DBG("Conn already exists");
		A2DP_UNLOCK();
		return &connection[i];
	}

[MarkWangChinese]

I understand you, but I don't think it is good to limit the A2DP function in order to let the codes simple. I can fix the above codes that you mention. Is it OK?

[jhedberg]

I'm struggling to understand your point

1. What I'm proposing is not adding any new limits over what you already have in the PR
2. I don't see how you could have multiple "sessions" (i.e. multiple signaling channels) over a single ACL, since the AVDTP spec doesn't define anything like that (AFAIK). It defines the initial signaling channel and how meaning is applied to subsequent L2CAP channels using the AVDTP PSM. There can be at most signaling + 3 other channels according to AVDTP spec 1.3 section 5.4.6. Also see section 1.2 where it only talks about the signaling channel in singular rather than plural.
3. Even if it was theoretically possible to have multiple signaling channels for an ACL. I'm not aware of any devices that do that. Are you? Adding code complexity for non-existent theoretical situation is not a good practice IMO.
4. The array of contexts you have uses CONFIG_BT_MAX_CONN, implying that you really do want a 1:1 relationship. Otherwise it'd make more sense to define a new Kconfig option for this.

[MarkWangChinese]

OK, @jhedberg Thanks, I see the follow line in the spec too. I will update the codes.

The signaling channel (1) is only established once between two streaming devices.

[MarkWangChinese]

updated. please help to review again.

[jhedberg]

Please never create macros that make this kind of assumptions of the environment that they're invoked in, i.e. in this case that there has to be a given variable with a specific name. You should instead pass the session as a macro parameter. That said, I'm not sure what's the benefit of using macros here, since I don't think you want to preserve the possibility of redefining them or similar. Can't these just be one-line functions (which the compiler will certainly inline)?

static void avdtp_lock(struct bt_avdtp *session)
{
	k_sem_take(&session->sem_lock);
}

[MarkWangChinese]

Updated as you suggested.

[MarkWangChinese]

Github doesn't allow doing inline commenting on commit messages, so I'll do it here. Let's look at the first commit in this PR:

Implement the bt_a2dp_stream_reconfig: Modify reconfig_req callback to
pass codec_cfg to application. Remove reconfig_rsp callback,
config_rsp is used. Remove reconfigured callback,
configured callback is used.
Implement the bt_a2dp_stream_release: The release_req,
release_rsp and released callbacks are already defined,
implement them in this change.
Implement the bt_a2dp_stream_suspend: The suspend_req,
suspend_rsp and suspended callbacks are already defined,
implement them in this change.
Add and implement the bt_a2dp_stream_abort: Add abort_req,
abort_rsp and aborted callbacks.
Implement bt_a2dp_disconnect.
Simplify the codes: (1) use the same bt_avdtp_ctrl_params to process open,
start, suspend, close and abort operations. (2) use same a2dp_ctrl_ind to
process open, start, suspend, close and abort operations. (3) use the same
bt_a2dp_ctrl_cb to process open, start, suspend, close and abort
operations. (4) use the same bt_avdtp_ctrl to process open, start,
suspend, close and abort operations. (5) move the status to common struct
bt_avdtp_req.
change A2DP_MAX_IE_LENGTH as BT_A2DP_MAX_IE_LENGTH

Each of those paragraphs is a single logical change in itself, i.e. these changes should all be in separate commits. Whenever you find yourself enumerating multiple changes in the commit message and those changes are not tightly coupled, it's a good indication that you need to split things up. Additionally, in this same commit you're also including coding style changes to unrelated lines of code - those should also be in a separate commit.

Hi @jhedberg I have split the commit to three commits. I do the code format firstly with one commit. Then add one commit for the reconfig implementation and codes optimization. The last one is the new added close, suspend and abort. The second commit is still little big, but it is hard and need more effort to split because there are some common optimizations in it. Please help to review again. Thanks.

[jhedberg]

@MarkWangChinese are you still working on this? There are several failed CI checks.

[MarkWangChinese]

@MarkWangChinese are you still working on this? There are several failed CI checks.

Hi @jhedberg Sure, I am still working on it. I will fix it recently. Thanks

[lylezhu2012]

Hierarchically, this lock has no corresponding unlock.

[MarkWangChinese]

Fixed

[jhedberg]

This looks generally good enough to be merged. Just a few comments regarding x vs X in hex values (not sure why you want X when almost everywhere the code uses x)