-
-
Notifications
You must be signed in to change notification settings - Fork 117
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
BLE central API #7
Comments
@mbanzi it would be valuable to also get Don Coleman's feedback on this. Is it ok to give him read access to this repo or share via another mechanism? |
+1 that. Don would have good feedback. |
A couple thoughts:
3/ It'd be nice to hide the references on the event handlers if possible. Is there another way to pass the peripheral in? In ScanCallback, I don't understand this line:
Is there a typo there? wouldn't it be i < something in the middle? Other than those notes, looks good so far. But I definitely think @cmaglie and @damellis should weigh in if they haven't already. |
Thanks for the feedback!
I'd like to as a minimum distinguish between a local and remote, peripherals and centrals. For example, if the Arduino is in peripheral mode For attributes services, characteristics, and descriptors,
I'd be up for that. I was remembering some suggestions Arturo had about the CurieIMU library using bool as an argument to enable/disable features. This rule probably doesn't apply here though.
Yes, we can change it to do that. I was trying to avoid copying peripheral instances. However, we can figure out a nicer to handle copies internally in the library. (Note: while working on this API, I was prototyping it on the BTLC1000 shield at the same time). The current CurieBLE library uses references, so we should update it as well.
No typo, but it's inconsistent.
the current I'll hold off making the changes discussed above until we get a bit more feedback from others. |
I think the local/remote thing is confusing. We don't distinguish between local/remote clients with the client/server libraries in WiFi, do we? Maybe the same principle should apply here. "However, it might not make sense in peripheral mode to subscribe to your own characteristic." You never know, someone will find a use for it. "The current CurieBLE library uses references, so we should update it as well." Yes, we should.
I think having the middle parameter NOT as a comparison and NOT related to i will be confusing to many coders. We're so used to the (i=0; i<limit; i++) model that to throw in a limit that has no relation to the iterator is confusing. I still don't understand it myself. Is there a way to phrase it that makes what it's doing more apparent? Maybe it's actually a while loop instead? |
Excellent point! I think we can make same happen on BLE then. It'll make the internal book keeping a bit trickier and probably use more RAM. However, on ARM and ARC there's much more RAM compared to AVR - we should put it to good use. I'll try to prototype universal classes for BLE central and peripheral in about 2 weeks after I wrap up my current project. Same applies for removing references. I don't expect any issues, just want to see what the performance/memory trade off will be.
Sorry, there was a typo in my updated loop snippet, I actually mean't: for (int i = 0; i < peripheral.advertisedServiceCount(); i++) {
// ...
} |
+1 to all that. glad we're on the same page. |
It's been awhile! I've pushed an updated draft to the BLE folder. It includes the items we've discussed above and an combined central + peripheral API. I would recommend taking a look at the examples folder There are a few breaking changes to the original BLEPeripheral API:
We'll need to have a sketch upgrade guide or think about adding backwards compatibility where possible. I'm still not happy with the BLEAttributeWithValue base type, need to think about using Another concern is there are a lot of new API's, so the documentation will become larger. |
Are these breaking changes going to be back-ported to BLE-Peripheral? I would recommend doing so in the interests of compatibility. |
In the central sensorTag application, why don't you have to check that the characteristic exists before you subscribe to it? |
Rest of the examples look good. I don't love having breaking changes, but they definitely are improvements. RE: central, the API makes sense, though I wish there was a way to simplify the scan() -> connect -> discover services -> discover characteristics -> take action sequence. But that is a problem of BLE, not this API. |
Good question, I'm in two minds:
The last two breaking changes will use much more RAM, so nRF8001 + Uno might not be useable. How many nRF8001 based boards do you see in your courses? I think Chi-Hung mentioned it's end of life. |
I see some 8001’s, but not always with uno.
|
I'll change it to:
Yes it's not great, maybe 1/2 or more them can be avoided by adding backwards compatible API's. They'll probably be a cost to code space and/or RAM though. For example, we can keep a legacy Need to think about the removing the references on callbacks, this is the trickiest.
Do you think we should add support to connecting to a BT address if you know it ahead of time and don't want to scan? I'm a fan of the Apple model where you always have to scan. I've combined discover service and characteristics into |
The more I think about it, this is the way to go ... best of both worlds. |
Good. I look forward to integrating all of this. When we're definitely settled on the API I might bug you to help me modify the MTT examples, though it looks like the changes will be minimal :) I like what you've done here, it's clean and simple. |
@tigoe going back to this:
One more thought, we can potentially remove the Instead, anytime you try to retrieve the service/characteristic/descriptor count or query if one exists we could send a request to the peripheral then. What do you think? It will result in one less call for the user to call. |
I like that. I assume you could still discover attributes if you want to, but you could also skip to the others if you prefer and get an error code, right? |
Good point, I forget about the error case. A snippet from the peripheral_explorer.ino example.
So I like your approach => if attributes haven't been discovered by |
Sounds good to me. |
@tigoe one thing that's missing from the current API spec is the ability to start scanning with a filter. Here's a proposal, feedback from everyone is welcome
Need to also revisit how scanning without filtering for duplicates fits in to this. It would be useful when scanning for iBeacons etc. |
startScanningForServiceUuid and startScanningForServiceUuid are scanning with filter. startScanning - scan with no filter. Liang adds comment here, post existing API here for review. |
The below is my proposal. Please review it. If this is acceptable. We can implement in this way.
|
I still feel like it's better to overload the startScanning() function to support either of the other two. @damellis might have good reason why not though. I can see common uses of all three, but the function names are rather long. First off, is there a reason to call it startScanning() rather than just scan() (or scanFor...())? Maybe: THen retain setEventHandler and BLE.available as shown in scan.ino and scan_callback.ino? |
At the risk of too much repetition: get rid of the references. Always. Always. Always. |
That's good feedback, should we go with
The underlying issue, is we are treating the UUID and address as strings, along with the name. @sgbihu's suggestion of
Another question, is do we really need to support built-in filtering by local name, is service UUID enough? (CoreBluetooth on OS X/iOS just has the option for scan for UUID, not name). Here's an updated proposal, assuming we want to keep scanning by name:
|
I like your updated proposal @sandeepmistry. Solves all the issues, I think, pretty well. How much does it add to the memory footprint to keep scanForName()? We may not really need it, but it is convenient. If it's a big memory hog, we could kill it, but if it's not a big deal, I'd keep it. |
Thanks @tigoe! I noticed there is some case inconsistency with the rest of the API's, so
The max local name is 29 bytes, if I'm not mistaken. So I think it's ok to keep. @sgbihu @SidLeung @noelpaz does the above proposal look good to you? Do you have any feedback. Let me know, then I can update the headers and examples in this repo. |
I've pushed the changes discussed above to master: 7a043a6 |
I suggest to add return value to indicate the scan success or failed. Or the user doesn't know the scan parameter is success. bool is Ok for sketches. |
Another idea is use default value for the "bool withDuplicates" parameter, so we only need define one API to implement two method. |
Good suggestion, is there any other API's this is missing from? Like starting and stopping advertising?
That is fine for your implementation, I'll leave them as separate in this repo. The documentation will also state both. |
1 similar comment
Good suggestion, is there any other API's this is missing from? Like starting and stopping advertising?
That is fine for your implementation, I'll leave them as separate in this repo. The documentation will also state both. |
There's some inconsistency with the advertising and scanning related API's since we've introduced the changes from #7 (comment). I propose we change:
to
I think these correlate more nicely with @tigoe @sgbihu @SidLeung @noelpaz @agdl any thoughts on this? |
I'm OK with those changes. |
You mean BLE.advertise(); was that a typo or intentional to lose the i. I also noticed scanFoName () and I know @sgbihu kept it that way as well in our version Anyway I am fine with keeping the tense of the verbs in functions uniform. Maybe you can scan the API names and keep the consistency |
do you think that the BLE api would be ready anytime soon? because the ESP32 guys are working on it (https://github.com/nkolban/ESP32_BLE_Arduino) and i think that if arduino.cc doesn't have an api it will end up the same as the HTTPClient where everyone made different functions |
Do you mean this? https://github.com/arduino-libraries/Arduinoble . It's been out for a few months, and is plenty stable. Or are you referring to the central API specifically? |
I completely missed the "central" from the title of this issue 😅 |
Based on the outcome of the API meeting, I've added a first draft of a BLE central API in the api/BLE/central folder.
It's probably best to review the examples in the following order:
scan.ino
but callback styleOther item's to discuss:
beginWrite
,Print
API's, andendWrite
.cc/ @tigoe @damellis @cmaglie @agdl
The text was updated successfully, but these errors were encountered: