✨ (ble): Add CoreGattServerEventHandler #490
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## develop #490 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 83 87 +4
Lines 1351 1395 +44
=========================================
+ Hits 1351 1395 +44
Continue to review full report at Codecov.
|
File comparision analysis report🔖 Info
Click to show memory sections📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
File comparision analysis report🔖 Info
Click to show memory sections📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
YannLocatelli
force-pushed
the
yann/feature/ble/gatt/add-core-gatt-server-event-handler
branch
from
3626803
to
e975d48
Compare
ladislas
requested changes
Feb 16, 2022
Comment on lines
15
to
16
| using updateServiceFunction = | ||
| std::function<void(GattAttribute::Handle_t characteristic_updated, const uint8_t *data, uint16_t n_data_bytes)>; |
There was a problem hiding this comment.
je suis pas super fan que cet alias soit dans le global namespace leka, elle peut être mise dans l'interface ou la class?
There was a problem hiding this comment.
je la renommerai pour bien faire comprendre que c'est un type
Suggested change
| using updateServiceFunction = | |
| std::function<void(GattAttribute::Handle_t characteristic_updated, const uint8_t *data, uint16_t n_data_bytes)>; | |
| using update_service_function_t = | |
| std::function<void(GattAttribute::Handle_t handle, const uint8_t *data, uint16_t size)>; |
et elle dit service alors qu'elle est utilisée dans updateData --> peut être qu'il faut harmoniser les noms utilisés pour simplifier la compréhension
There was a problem hiding this comment.
et elle peut pas prendre un span plutôt qu'un * et un size?
There was a problem hiding this comment.
C'est déplacé
--
L'idée du span est intéressant mais il y a un bémol dans l'usage: parfois uint8_t *data pointe vers une variable de type uint8_t ou bool, donc pas nécessairement un type array
There was a problem hiding this comment.
c'est pas très grave si c'est pas un array, si c'est juste une variable, tu peux faire passer std::span{&i, 1}
exemple: https://gcc.godbolt.org/z/nhPY7jKj7
Comment on lines
23
to
24
| BLEService(const UUID &uuid, std::span<GattCharacteristic *> characteristics, unsigned characteristics_count) | ||
| : GattService(uuid, characteristics.data(), characteristics_count) {}; |
There was a problem hiding this comment.
est-ce qu'on se mettrait pas const UUID &uuid, std::span<GattCharacteristic *> characteristics, unsigned characteristics_count dans une struct?
There was a problem hiding this comment.
à minima
Suggested change
| BLEService(const UUID &uuid, std::span<GattCharacteristic *> characteristics, unsigned characteristics_count) | |
| : GattService(uuid, characteristics.data(), characteristics_count) {}; | |
| BLEService(const UUID &uuid, std::span<GattCharacteristic *> characteristics) | |
| : GattService(uuid, characteristics.data(), std::size(characteristics) {}; |
There was a problem hiding this comment.
Le characteristics_count a été retiré
Comment on lines
26
to
27
| virtual void onDataWritten(const GattWriteCallbackParams ¶ms) = 0; | ||
| virtual void updateData(updateServiceFunction &update) = 0; |
There was a problem hiding this comment.
je pense que tu as même pas besoin de virtual ici, à moins de vouloir savoir avec quoi elles sont appelées et encore.
tu peux juste avoir ta struct de base qui implémente rien (ça donnera un linker error si tu essaye de l'utiliser) et ta classe qui implémente un service qui elle donnera l'implementation concrète
comme ici https://gcc.godbolt.org/z/zbhafPs5o
Comment on lines
82
to
95
| const UUID uuid {0x0001}; | ||
| static const uint8_t serivce_1_characteristics_count {1}; | ||
| static const uint8_t serivce_2_characteristics_count {2}; | ||
| static const uint8_t services_count {2}; | ||
| uint8_t characteristic_value {}; | ||
| ReadOnlyGattCharacteristic<uint8_t> writable_characteristic {0x1234, &characteristic_value}; | ||
| std::array<GattCharacteristic *, serivce_1_characteristics_count> service_1_characteristic_table { | ||
| &writable_characteristic}; | ||
| std::array<GattCharacteristic *, serivce_2_characteristics_count> service_2_characteristic_table { | ||
| &writable_characteristic, &writable_characteristic}; | ||
| mock::BLEService mock_service_1 = | ||
| mock::BLEService(uuid, service_1_characteristic_table, serivce_1_characteristics_count); | ||
| mock::BLEService mock_service_2 = | ||
| mock::BLEService(uuid, service_2_characteristic_table, serivce_2_characteristics_count); |
There was a problem hiding this comment.
c'est très dur à lire et à suivre.
il faut mettre un peu plus d'auto et std::to_array et raccourcir le nom des variables.
Suggested change
| const UUID uuid {0x0001}; | |
| static const uint8_t serivce_1_characteristics_count {1}; | |
| static const uint8_t serivce_2_characteristics_count {2}; | |
| static const uint8_t services_count {2}; | |
| uint8_t characteristic_value {}; | |
| ReadOnlyGattCharacteristic<uint8_t> writable_characteristic {0x1234, &characteristic_value}; | |
| std::array<GattCharacteristic *, serivce_1_characteristics_count> service_1_characteristic_table { | |
| &writable_characteristic}; | |
| std::array<GattCharacteristic *, serivce_2_characteristics_count> service_2_characteristic_table { | |
| &writable_characteristic, &writable_characteristic}; | |
| mock::BLEService mock_service_1 = | |
| mock::BLEService(uuid, service_1_characteristic_table, serivce_1_characteristics_count); | |
| mock::BLEService mock_service_2 = | |
| mock::BLEService(uuid, service_2_characteristic_table, serivce_2_characteristics_count); | |
| ReadOnlyGattCharacteristic<uint8_t> charac {0x1234, 0}; | |
| auto s1_charac_table = std::to_array<GattCharacteristic *>({&charac}); // ? One characteristic | |
| auto s2_charac_table = std::to_array<GattCharacteristic *>({&charac, &charac}); // ? Two characteristics | |
| const UUID s1_uuid {0x0001}; | |
| auto s1 = mock::BLEService(s1_uuid, s1_charac_table, std::size(s1_charac_table); | |
| const UUID s2_uuid {0x0002}; | |
| auto s1 = mock::BLEService(s2_uuid, s2_charac_table, std::size(s2_charac_table); |
YannLocatelli
force-pushed
the
yann/feature/ble/gatt/add-core-gatt-server-event-handler
branch
from
e9807d0
to
63242a0
Compare
YannLocatelli
force-pushed
the
yann/feature/ble/gatt/add-core-gatt-server-event-handler
branch
from
63242a0
to
f800cd6
Compare
ladislas
requested changes
Feb 22, 2022
There was a problem hiding this comment.
c'est beaucoup plus clair, merci @YannLocatelli ! 👍
j'ai encore fait quelques retours, en particulier, j'avais du mal à voir comment BLEService allait ensuite s'intégrer avec le reste, du coup j'ai fait un exemple ici
https://gcc.godbolt.org/z/xv6dxcv1M
je continue sur slack, parce que ça m'a donné d'autres idées.
| #include "ble/gatt/GattCharacteristic.h" | ||
| #include "ble/gatt/GattService.h" | ||
|
|
||
| namespace leka::interface { |
There was a problem hiding this comment.
dans la mesure où on propose une implémentation de certaines fonctions, ce n'est pas "vraiment" une interface.
je le mettrai juste dans leka
je vois aussi qu'il est dans internal, c'est qu'il n'aura pas de visibilité public?
dans ce cas là il faudrait qu'il soit dans leka::ble::internal mais le ble risque de faire des bugs avec le ble de mbed...
ou alors le mettre dans include
There was a problem hiding this comment.
Ah oui, c'est le fait d'avoir retiré les virtual qui fait qu'on a perdu le caractère de interface
Ce qui me gène, c'est que c'est un modèle et qu'il ne devrait pas donner l'idée qu'il puisse être utilisé autre qu'en tant que modèle. C'est aussi dans ce sens que va le internal...
Mais si on regarde dans le futur avec les dernières suggestions, dans la PR #485, on est amené à instancié un array avec du interface::BLEService *
Comment on lines
27
to
30
| virtual void onDataWritten(const GattWriteCallbackParams ¶ms) = 0; | ||
|
|
||
| virtual void registerUpdateDataFunction(update_data_function_t const &function) = 0; | ||
| virtual void updateData() = 0; |
There was a problem hiding this comment.
j'ai toujours du mal avec les noms. Je trouve que onDataWritten il est pas assez spécifique parce qu'en pratique on écrit dans les deux sens : soit quelqu'un nous écrit et on lit (ce qui l'usage là) soit on "écrit" pour update dans le server.
je pense à un truc comme ça:
Suggested change
| virtual void onDataWritten(const GattWriteCallbackParams ¶ms) = 0; | |
| virtual void registerUpdateDataFunction(update_data_function_t const &function) = 0; | |
| virtual void updateData() = 0; | |
| using data_received_handle_t = GattWriteCallbackParams; | |
| using data_to_send_handle_t = std::tuple<GattAttribute::Handle_t, const uint8_t *, uint16_t> | |
| virtual void onDataReceived(const data_received_handle_t &handle) = 0; | |
| virtual void onDataReadyToSend(std::function<void(const data_to_send_handle_t &)> callback) = 0; |
j'avais du mal à comprendre après comment tout ça allait s'orchestrer entre ce que fait le service et le gattserver, du coup j'ai créé un exemple pour m'aider
There was a problem hiding this comment.
En relisant, c'est celui dans l'event handler qu'on ne peut pas le changer.
Ici comme le Service a été dissocié de l'event handler, on peut aujourd'hui lui donner le nom qu'on souhaite
Je vais me répéter mais la PR #485 permet de voir ce à quoi va ressembler un système qui utilise le server et des (un) services
Et actuellement il fait que du "setup" et n'est pas utilisé "on run": définir l'event handler pour le BLE, définir quels services vont être utilisés, définir comment écrire sur le serveur
|
Kudos, SonarCloud Quality Gate passed! |
ladislas
deleted the
yann/feature/ble/gatt/add-core-gatt-server-event-handler
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Need #486 #489