search

arduino / ArduinoCore-API

Hardware independent layer of the Arduino cores defining the official API
https://www.arduino.cc/reference/en/
GNU Lesser General Public License v2.1
215 stars 120 forks source link

BLE central API #7

Open sandeepmistry opened 8 years ago

sandeepmistry commented 8 years ago

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:

Other item's to discuss:

  1. Are there too many API's to access attributes of a peripheral? I've added a few to cater for various use cases, however I understand this might make documentation more difficult.
  2. Is there a nicer way to handle reading and writing attributes with values (characteristics and descriptors). See BLERemotePeripheralAttributeWithValue.h. Maybe we can re-use the UDP style, with beginWrite, Print API's, and endWrite.
  3. Class names, they are long now, mostly to avoid clashes with the BLEPeripheral API. In case there are use cases for using both peripheral and central mode at the same time in the future.

cc/ @tigoe @damellis @cmaglie @agdl

sandeepmistry commented 8 years ago

@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?

tigoe commented 8 years ago

+1 that. Don would have good feedback.

tigoe commented 8 years ago

A couple thoughts:

  1. Yes, work toward shorter class names. How much overlap is there between the central and peripheral libraries, and can they share any common transport or config functions? When it's possible to do something the same way on both, I think that might be helpful.
  2. startScanningWithDuplicates, could the WithDuplicates be a parameter of startScanning()? Is there a reason why that'd be a bad idea?

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: 

 for (int i = 0; peripheral.hasAdvertisedService(i); i++) {

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.

sandeepmistry commented 8 years ago

Thanks for the feedback!

How much overlap is there between the central and peripheral libraries, and can they share any common transport or config functions?

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 BLEPeripheral refers to the local peripheral type and BLERemoteCentral is used for centrals connected to the local peripheral. Similar for central mode, BLECentral is the local central, BLERemotePeripheral would be used for the peripheral's you are connecting to. Maybe there's a nicer term than Remote to distinguish both.

For attributes services, characteristics, and descriptors, BLEService, BLECharacteristic and BLEDescriptor could be re-used. However, it might not make sense in peripheral mode to subscribe to your own characteristic. If we go with a similar approach to above, you could have local attributes and remote attributes with different API's.

startScanningWithDuplicates, could the WithDuplicates be a parameter of startScanning()? Is there a reason why that'd be a bad idea?

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.

3/ It'd be nice to hide the references on the event handlers if possible. Is there another way to pass the peripheral in?

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.

Is there a typo there? wouldn't it be i < something in the middle?

No typo, but it's inconsistent. peripheral.advertisedServiceCount() is a better fit:

 for (int i = 0; peripheral.advertisedServiceCount(); i++) {

the current hasAdvertisedService API returns a bool to see if advertised service i exists before printing it out.

I'll hold off making the changes discussed above until we get a bit more feedback from others.

tigoe commented 8 years ago

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.

 for (int i = 0; peripheral.advertisedServiceCount(); i++

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?

sandeepmistry commented 8 years ago

We don't distinguish between local/remote clients with the client/server libraries in WiFi, do we? Maybe the same principle should apply here.

"The current CurieBLE library uses references, so we should update it as well." Yes, we should

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.

I think having the middle parameter NOT as a comparison and NOT related to i will be confusing to many coders.

Sorry, there was a typo in my updated loop snippet, I actually mean't:

 for (int i = 0; i < peripheral.advertisedServiceCount(); i++) {
  // ...
}
tigoe commented 8 years ago

+1 to all that. glad we're on the same page.

sandeepmistry commented 8 years ago

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 Stream - but also need to be careful of sizes of characteristics. For example, if the remote characteristic only expects 1 bytes, what should characteristic.write(1) do? The current approach has a characteristic.writeByte(1) to make the caller think about sizes.

Another concern is there are a lot of new API's, so the documentation will become larger.

cc/ @tigoe @cmaglie @agdl @kdecoi

tigoe commented 8 years ago

Are these breaking changes going to be back-ported to BLE-Peripheral? I would recommend doing so in the interests of compatibility.

tigoe commented 8 years ago

In the central sensorTag application, why don't you have to check that the characteristic exists before you subscribe to it?

tigoe commented 8 years ago

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.

sandeepmistry commented 8 years ago

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.

tigoe commented 8 years ago

I see some 8001’s, but not always with uno.

On Aug 30, 2016, at 3:34 PM, Sandeep Mistry notifications@github.com wrote:

Good question, I'm in two minds:

Back port them Create a new library for the nRF51 and nRF52 which includes both central and peripheral 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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/arduino/ArduinoCore-API/issues/7#issuecomment-243553606, or mute the thread https://github.com/notifications/unsubscribe-auth/AAXOnxfGrH62kJuvZGL4S6Y0MIhtW7fRks5qlIXfgaJpZM4IQc2T.

sandeepmistry commented 8 years ago

In the central sensorTag application, why don't you have to check that the characteristic exists before you subscribe to it?

I'll change it to:

  if (!simpleKeyCharacteristic) {
    Serial.println("no simple key characteristic found!");
    peripheral.disconnect();
    return;
  } else if (!simpleKeyCharacteristic.subscribe()) {
    Serial.println("subscription failed!");
    peripheral.disconnect();
    return;
  } else {
    Serial.println("Subscribed");
  }

I don't love having breaking changes, but they definitely are improvements.

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 BLEPeripheral type that uses BLE internally.

Need to think about the removing the references on callbacks, this is the trickiest.

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.

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 peripheral.discoverAttributes(), we could have another convenience API like peripheral.connectAndDiscoverAttributes().

sandeepmistry commented 8 years ago

we can keep a legacy BLEPeripheral type that uses BLE internally.

The more I think about it, this is the way to go ... best of both worlds.

sandeepmistry commented 8 years ago

I've added a BLE. setAdvertisedService(service) based on feedback from @cmaglie.

@tigoe were going to give Intel read access to this repo soon.

tigoe commented 8 years ago

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.

sandeepmistry commented 8 years ago

@tigoe going back to this:

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.

One more thought, we can potentially remove the peripheral.discoverAttributes() call. See led_control.ino for more context.

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.

tigoe commented 8 years ago

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?

sandeepmistry commented 8 years ago

Good point, I forget about the error case.

A snippet from the peripheral_explorer.ino example.

  // loop the services of the peripheral and explore each
  for (int i = 0; i < peripheral.serviceCount(); i++) {
    BLEService service = peripheral.service(i);

    exploreService(service);
  }

So peripheral.serviceCount() could return -1 on error. (0 for no services - a really odd case).

I like your approach => if attributes haven't been discovered by peripheral.discoverAttributes(), peripheral.serviceCount() (and related API's) will trigger the attribute discovery first.

tigoe commented 8 years ago

Sounds good to me.

sandeepmistry commented 8 years ago

@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

// starts scanning and filters out duplicates
BLE.startScanning();

// starts scanning for a peripheral advertising service UUID string
BLE.startScanningForServiceUuid(serviceUuid); 

// starts scanning for a peripheral advertising local name string
BLE.startScanningForLocalName(localName);

Need to also revisit how scanning without filtering for duplicates fits in to this. It would be useful when scanning for iBeacons etc.

cc/ @SidLeung @noelpaz @sgbihu @agdl

SidLeung commented 8 years ago

startScanningForServiceUuid and startScanningForServiceUuid are scanning with filter. startScanning - scan with no filter.

Liang adds comment here, post existing API here for review.

sgbihu commented 8 years ago

The below is my proposal. Please review it. If this is acceptable. We can implement in this way. /* * @brief Start scanning for peripherals without filter * @param none * @return none * @note none */ void startScanning();

/**
 * @brief   Start scanning for peripherals and filter by device name in ADV
 *
 * @param   name    The device's local name.
 *
 * @return  none
 *
 * @note  none
 */
void startScanning(String name);

/**
 * @brief   Start scanning for peripherals and filter by service in ADV
 *
 * @param   service    The service
 *
 * @return  none
 *
 * @note  none
 */
void startScanning(BLEService& service);
tigoe commented 8 years ago

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: scan() - scan for anything scan(String name) - scan for localName scan(int UUID) - scan for service UUID scan(int macAddr) - ? Ambiguous overload; is it useful at all? Maybe not.

THen retain setEventHandler and BLE.available as shown in scan.ino and scan_callback.ino?

tigoe commented 8 years ago

void startScanning(BLEService& service);

At the risk of too much repetition: get rid of the references. Always. Always. Always.

sandeepmistry commented 8 years ago

First off, is there a reason to call it startScanning() rather than just scan() (or scanFor...())?

That's good feedback, should we go with stopScan() to stop?

scan() - scan for anything scan(String name) - scan for localName scan(int UUID) - scan for service UUID scan(int macAddr) - ? Ambiguous overload; is it useful at all? Maybe not.

The underlying issue, is we are treating the UUID and address as strings, along with the name.

@sgbihu's suggestion of startScanning(BLEService& service); avoids this, but forces the user to create a BLEService object:

BLE.scanFor(BLEService("180f")); // scan for the peripherals advertising the battery service

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:

BLE.scan();
BLE.scan(int withDuplicates); // option to filter out duplicate addresses

BLE.scanForUUID(String uuid);
BLE.scanForUUID(String uuid, int withDuplicates);

BLE.scanFoName(String name);
BLE.scanFoName(String name, int withDuplicates);

BLE.scanForAddress(String address);

BLE.stopScan();
tigoe commented 8 years ago

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.

sandeepmistry commented 8 years ago

Thanks @tigoe!

I noticed there is some case inconsistency with the rest of the API's, so BLE.scanForUUID(String uuid); needs to be changed to BLE.scanForUuid(String uuid);.

How much does it add to the memory footprint to keep scanForName()?

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.

sandeepmistry commented 8 years ago

I've pushed the changes discussed above to master: https://github.com/arduino/ArduinoCore-API/commit/7a043a6e3d36dd1f1a2a4eab32bad50219797e29

sgbihu commented 8 years ago

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.

sgbihu commented 8 years ago

Another idea is use default value for the "bool withDuplicates" parameter, so we only need define one API to implement two method.

sandeepmistry commented 8 years ago

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.

Good suggestion, is there any other API's this is missing from? Like starting and stopping advertising?

Another idea is use default value for the "bool withDuplicates" parameter, so we only need define one API to implement two method

That is fine for your implementation, I'll leave them as separate in this repo. The documentation will also state both.

sandeepmistry commented 8 years ago

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.

Good suggestion, is there any other API's this is missing from? Like starting and stopping advertising?

Another idea is use default value for the "bool withDuplicates" parameter, so we only need define one API to implement two method

That is fine for your implementation, I'll leave them as separate in this repo. The documentation will also state both.

sandeepmistry commented 8 years ago

There's some inconsistency with the advertising and scanning related API's since we've introduced the changes from https://github.com/arduino/ArduinoCore-API/issues/7#issuecomment-257862809.

I propose we change:

BLE.startAdvertsing();
BLE.stopAdvertising();

to

BLE.advertse();
BLE.stopAdvertise();

I think these correlate more nicely with BLE.scan() and BLE.stopScan().

@tigoe @sgbihu @SidLeung @noelpaz @agdl any thoughts on this?

tigoe commented 8 years ago

I'm OK with those changes.

noelpaz commented 8 years ago

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

sandeepmistry commented 8 years ago

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

Yes, both were typos, I've corrected in https://github.com/arduino/ArduinoCore-API/commit/2eafbef754b4752c870ea31b2d1e3f3c2b8bc604. Also, renamed the advertising related API's as discussed above.

aster94 commented 5 years ago

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

tigoe commented 5 years ago

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?

aster94 commented 5 years ago

I completely missed the "central" from the title of this issue 😅 The link you provided and the https://www.arduino.cc/en/Reference/CurieBLE (which i guess uses the same API is perfect) thanks!