BLE: feature gatt client - Generic GattClient implementation #4781
Conversation
Base types like the connection handle type or the attribute handle used accross all the layers were defined in the upper layer. It is not right to makes lower layers needed those types dependent on the upper layer. This change introduce a new header defining those base types shared accross high and lovel layer.
In case of faillure during descriptor discovery, the error code can help the user to determine the cause of the error.
|
@apalmieriGH @nvlsianpu Could you have a look at this PR ? It should help you in the process of providing a |
* Include necessary AttServerMessage header. * Force AttClient::write_request to be pure virtual member function.
… property construction.
|
@pan- Thanks for the PR. Because we use the PR subject titles in our release notes, could you please change your subject line to imperative mood? We recommend changing it to "BLE: implement generic GattClient". Thanks so much. |
|
@pan- any news? |
|
@theotherjimmy Not from our ble partners unfortunately. |
|
@pan I will review this until end of week. |
update for the rest soon |
|
@nvlsianpu Thanks for your comments.
A connection handle, in HCI term, is a 16 bit integer (ranging from 0x0000 to 0x0EFF). Unfortunately, this might not be the same thing for some stack (it might refer to an opaque pointer for instance). That's the reason behind the change.
At some points, we will have to split out event associated with an ATT request from event associated to an ATT response. Historically, they have been the same and it felt just wrong once we add error handling in the equation. Some fields, like data in GattWriteCallbackParams also doesn't make sense in the response.
Yes I will change the name of the file. |
|
I reviewed (almost) GenericGattClient.cpp - I have some remarks: I think It would be nice if callback chains of procedures were documented (especially for discovery process ) - this is minor wish only - but I think It would simplify maintenance further as well. |
These procedures are not yet defined in the
The procedures to be added are:
I agree, could you list what interaction diagram you would like to have in the documentation ? |
|
@pan Thanks for answer. I'm done with review this PR. Great job - I really like how GenaricGattClientit is written and organized.
message or state diagram for launchServiceDiscovery implementation would be OK.
Unless it works properly it is OK to have it as is. I agree - fragmented discovery will be more useful for real application. My opinion is that the Discovery of whole device should be moved to an example or a library. |
I can do that, basically it will model behavior described in the porting guide.
I was thinking of deprecating it, not removing it. My main issue with that function is its complexity. It also hides to the user which procedure is really used and it is a bad thing if you want to tailored your application to consume less energy. Direct access to the standard procedure is, I believe, the way to go. I'd like to see examples demonstrating the different kind of discovery available while explaining clearly the use case for each and the impact on power when one or the other is used. |
|
Prose is OK as well (even without diagrams). What I had as idea is to have states, messages, request, and respond names used in documentation - which makes geting into the code more harmless. |
| namespace ble { | ||
| namespace generic { | ||
|
|
||
| // forward declaration |
| uint16_t GenericGattClient::get_mtu(Gap::Handle_t connection) const { | ||
| uint16_t result = 23; | ||
| if(_pal_client->get_mtu_size((connection_handle_t) connection, result) != BLE_ERROR_NONE) { | ||
| result = 23; |
There was a problem hiding this comment.
I prefer assertion which hang process upon error. In case you want to have default value for mtu size I prefer to have virtual method with default implementation which return 23 instead of abstract one already implemented.
There was a problem hiding this comment.
Comment it is related to every get_mtu in this file.
|
|
||
| memcpy(data + (current_offset - offset), read_response.data(), read_response.size()); | ||
| current_offset = current_offset + read_response.size(); | ||
| client->_pal_client->read_attribute_blob( |
There was a problem hiding this comment.
I recommend error handling as for initial read request (+ resource free)
| break; | ||
|
|
||
| default: | ||
| // error |
| break; | ||
|
|
||
| default: | ||
| // error |
| static DiscoveredCharacteristic::Properties_t get_properties(const ArrayView<const uint8_t>& value) { | ||
| uint8_t raw_properties = value[0]; | ||
| DiscoveredCharacteristic::Properties_t result; | ||
| result._broadcast = raw_properties && (1 << 0); |
There was a problem hiding this comment.
here and next few lines & instead of &&
| if (last_characteristic.getValueHandle() == services_discovered->end) { | ||
| handle_all_characteristics_discovered(client); | ||
| } else { | ||
| client->_pal_client->discover_characteristics_of_a_service( |
There was a problem hiding this comment.
need error service here and around
|
|
||
| virtual ~descriptor_discovery_control_block_t() { } | ||
|
|
||
| ble_error_t start(GenericGattClient* client) { |
There was a problem hiding this comment.
Nice - what about a start method for other xxx_control_block_t - a see at last pros of coherence
|
@pan- any news? |
|
@pan- Bump |
|
I close this, comments have been addressed in #5060. |
Description
This PR adds a generic implementation of the GattClient. This implementation relies on the
pal::GattClientbeing provided by the vendor.Status
READY
Migrations
NO