MariuszSkamra commented on code in PR #1843:
URL: https://github.com/apache/mynewt-nimble/pull/1843#discussion_r1721469106
##########
nimble/host/audio/services/bass/src/ble_audio_svc_bass.c:
##########
@@ -298,10 +320,23 @@ ble_svc_audio_bass_add_source(uint8_t *data, uint16_t
data_len, uint16_t conn_ha
operation.op = BLE_SVC_AUDIO_BASS_OPERATION_ADD_SOURCE;
operation.conn_handle = conn_handle;
+ offset++;
operation.add_source.adv_addr.type = data[offset++];
+ if (operation.add_source.adv_addr.type < 0 ||
+ operation.add_source.adv_addr.type > 3) {
+ rc = BLE_HS_EINVAL;
+ ev.bass_operation_status.status = BLE_HS_EINVAL;
+ goto done;
+ }
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) {
Review Comment:
adv_sid is unsigned value, so `< 0` check is not needed.
##########
nimble/host/audio/services/bass/src/ble_audio_svc_bass.c:
##########
@@ -326,6 +361,7 @@ ble_svc_audio_bass_add_source(uint8_t *data, uint16_t
data_len, uint16_t conn_ha
ev.bass_operation_status.status = BLE_HS_EREJECT;
goto done;
}
+ operation.add_source.bis_sync[i] = get_le32(&data[offset]);
operation.add_source.subgroups[i].bis_sync_state =
get_le32(&data[offset]);
Review Comment:
(1) here. See, the `&data[offset]` is passed in two variables.
##########
nimble/host/audio/services/bass/include/services/bass/ble_audio_svc_bass.h:
##########
@@ -286,6 +286,9 @@ struct ble_svc_audio_bass_operation {
/** Number of subgroups */
uint8_t num_subgroups;
+ /** BIS Synchronisation of subgroups */
+ uint32_t bis_sync[BLE_SVC_AUDIO_BASS_SUB_NUM_MAX]
+
Review Comment:
This seems to me not needed, as the bis_sync is passed in `struct
ble_svc_audio_bass_subgroup.bis_sync_state` (1).
##########
nimble/host/audio/services/bass/src/ble_audio_svc_bass.c:
##########
@@ -298,10 +320,23 @@ ble_svc_audio_bass_add_source(uint8_t *data, uint16_t
data_len, uint16_t conn_ha
operation.op = BLE_SVC_AUDIO_BASS_OPERATION_ADD_SOURCE;
operation.conn_handle = conn_handle;
+ offset++;
operation.add_source.adv_addr.type = data[offset++];
+ if (operation.add_source.adv_addr.type < 0 ||
+ operation.add_source.adv_addr.type > 3) {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
