[NRF52840]: SecurityManager::getAddressesFromBondTable #4178
Conversation
|
@nvlsianpu If it needs #4132 , could you indicate it in the description ? |
nvlsianpu
changed the title
|
I restarted Travis, please have a look once it finishes. As @pan- is not available these days, who could help reviewing this? |
|
@anangl Could you review the PR. |
|
@anangl Could you review? (bump) |
| * | ||
| * @return | ||
| * BLE_ERROR_NONE if successful. | ||
| */ |
There was a problem hiding this comment.
And what if not successful?
BLE_ERROR_UNSPECIFIED could be mentioned here, or at least "error code indicating reason for failure", like it is for other functions.
| if (addrList.capacity <= addrList.size) { | ||
| /* Ran out of space in the output Gap::Whitelist_t */ | ||
| return BLE_ERROR_NONE; | ||
| } |
There was a problem hiding this comment.
When there is no more space in the output list, there is no point in reading the bonding data. Hence, I'd move this checking to while condition, and change it to addrList.size < addrList.capacity (IMHO this would better express what is actually checked).
| } else { | ||
| memcpy( &addrList.addresses[addrList.size].address, | ||
| &bond_data.peer_ble_id.id_addr_info.addr, | ||
| sizeof(addrList.addresses[0].address)); |
There was a problem hiding this comment.
This indentation looks a bit strange.
There was a problem hiding this comment.
unwanted tabulation
| * | ||
| * @return | ||
| * BLE_ERROR_NONE if successful. | ||
| */ |
There was a problem hiding this comment.
Again, it would be good to mention something about what is returned when an error occurs.
| * Fill addresses list: | ||
| * Copy addresses from bond table, or | ||
| * for every private resolvable address in the bond table check if | ||
| * there is maching address in th provided whitelist and copy resolvable address. |
There was a problem hiding this comment.
Maybe I'm missing something, but I feel a bit confused with this comment. I can't see below any checking if there is a matching address. And what is the provided whitelist?
|
could you address @anangl 's comments? |
|
retest uvisor |
1 similar comment
|
retest uvisor |
|
java exception, retest uvisor |
|
retest uvisor |
…le for SD 5.x.x" to NRF5 BLE port sources.
nvlsianpu
force-pushed
the
getAddrFromPeerTab_sd_api_5
branch
from
30d7d8e to
39c1b3f
Compare
|
I rebased and redirected previous changes of NRF5_SDK13 port to NRF5 port because #4245 move nRF52840 support to common NRF5 sources and NRF5_SDK13 sources will be removed. |
|
test ble {
"targets": ["NRF51_DK", "NRF52_DK"],
"toolchains": ["GCC_ARM", "ARM"]
} |
|
Not sure why this failed, I've restarted it |
|
Any update on this? |
|
@pan- bump |
|
test ble {
"targets": ["NRF52_DK"],
"toolchains": ["GCC_ARM", "ARM"]
} |
|
@pan- it looks like the BLE tests are still failing. Any news? code freeze is tomorrow. |
|
@theotherjimmy Some tests are not supported by the Nordic target and therefore will report a failure, looking at the tests results, this PR is good to go. |
|
Thanks for the update. As this touches BLE files only, I believe morph test is not needed, thus all CI done that is required for these changes, should be good to go. |
Description
Implementation of SecurityManager::getAddressesFromBondTable (SD API <3, 5>)
Status
READY