ARMmbed / ble

API to abstract working with Bluetooth Smart Controllers
Other
80 stars 75 forks source link

All BLE event handling should happen in thread mode #89

Closed betzw closed 8 years ago

betzw commented 8 years ago

Hi @rgrover, I have the impression that the current version of ble is not "really" ported to mbedOS/minar as it still makes use of the "old" interfaces of Tickers and callbacks which get then executed in interupt context (e.g. https://github.com/ARMmbed/ble/blob/master/ble/services/EddystoneService.h#L490). Can you confirm this and is there a version of the BLE stack which would use minar callbacks instaed?

rgrover commented 8 years ago

@betzw There is only one version of ble. And we intend to keep it compatible both with mbedOS and mbed-classic. Use of minar to schedule callbacks would violate compatibility with mbed-classic. Tickers are abstractions that remain alive across mbed-classic and mbed OS.

The fact is that the current Ticker implementation in mbed OS uses interrupt context (like it used to in mbed-classic). If Ticker is used in conjunction with BLE in an application, it could result in race conditions where BLE stack callbacks are executed in thread mode while the Ticker callbacks get to run in handler mode. If the Ticker callbacks use BLE_API, it could mess the system.

Thanks for pointing this out.

Ticker can't be implemented using minar because Ticker requires microsecond resolution (for legacy reasons), but minar can only work with millisecond resolution.

We need to implement a cross platform Ticker (or low-power ticker) which can be used in both mbed-classic and mbed OS.

Thanks so much.

betzw commented 8 years ago

Yes, this is exactly what happens with the st-ble-shield, it gets messed up :( So do you have any concrete plans to implement tickers which do (at least in mbedOS) not execute their callbacks in interrupt context?

rgrover commented 8 years ago

I believe applications needing periodic callbacks can easily switch to Minar supported callbacks for mbed OS. We'll be updating EddystoneService (and the likes) to not use Ticker; or at least not call BLE_API from Ticker handlers.

betzw commented 8 years ago

Sounds great! Pls. let us know when we can try these modifications.

@screamerbg

rgrover commented 8 years ago

One of my colleagues is working on a version of Eddystone which doesn't use the Ticker for BLE_API calls. I'm sure he would send us an update when he has something to share.

rainierwolfcastle commented 8 years ago

ARM Internal Ref: IOTSFW-1099

rgrover commented 8 years ago

Radio-notification handling is now done at a lower priority. They post events to MINAR. Refer to https://github.com/ARMmbed/ble-nrf51822/blob/develop/source/nRF5xGap.h#L114 @note: this is still in the develop branch of ble-nrf51822. @betzw : Please confirm that this solves your issue.

rgrover commented 8 years ago

In our recent posts for this issue, we did not clarify how RadioNotification callbacks are related to this issue. Apologies. Our goal is to provide an alternate implementation for Eddystone which doesn't rely upon Ticker to call into BLE_API. This updated implementation uses the radioNotificationCallback, which uncovered another issue with their use on the Nordic platform.

We've now resolved the problem with Radio Notification, and are about to release this new version of Eddystone.

betzw commented 8 years ago

@rgrover could you pls. clarify which repo/branch I should test?

ghost commented 8 years ago

@betz develop branch of https://github.com/ARMmbed/ble-nrf51822/

rgrover commented 8 years ago

I'll soon be publishing an updated version of Eddystone developed by @andresag01. It should arrive under https://github.com/ARMmbed/ble-examples; and eventually under https://github.com/ARMmbed/ble Presently it is a pull request for ble-examples.

betzw commented 8 years ago

Looking at the code at https://github.com/ARMmbed/ble-nrf51822/blob/develop/source/nRF5xGap.h#L114 to me this seems to be in the right direction. But how can I test it and confirm that it resolves the issue on our expansion board?

ghost commented 8 years ago

by replacing your version of ble-nr51822 with the one from the develop branch.

betzw commented 8 years ago

Are you kidding?

ghost commented 8 years ago

no? you configure your your module.json for ble-nrf51822 like so

"dependencies": { "ble-nrf51822": "ARMmbed/ble-nrf51822#develop" }

betzw commented 8 years ago

I do not have a nrf51822.

rgrover commented 8 years ago

@betzw as long as your port of BLE handles radio-notification events in a CriticalSection-safe manner, you're fine. You don't need to test this against Nordic if you don't have an nRF51.

rgrover commented 8 years ago

@betzw as a general guide for porters of BLE_API: BLE stack generated interrupts should respect the HAL's CriticalSection; if any interrupt violates this expectation, then mbedOS APIs shouldn't be called directly from such a handler.

betzw commented 8 years ago

OK, but how can I test it on our x-nucleo-idb04a1? I.e. should I wait for you to merge it into ARMmbed/ble-examples and/or ARMmbed/ble?

rgrover commented 8 years ago

@betzw you'll soon find an update version of Eddystone under ble-examples. Please test that.

andresag01 commented 8 years ago

@betzw We have made a new implementation of EddystoneService that does not use any Timeout or Ticker callbacks when used with mbedOS. Please use this one on your tests. It can be found on ble-examples.

andresag01 commented 8 years ago

@betzw to avoid any confusion, we have consolidated the Eddystone examples in ARMmbed/ble-examples repo master branch. We have removed all the "old" examples that used the previous implementation (i.e. BLE_EddystoneBeacon and BLE_EddystonBeaconConfigService) and replaced them with the example using the new Eddystone implementation that can be found under BLE_EddystoneService. Please use the code in BLE_EddystoneService for any tests from now on.

betzw commented 8 years ago

Seems as if there is a nrf dependence in the code:

/mnt/disk2/betzw/Projects/mbed/mbed_3.0/yotta_my/apps/ble-examples/BLE_EddystoneService/source/nrfConfigParamsPersistence.cpp:17:22: fatal error: pstorage.h: No such file or directory
 #include "pstorage.h"
                      ^
compilation terminated.
rgrover commented 8 years ago

@andresag01: could you remove persistence from the demo?

andresag01 commented 8 years ago

@rgrover @betzw Dependencies on nrf51 modules have been removed from the example.

rgrover commented 8 years ago

@andresag01 :+1:

betzw commented 8 years ago

OK, seems to work fine. Thanks!

seajayshore commented 8 years ago

Hi there, sorry to comment on a closed issue like this.

There are multiple seemingly related issues with using mbed-classic Tickers & the BLE_API / Softdevice on the Nordic devices where basically, after some time, the ticker interrupts stop firing and the application never returns to main() (but the Softdevice keeps running happily).

I've tried to get to the bottom of it but I'm not so familiar with the libraries & I've found myself here reading about this Ticker/Timeout usage within the BLE_API callbacks.

Could anyone more knowledgeable please say if they think these such issues are due to the mbed-classic BLE_API problems described in this thread above?

Issues links: https://developer.mbed.org/questions/69710/Tickers-crashhang-on-the-nRF51822-especi/ https://github.com/mbedmicro/mbed/issues/1533

I'd massively appreciate anyone helping on this. :-)

pan- commented 8 years ago

@seajayshore I'm able to reproduce the ticker issue on mbed OS, I don't think it is related to a race condition due to a ticker but rather to an internal overflow in the ticker. I'm still investigating it.