-
Notifications
You must be signed in to change notification settings - Fork 397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
nimble/audio/bass: Modify add source operation in BASS #1843
nimble/audio/bass: Modify add source operation in BASS #1843
Conversation
/** BIS Synchronisation of subgroups */ | ||
uint32_t bis_sync[BLE_SVC_AUDIO_BASS_SUB_NUM_MAX] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to me not needed, as the bis_sync is passed in struct ble_svc_audio_bass_subgroup.bis_sync_state
(1).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
operation.add_source.bis_sync[i] = get_le32(&data[offset]); | ||
operation.add_source.subgroups[i].bis_sync_state = get_le32(&data[offset]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(1) here. See, the &data[offset]
is passed in two variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, removed.
if (operation.add_source.adv_addr.type < 0 || | ||
operation.add_source.adv_addr.type > 3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type won't be < 0 as it's uint8. I wonder whether we need such check in this place. I there any test that test for such scenario? Maybe we could have a assert check defined globally in ble.h so that you could use it in this place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check if current check looks good. :)
memcpy(operation.add_source.adv_addr.val, &data[offset], 6); | ||
offset += 6; | ||
operation.add_source.adv_sid = data[offset++]; | ||
if (operation.add_source.adv_sid < 0 || operation.add_source.adv_sid > 0x0f) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adv_sid is unsigned value, so < 0
check is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, also added a specified define for max allowed value so this won't be checked against magic number.
72172bb
to
9a16147
Compare
rcv_state->state.source_adv_sid = operation.add_source.adv_sid; | ||
rcv_state->state.broadcast_id = operation.add_source.broadcast_id; | ||
rcv_state->state.pa_sync_state = operation.add_source.pa_sync; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe delegator should set it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
rcv_state->state.source_adv_sid = operation.add_source.adv_sid; | ||
rcv_state->state.broadcast_id = operation.add_source.broadcast_id; | ||
rcv_state->state.pa_sync_state = operation.add_source.pa_sync; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be removed - sync delegator handles this.
9a16147
to
f9be809
Compare
goto done; | ||
} | ||
|
||
if (data_len != 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it pass a test where broadcaster has more than 1 subgroup ? I think that it will not work due to this check here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
f9be809
to
54b0ef6
Compare
Fix memcpy wrong field format from address. Move check_bis_sync up, so add source can use it. Add checks for address type. Add checks for advertising sid. Add pa_sync_state and num of subgroups to receive state.
54b0ef6
to
3cb233e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Modify add source with new mbuf data style format. Add BIS sync information to receive state.
Fix memcpy wrong field format from address.
Move check_bis_sync up, so add source can use it.
Add checks for address type.
Add checks for advertising sid.
Add pa_sync_state and num of subgroups to receive state.