lib: bluetooth: ble_adv: change error codes to nrf_error#436
Conversation
|
You can find the documentation preview for this PR here. |
c94cb69 to
6fc813d
Compare
6fc813d to
c2fa6b6
Compare
|
|
||
| if (err) { | ||
| LOG_ERR("Failed to initialize advertising, err %d", err); | ||
| if (nrf_err != NRF_SUCCESS) { |
There was a problem hiding this comment.
Do we need to be this explicit? I think it's easier to read if (nrf_err)
There was a problem hiding this comment.
I think we should for good practice, as we don't have full control over the nrf_error values (although in practice it is very unlikely that NRF_SUCCESS will change). It also makes a clear distinction between errnos and nrf_errors.
There was a problem hiding this comment.
I think we can safely stick to the simpler:
if (nrf_err) {
}
|
|
||
| if (err) { | ||
| LOG_ERR("Failed to initialize advertising, err %d", err); | ||
| if (nrf_err != NRF_SUCCESS) { |
There was a problem hiding this comment.
I think we can safely stick to the simpler:
if (nrf_err) {
}
There was a problem hiding this comment.
Why are you changing this file?
There was a problem hiding this comment.
Should not be part of it...
| /* Check parameter consistency */ | ||
| if (ble_adv_data->srv_list.service == NULL) { | ||
| return -EFAULT; | ||
| return NRF_ERROR_INVALID_PARAM; |
| if (service_data->len > 0) { | ||
| if (service_data->data == NULL) { | ||
| return -EINVAL; | ||
| return NRF_ERROR_INVALID_PARAM; |
There was a problem hiding this comment.
Use NRF_ERROR_NULL when a check against NULL fails
There was a problem hiding this comment.
I am think we should stick to what is returned in the nRF5 SDK here, and other similar places. If changed to NRF_ERROR_NULL, the retval description for NRF_ERROR_NULL would also need to change for function ble_adv_data_encode().
There was a problem hiding this comment.
OK. Not a big deal, _INVALID_PARAM also works fine considering that all these pointers are optional, and in this case data could be NULL, if len wasn't also 0. An argument can be made that the error is len being non-zero, instead of data being NULL :)
| { | ||
| #if CONFIG_BLE_ADV_FAST_ADVERTISING | ||
| int err; | ||
| uint32_t err; |
There was a problem hiding this comment.
We should decide whether we want to store nrf_error values into nrf_err or whether it can be called generically err where there is only one error.
There was a problem hiding this comment.
Yes, I was thinking about that as well. For now I've only updated in the samples where it collides.
031b124 to
89653ba
Compare
89653ba to
ff36289
Compare
| err = ble_adv_start(ble_adv, BLE_ADV_MODE_DIRECTED_HIGH_DUTY); | ||
| if (err) { | ||
| nrf_err = ble_adv_start(ble_adv, BLE_ADV_MODE_DIRECTED_HIGH_DUTY); | ||
| if (nrf_err != NRF_SUCCESS) { |
There was a problem hiding this comment.
I don't think we should be checking explictly against NRF_SUCCESS, it creates lots of noise for nothing.
There was a problem hiding this comment.
Missed this here, will update.
ff36289 to
9c3c4e3
Compare
9c3c4e3 to
d1c16d2
Compare
| if (service_data->len > 0) { | ||
| if (service_data->data == NULL) { | ||
| return -EINVAL; | ||
| return NRF_ERROR_INVALID_PARAM; |
There was a problem hiding this comment.
I am think we should stick to what is returned in the nRF5 SDK here, and other similar places. If changed to NRF_ERROR_NULL, the retval description for NRF_ERROR_NULL would also need to change for function ble_adv_data_encode().
d1c16d2 to
315ed52
Compare
anhmolt
left a comment
There was a problem hiding this comment.
Fix the comments and I'm happy with this.
315ed52 to
aacb251
Compare
Happily addressed. |
|
@nrfconnect/ncs-eris @nrfconnect/ncs-bm-doc Please review. |
| if (service_data->len > 0) { | ||
| if (service_data->data == NULL) { | ||
| return -EINVAL; | ||
| return NRF_ERROR_INVALID_PARAM; |
There was a problem hiding this comment.
OK. Not a big deal, _INVALID_PARAM also works fine considering that all these pointers are optional, and in this case data could be NULL, if len wasn't also 0. An argument can be made that the error is len being non-zero, instead of data being NULL :)
aacb251 to
f3bbe57
Compare
|
|
||
| * Updated the following libraries to return ``nrf_errors`` instead of ``errnos``: | ||
|
|
||
| * BLE Advertising library. |
There was a problem hiding this comment.
| * BLE Advertising library. | |
| * :ref:`lib_ble_adv`. |
f3bbe57 to
d7640af
Compare
Change error codes to nrf_errors. Signed-off-by: Eivind Jølsgard <eivind.jolsgard@nordicsemi.no>
d7640af to
77a3ff2
Compare
Change error codes to nrf_errors.
This is the first PR for updating the error codes for BLE libraries. Starting with the ble_adv library.
Moving forward all non-BLE libraries and components will return errnos, while BLE related code as BLE services, libraries and the SoftDevice will return nrf_errors.