Bluetooth: CSIP: Set member: Fix issue with re-registration #89089
Conversation
Thalley
force-pushed
the
csip_register_fix
branch
2 times, most recently
from
6e09f61 to
c0ceb01
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the re‐registration issue for CSIS by removing the static counter and instead scanning the service instance array to locate available entries. It also restores the original service definition during unregistration and adds a test to verify that re‐registration works as expected.
- Removed the instance counter in bt_csip_set_member_register and replaced it with an iterative search.
- Restored the original attribute declaration during unregistration.
- Introduced a new test (test_register) to validate repeated register/unregister cycles.
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/bsim/bluetooth/audio/src/csip_set_member_test.c | Added test_register function to iterate through registration/unregistration scenarios. |
| subsys/bluetooth/audio/csip_set_member.c | Updated bt_csip_set_member_register to remove the counter and restore service definitions in bt_csip_set_member_unregister to enable re-registration. |
Files not reviewed (1)
- tests/bsim/bluetooth/audio/test_scripts/csip_register.sh: Language not supported
Comments suppressed due to low confidence (1)
subsys/bluetooth/audio/csip_set_member.c:852
- The flag 'first_register' is never updated after the first registration, which could result in repeated callback registrations. Set 'first_register' to true after the first successful registration to prevent duplicate registrations.
if (!first_register) {
1931b39 to
5ec06c3
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the re-registration issue with CSIS by removing the outdated counter mechanism and restoring the original service definition. It also adds a new test to ensure the fix works as expected.
- Updated test to validate re-registration behavior
- Modified the service registration logic to search for a free service instance slot and restore original attributes on unregistration
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/bsim/bluetooth/audio/src/csip_set_member_test.c | Added the test_register function to validate correct re-registration handling |
| subsys/bluetooth/audio/csip_set_member.c | Revised registration logic, fixed formatting, and restored original service attributes |
Files not reviewed (1)
- tests/bsim/bluetooth/audio/test_scripts/csip_register.sh: Language not supported
github-actions
bot
added
the
platform: nRF BSIM
github-actions
bot
requested review from
aescolar,
asbjornsabo,
babrsn,
Casper-Bonde-Bose,
fredrikdanebjer,
jhedberg,
jthm-ot,
larsgk,
MariuszSkamra,
niym-ot,
pin-zephyr and
sjanc
Thalley
force-pushed
the
csip_register_fix
branch
from
5ec06c3 to
a3d69c8
Compare
Thalley
force-pushed
the
csip_register_fix
branch
from
a3d69c8 to
0683f6c
Compare
| .pairing_complete = auth_pairing_complete, | ||
| .bond_deleted = csip_bond_deleted | ||
| }; | ||
| static struct bt_conn_auth_info_cb auth_callbacks = {.pairing_complete = auth_pairing_complete, |
There was a problem hiding this comment.
Is this really done on purpuse? This is not how we usually format a statically defined callback structure!? Pre-changed looks more consistent
There was a problem hiding this comment.
Welp, my editor done goofed and formatted the entire file. Will revert the formatting lines
There was a problem hiding this comment.
Haha, I was pondering whether I should make a comment on that - it did seem like a lot of formatting without mentioning it :D
The bt_csip_set_member_register kept a counter that was not decreased when bt_csip_set_member_unregister was called. This meant that we could register and unregister CSIS, but we could not re-register once it had been unregistered. This commit fixes this by removing the counter and instead rely on the service instance state, which also requires restoring the original service definition, as well as adding a test that would have failed with the previous version. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>
Thalley
force-pushed
the
csip_register_fix
branch
from
b736abf to
efe484c
Compare
|
The bt_csip_set_member_register kept a counter that was not decreased when bt_csip_set_member_unregister was called. This meant that we could register and unregister CSIS, but we could not re-register once it had been unregistered.
This commit fixes this by removing the counter and instead rely on the service instance state, which also requires restoring the original service definition, as well as adding a test that would have failed with the previous version.