BLE: Nordic pal client implementation #5739
Conversation
|
@nvlsianpu @paul-szczepanek-arm Could you review ? |
| GattProcedure* _procedures[max_procedures_count]; | ||
| }; | ||
|
|
||
| } // cordio |
| static const size_t max_procedures_count = | ||
| CENTRAL_LINK_COUNT + PERIPHERAL_LINK_COUNT; | ||
|
|
||
| // Note: Ideally we would have used an array of variant here |
There was a problem hiding this comment.
I don't understand what this comment means, is this a todo?
There was a problem hiding this comment.
Not really a todo, I won't implement a compliant variant in C++03; it requires an heavy TMP machinery. However if we have a variant type added in mbed platform then pointers can be replaced by a variant.
| * The concept of procedures for Gatt operations formalize the process. A | ||
| * procedure lifecycle is defined by three function: | ||
| * - start: Launch of the procedure. It initiate the first request to send to | ||
| * the peer. It must be implemented by Procedure childs. |
There was a problem hiding this comment.
childs -> derived classes
| characteristic_value_handle, | ||
| /* offset */ 0, | ||
| static_cast<uint16_t>(value.size()), | ||
| const_cast<uint8_t*>(value.data()) |
There was a problem hiding this comment.
what's with the juggling of consts? is const or not?
There was a problem hiding this comment.
The nordic API is not const correct; the data passed in the write params won't be modified by the API.
| /** | ||
| * A regular procedure is a procedure that follows Gatt specification. | ||
| * | ||
| * It initiate a single request to the peer and except a single response. This |
|
|
||
| const ble_gattc_evt_read_rsp_t &rsp = evt.params.read_rsp; | ||
|
|
||
| uint16_t expected_handle = bytes_to_u16(&response[idx][0]); |
There was a problem hiding this comment.
do all those numbers like 0, 16, 4 mean something? maybe they should be consts
There was a problem hiding this comment.
Offset 0 is explicit enough otherwise we can add static const variable for loong uuid length (16) and long uuid offset in read by group type response.
| /** | ||
| * Extract an uint16_t value from a memory location. | ||
| */ | ||
| static uint16_t bytes_to_u16(const void* b) |
There was a problem hiding this comment.
why is this needed? alignment problems?
There was a problem hiding this comment.
Yes, to avoid alignment issues. Load instructions works with unaligned data however it is not the case for Load Multiple instructions. Depending on the code, the compiler is free to use LDM instructions which raise a processor exception on unaligned data. Since it is compiler dependent, an update of the compiler, or a change in optimizations flags may issue code that don't work anymore.
|
I'm really happy seeing this :D I hope I'll find time too look on this soon. |
| abort(); | ||
| } | ||
|
|
||
| uint8_t *it = buffer; |
There was a problem hiding this comment.
is this supposed to continue past abort()?
There was a problem hiding this comment.
There are a few more aborts that continue, you might want to have a quick look
* invalid namespace name documentation * vocabulary * typo * Add constants to improve readability * Fix abort usages
|
@paul-szczepanek-arm I addressed your comments. |
|
/morph build |
Build : FAILUREBuild number : 766 |
|
@pan- Some more work on this PR is needed. Reference the last Build Failure. Several IAR targets are failing to build, and all NRF52840_DK targets failed to build as well. |
| ); | ||
|
|
||
| switch (err) { | ||
| case NRF_SUCCESS: |
There was a problem hiding this comment.
Maybe:
if (err == NRF_SUCCESS) {
nordic_uuid.uuid = bytes_to_u16(uuid.getBaseUUID() + 12);
}
return convert_sd_error(err);
|
|
||
| ble_error_t nRF5XGattClient::exchange_mtu(connection_handle_t connection) | ||
| { | ||
| // Note: Not supported by the current softdevice |
There was a problem hiding this comment.
Not supported by SD 13x 2.x.x.
Is supported by SD 140 5.x.x (call sd_ble_gattc_exchange_mtu_request)
| } | ||
| } | ||
|
|
||
| typedef uint8_t packed_discovery_response_t[20]; |
There was a problem hiding this comment.
What is the cause for have 20 here?
There was a problem hiding this comment.
Attribute data present in the read by group type response will be 20 bytes long:
- Attribute handle: 2 bytes
- End group handle: 2 bytes
- Attribute value (128bits UUID): 16 bytes
I will clarify the code.
|
@pan- Can you point me how are implemented discovery of characteristic/service of long UUID? I mean the difference between API of SD v 2.x.xx and 5.x.x. For old-port was implemented here https://github.com/ARMmbed/mbed-os/blob/master/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NRF5/source/nRF5xServiceDiscovery.cpp#L292 and few line below. |
|
@pan- #5739 (comment) is not contended. |
|
@nvlsianpu I'm not sure to understand: Incompatibilities between softdevices is handled by e9ba841 . |
Test : SUCCESSBuild number : 668 |
Exporter Build : FAILUREBuild number : 488 |
|
In old port sd_ble_gattc_evt_char_val_by_uuid_read_rsp_iter is used for API >=3. I don't see this here. |
|
@nvlsianpu |
|
thanks for explanation - I had limited timed for dig in. So PR LGTM. |
|
/morph export-build |
Exporter Build : FAILUREBuild number : 491 |
|
CI issue the latest one /morph export-build |
Exporter Build : SUCCESSBuild number : 493 |
Read By group type response can return 4 descriptor discovered when the remote server have 4 descriptors with a 16 bit UUID. The handle, UUID pair get stored in a ble_gattc_desc_t that is 20 bytes long. This PR increase buffer size to handle this use case.
|
BLE testing revealed few issues with this pr: the reset strategy of the GattClient was not handled correctly and a bug in the Nordic SDK has been revealed. @nvlsianpu Could you review 3105327 ? |
|
looks good, I don't seem to be able to re-approve it so I guess it's all good to go already |
|
Rerunning tests due to small changes with recent commits. /morph build |
Build : SUCCESSBuild number : 837 Triggering tests/morph test |
Exporter Build : FAILUREBuild number : 500 |
|
/morph uvisor-test |
Exporter Build : SUCCESSBuild number : 502 |
Test : SUCCESSBuild number : 679 |
|
yes, it is a bug in SDK v11, it is fixed in SDK v13 (hence the change in the type holding discovered descriptors). |
Description
This patch get rid of the old client implementation present present on Nordic targets and replace it by an implementation based on the platform adaptation result. As a result all the missing GATT procedures are implemented:
The motivation behind this submission is the discovery of bugs in a new example for the GattClient ARMmbed/mbed-os-example-ble#111 ; The bug was well buried inside the logic of the previous GattClient (improper queuing and order of operations). It was easier to provide the new implementation than rewriting all the internal.
Status
READY
Migrations
NO