-
Notifications
You must be signed in to change notification settings - Fork 511
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add characteristic discovery to BleService #2203
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @mgolu for submitting this great PR! There is still something we can improve as I commented. If you'd like, I'm glad to take over this PR to make the final change.
Yes, great comments. I agree we can refactor, I just didn't want to touch existing APIs yet. If you have the time to work on it, go for it! |
502e15d
to
581f75a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great to save RAM by keeping just one copy of characteristics. I think the application will have to keep track of the peer associated with each service (for example, when connecting to multiple peers).
Maybe we can add a call to the BleService class that calls the peer's discoverCharacteristicsOfService? That way the application doesn't have to keep track of which peer is for each service.
The initial objective of the BLE wiring APIs are designed to be easy to use for non professional developers. Thus, we just provide two main mechanisms for BLE operation, data transaction oriented and non-data transaction oriented. For non-data transaction oriented operation, we have the |
The use case would be for an Enterprise customer who might have multiple peers connected, which might be the same Profile or type. Then, they would have multiple BleService instances with the same UUID, but related to different peers. If they want to discover characteristics on an instance of BleService, that's currently impossible since there's no API on BleService to discover characteristics, and there's no reference to the BlePeerDevice that the instance is associated with. That means that the developer needs to keep track themselves of each BleService instance association to a BlePeer. |
I see no reason to keep tracking a BLE service with a peer device in user application. For discovering characteristics under a peer associated service, we can call |
I tested with the new proposed BLE Gateway library (https://github.com/particle-iot/ble-gateway-library) and the new changes are working well. Just submitted a small fix. |
@avtolstoy Do you have any comment here? |
7a255ce
to
7f1806e
Compare
Can this branch be merged into develop? |
We will get it merged in this sprint (in two weeks). Cc. @avtolstoy |
wiring/src/spark_wiring_ble.cpp
Outdated
@@ -1001,10 +1002,24 @@ class BleServiceImpl { | |||
return endHandle_; | |||
} | |||
|
|||
bool& characteristicsDiscovered() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This is a really weird pattern when considering trivial types. I'd rather have this as a public member then or use setters/getters.
d567d5b
to
e3967bb
Compare
0abf96c
to
84ebbd9
Compare
existing char instead of passed value
84ebbd9
to
28175ba
Compare
This PR is targeting to the feature/ble_cpp_callback/ch65803 branch
Problem
Allows for hierarchical discovery of characteristics inside services.
Solution
Adds a discoverAllCharacteristics function to BleServiceImpl class.
Steps to Test
peer->discoverAllServices();
peer.getServiceByUUID(service, );
service.discoverAllCharacteristics();
Completeness