Bluetooth: Audio: CSIP set member register fix #56034
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Thalley
requested review from
jhedberg,
Vudentz,
wopu-ot,
alwa-nordic and
sjanc
as code owners
zephyrbot
requested review from
asbjornsabo,
Casper-Bonde-Bose,
fredrikdanebjer,
kruithofa,
larsgk,
MariuszSkamra and
szymon-czapracki
Thalley
force-pushed
the
csip_set_member_register_fix
branch
2 times, most recently
from
9ae86ca
to
bc7fa59
Compare
Thalley
force-pushed
the
csip_set_member_register_fix
branch
from
bc7fa59
to
93e090f
Compare
Comment on lines
+785
to
+795
| if (param->rank == 0U) { | ||
| remove_csis_char(BT_UUID_CSIS_RANK, inst->service_p); | ||
| } | ||
|
|
||
| if (param->set_size == 0U) { | ||
| remove_csis_char(BT_UUID_CSIS_SET_SIZE, inst->service_p); | ||
| } | ||
|
|
||
| if (!param->lockable) { | ||
| remove_csis_char(BT_UUID_CSIS_SET_LOCK, inst->service_p); | ||
| } |
There was a problem hiding this comment.
Wouldn't it be easier and cleaner to do the opposite? I mean to compose the service attributes based on parameters given?
Luckily, there is no bt_csip_set_member_unregister method, because, currently, this does not restore the csip_set_member_service_list initial state.
There was a problem hiding this comment.
That's a good point, but the reason why it is done this way, is that the list of CSIS instances are declared using BT_GATT_SERVICE_INSTANCE_DEFINE.
We could modify BT_CSIP_SERVICE_DEFINITION to have these 3 characteristics as empty by default and then populate them when requested. That might indeed by the cleaner solution, even if takes a bit more effort :)
Let me know if that makes sense to you
szymon-czapracki
previously approved these changes
May 9, 2023
| } else { | ||
| cur_inst = NULL; | ||
| if (!busy && err == 0) { | ||
| /* Not lock states are being read, can just call the ordered access callback |
There was a problem hiding this comment.
This comment is not clear to me, possibly there is a typo in it?
There was a problem hiding this comment.
Yeah, that could be improved. While improving the comment, figured that the code could also be improved for this if statement, so that was also changed
The bt_csip_set_member_register function would register the connection, and connection auth info, callbacks for each CSIS instance, which could give errors when/if the callbacks are called too many times. In any case, they should not be registered multiple times. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Whenever a CSIS instance is registered with bt_csip_set_member_register we now properly check if the set size, rank and lock characteristics should be present. If not, then remove them from the service declaration. It is done this way, as it is easier (read: possible) to remove characteristics from the service declarations, than it is to add the optional characteristics, as the optional characteristics may be present in some CSIS instances, but not in others. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Enable the CSIP Set Member debug log. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Thalley
force-pushed
the
csip_set_member_register_fix
branch
from
93e090f
to
cbe4bec
Compare
Thalley
force-pushed
the
csip_set_member_register_fix
branch
from
cbe4bec
to
18f6fb4
Compare
larsgk
reviewed
May 9, 2023
| @@ -681,7 +684,7 @@ static bool valid_register_param(const struct bt_csip_set_member_register_param | |||
| return false; | |||
| } | |||
|
|
|||
| if (param->rank > 0 && param->rank > param->set_size) { | |||
| if (param->rank > 0 && param->set_size > 0 && param->rank > param->set_size) { | |||
| LOG_DBG("Invalid rank: %u (shall be less than set_size: %u)", param->set_size, | |||
There was a problem hiding this comment.
first arg should be param->rank and should it be 'less than' or 'less than or equal', which seems to be what the test checks for
Thalley
force-pushed
the
csip_set_member_register_fix
branch
from
18f6fb4
to
98359f8
Compare
The set size and rank validation did not allow setting e.g. rank = 2 and set_size = 0, which should be allowed. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
The set info returned by the Set Coordinator did not contain information about the lockable state. Furthermore the set info was not properly initiated, as it was missing the rank, which was set somewhere else. This commit adds the lockable state to the set info. This commit removes the rank from internal structure, and uses a reference to the set info instead, and only store the rank there. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Split the CSIP tests to multiple scripts to make them more specific and able to run in parallel. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Fixed issue when performing the ordered access procedure on a set of devices where none had the lock characteristics. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Add tests for cases where the rank characteristic, the size characteristic and the lock characteristics are not set on the set members. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Thalley
force-pushed
the
csip_set_member_register_fix
branch
from
98359f8
to
1fb3225
Compare
fredrikdanebjer
approved these changes
May 9, 2023
szymon-czapracki
approved these changes
May 10, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes a bug with the CSIP set member register. Fixing this issue and adding tests for it, also found a few other issues that were found and fixed in order to get the tests parsing.
Fixes #55457