BLE central API about arduinocore-api HOT 41 OPEN

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();

from arduinocore-api.

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.

from arduinocore-api.

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

from arduinocore-api.

+1 that. Don would have good feedback.

from arduinocore-api.

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.

from arduinocore-api.

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.

from arduinocore-api.

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?

from arduinocore-api.

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++) {
  // ...
}

from arduinocore-api.

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

from arduinocore-api.

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:

• Now there is a global BLE (of type BLEDevice) instance created for you. No more BLEPeripheral blePeripheral; declaration needed. BLEDevice can represent the local device or remote, some methods are no-ops depending on what it represents. For example, BLE.connect() does nothing because you can't connect to yourself.
• BLE.begin() must be called at start of sketch to initialize hardware. Then BLE.startAdvertising() needs to be called to advertise. Before blePeripheral.begin() was called last and started advertising.
• There's no more .addAttribute(...), replaced by: BLE.addService(service), service.addCharacteristic(characteristic), and characteristic.addDescriptor(descriptor)
• characteristic.setValue(...) has been renamed to characteristic.writeValue(...)
• Callback signatures no longer use references

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

from arduinocore-api.

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

from arduinocore-api.

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

from arduinocore-api.

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.

from arduinocore-api.

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.

from arduinocore-api.

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

On Aug 30, 2016, at 3:34 PM, Sandeep Mistry [email protected] 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 #7 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AAXOnxfGrH62kJuvZGL4S6Y0MIhtW7fRks5qlIXfgaJpZM4IQc2T.

from arduinocore-api.

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().

from arduinocore-api.

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.

from arduinocore-api.

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

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

from arduinocore-api.

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.

from arduinocore-api.

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

from arduinocore-api.

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?

from arduinocore-api.

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.

from arduinocore-api.

Sounds good to me.

from arduinocore-api.

@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

from arduinocore-api.

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

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

from arduinocore-api.

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);

from arduinocore-api.

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?

from arduinocore-api.

void startScanning(BLEService& service);

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

from arduinocore-api.

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.

from arduinocore-api.

I've pushed the changes discussed above to master: 7a043a6

from arduinocore-api.

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.

from arduinocore-api.

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

from arduinocore-api.

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.

from arduinocore-api.

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.

from arduinocore-api.

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:

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?

from arduinocore-api.

I'm OK with those changes.

from arduinocore-api.

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

from arduinocore-api.

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 2eafbef. Also, renamed the advertising related API's as discussed above.

from arduinocore-api.

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

from arduinocore-api.

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?

from arduinocore-api.

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!

from arduinocore-api.