Skip to content
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

[RFC] add a bunch of hackish (but working) observer role functions #155

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

floe
Copy link
Contributor

@floe floe commented May 19, 2017

Amazing library, saved me a lot of work when trying to get the BBC micro:bit up and running for a student. Thanks!

That being said, I'm currently looking into broadcast-based mesh applications, and I need observer support for these (not a full-scale central role, but at least receiving advertisements). I hacked together a couple of control functions and wanted your opinion on

  • if this an addition to the library you'd want to integrate (maybe difficult, I guess the nRF8001 doesn't support scanning at all)
  • if you have any suggestions how to properly handle the scan results - I'm currently thinking to just have a uint8_t* getScanResult() method that's supposed to be called after poll() and will return the most recent result, or NULL.

Best, Florian

@sandeepmistry
Copy link
Owner

Hey @floe,

Thanks for taking the time to submit this PR!

For compatibility reasons, I'd prefer to keep this library peripheral only.

However, moving forward it would be great to great a dual central and peripheral mode library for the nRF51/52 based on the CurieBLE API: https://github.com/01org/corelibs-arduino101/tree/master/libraries/CurieBLE

Thoughts? I don't have time to start this at the moment though ...

@floe
Copy link
Contributor Author

floe commented May 29, 2017

Ah, didn't know about CurieBLE yet, I'll have a look. For the moment, I'll probably go with my hackish-but-works solution... no time for a proper solution, either ;-)

BTW, PR #120 is related in terms of API, isn't it?

@floe
Copy link
Contributor Author

floe commented May 29, 2017

P.S. The alternative would be to create a dedicated fork of your library that only supports the nRF5x series. Would probably be less work than rebuilding everything to fit the CurieBLE API?

@sandeepmistry
Copy link
Owner

P.S. The alternative would be to create a dedicated fork of your library that only supports the nRF5x series. Would probably be less work than rebuilding everything to fit the CurieBLE API?

Yes, that's a good way to start. We can always split it out into a new repo later.

@floe
Copy link
Contributor Author

floe commented Oct 23, 2017

Came back to this after a couple of months, PR is now in a state where it's working fine for my specific use case. My suggestion would be to review this under the assumption that we'll get rid of the nRF8001 support sooner or later.

@sandeepmistry
Copy link
Owner

My suggestion would be to review this under the assumption that we'll get rid of the nRF8001 support sooner or later.

No plans to remove it at this time :)

@floe
Copy link
Contributor Author

floe commented Nov 13, 2017

Well, removing is probably too harsh, but how about splitting your library into a legacy branch that still supports the nRF8001, and a new branch that supports nRF51/52?

@sandeepmistry
Copy link
Owner

New branch sounds ok short term for development. However, to make it compatible with the Arduino IDE's library manager a new repo would need to be created.

@cheeta1
Copy link

cheeta1 commented Dec 7, 2017

hi @floe can you provide the example of observer role in you master branch?

@floe
Copy link
Contributor Author

floe commented Dec 7, 2017

@cheeta1 sure, no problem, see examples/observer/ in my master branch. It's a bit kludgy, but it demonstrates simultaneous sending and receiving of advertisements (i.e. broadcaster and observer in one).

@floe
Copy link
Contributor Author

floe commented Dec 7, 2017

@sandeepmistry if I rename my fork to, let's say, BLEPeripheralObserver, would that work?

@sandeepmistry
Copy link
Owner

if I rename my fork to, let's say, BLEPeripheralObserver, would that work?

@floe works for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants