sandeepmistry / arduino-BLEPeripheral

An Arduino library for creating custom BLE peripherals with Nordic Semiconductor's nRF8001 or nR51822.
MIT License
464 stars 180 forks source link

allow calling poll() with BLE event data #120

Closed urish closed 4 years ago

urish commented 7 years ago

Hi again Sandeep!

First of all, thank you again for your enormous contribution to the BLE community. I use your projects on a daily basis, and find them very valuable.

This pull request lets the user of the library pass the BLE event data to poll(), instead of have poll() get it directly from the BLE stack.

My use case for this is - I have an app that acts as a BLE peripheral, while at the same time scans for BLE advertisements from nearby devices. That's the reason why I need to get the raw SoftDevice events.

This is a sketch of how my code uses the proposed feature:

  uint32_t   evtBuf[(sizeof(ble_evt_t) + (GATT_MTU_SIZE_DEFAULT))];
  uint16_t   evtLen = sizeof(evtBuf);

  if (sd_ble_evt_get((uint8_t*)evtBuf, &evtLen) == NRF_SUCCESS) {
    ble_evt_t* bleEvt = (ble_evt_t*)evtBuf;
    // here I check if this is an BLE advertisement packet and handle it
  } else { 
    return;
  }

  BLECentral central = blePeripheral.central(evtBuf, evtLen);
  // do the usual stuff with the central object

So far, I have only implemented it for the NRF51 variant, as I wanted to get your feedback first, get your feedback about this approach, and see if it is heading the right direction. I would love to have some solution for my use case, and I am open for feedback on this PR before I continue working on this.

Thanks!

sandeepmistry commented 7 years ago

@urish not a bad idea, what you you think about the opposite?

Having a API in BLEPeripheral to set a callback to trigger when an event is received? This would be portable to both nRF8001 and nRF51/nRF51.

Longer term, I think a new library needs to be created for combined central and peripheral functionality. nRF8001 can be left out of the new library.

Arduino and Intel have the next generation API defined in the CurieBLE master: https://github.com/01org/corelibs-arduino101/tree/master/libraries/CurieBLE

We're still working together to refine a few things. Do you have time to start on the new library nRF51/nRF52 boards?

urish commented 7 years ago

Actually, I have implemented the opposite just before going this direction, but I liked this approach more - if we introduce an API that is triggered whenever an event is received, it would need to pass the event data to the user - and then, it would either need to always save the latest event data in some memory location, or the user will need to make sure he is not using the event data outside the handler.

As for CurieBLE - I like the approach of having a library that combines central and peripheral (AFAIK the RBL implementation does exactly that), though, I prefer the MIT license that you have here over LGPL, at least when it comes to embedded devices.

sandeepmistry commented 7 years ago

@urish ok, how do you suggest we move forward?

urish commented 7 years ago

If you are open to it, I can try to implement poll(void* , uint16_t) for the nRF8001 and update API.md accordingly. Though, I don't have an actual nRF8001 chip here, so I will only be able to verify that the code compiles and seems to do the right thing. Thoughts?

sandeepmistry commented 7 years ago

I still prefer the opposite, as there are a lot of internal calls in the library itself to poll(), for example:

Adding a callback for the event data will also help with the discussion in https://github.com/sandeepmistry/arduino-BLEPeripheral/pull/115#issuecomment-272828387

urish commented 7 years ago

Okay, I will try to do it the other way. Currently, I see that all events handlers get just the central as a parameters. We could store a copy of the last event received in the central object, and then the user's event handler implementation will pull it from there, though, I am not sure that it is the cleanest way to do it. Thoughts?

sandeepmistry commented 7 years ago

@urish good question.

Maybe an API like setRawEventHandler(callback)

where callback has a signature of

void callback(int eventId, const byte eventData[]) {
  // ...
}

A type of const void* could also be an option, this should work nicely for both nR8001 and nRF51822.

Thoughts?

urish commented 7 years ago

Hi Sandeep, my focus has shifted to Espruino for this project. However, if you are still interested, I will try to create a PR for this feature with the callback approach.

sandeepmistry commented 7 years ago

@urish sure, I think the feature is still valuable.

Do you have a link to your Espruino work?

urish commented 7 years ago

yes, here we go:

https://github.com/urish/Espruino/tree/ng-beacon-nrf52

floe commented 7 years ago

FWIW, I've updated #155 with a couple of new handlers for BLEDeviceEvent* - maybe have a look there & comment, I think it's similar to what was proposed here earlier.

urish commented 4 years ago

Closing due to inactivity.