adafruit / Adafruit_CircuitPython_BLE

Bluetooth Low Energy (BLE) library for CircuitPython
MIT License
127 stars 58 forks source link

Add an AdafruitRadio Advertisement class. #33

Closed ntoll closed 4 years ago

ntoll commented 4 years ago

DO NOT MERGE

This doesn't work, in so far as I'm able to broadcast messages with the new class but incoming messages don't appear to match via the prefix. When I test-scan without any specified prefixes I see the receiving device registers these messages, but they're filtered out if the prefix is specified.

Any hints would be most welcome. I'm sure I'm missing something really obvious but just not seeing it. ;-)

My tactic has been to take the working example code and modify it only slightly. As you can see, even this minimal change hasn't worked. :-/

jerryneedell commented 4 years ago

Tested on on CPB with example from toll/adafruit_circuitpython_radio -- works for me!

tannewt commented 4 years ago

I'd suggest loosening the prefix to filter only adafruit manufacturer data and then add a matches function that checks the manufacturer data map for the key. That will allow you to have a variable string length.

Are you planning on adding the other things such as group to the message?

ntoll commented 4 years ago

Hmm.... @tannewt while I totally understand the words you use, I'm not quiet sure I understand them used together. ;-) (Imagine I'm Winnie the Pooh - a developer with a very little brain).

Can you point me at some docs -- docs = I'll be able to figure it out / figure out what I should be Googling.

Ack and agree about variable string length being desirable.

tannewt commented 4 years ago

By loosening the prefix I mean shorten it so it matches all adafruit manufacturing data. Then override this to look for the specific key for radio data: https://github.com/adafruit/Adafruit_CircuitPython_BLE/blob/master/adafruit_ble/advertising/__init__.py#L256

I don't think there are any great docs yet since it all is very new.

ntoll commented 4 years ago

OK, I've added what I think is the modification needed for extended messages. Please check..!

FWIW, I've tested on hardware and it works for me..! YMMV. ;-)

Still need to figure out the prefix related stuff. AFAICT, it just works right now and I'm not sure what benefit the change you're suggesting @tannewt would have.

ntoll commented 4 years ago

Screenie of Mary Poppins testing the extended advertisement.... ;-)

supercalifragilisticexpialidotious

tannewt commented 4 years ago

Thanks @ntoll, love the gif!

Shortening the prefix would allow you to vary the overall length of the advertisement. Right now it includes the total length which fixes it for all messages.

tannewt commented 4 years ago

@ntoll Has this dropped off of your radar?

ntoll commented 4 years ago

@tannewt :wave: nope... I've just been arranging my next gig (starting in January) and had to travel up to Edinburgh Wed->Thurs to give a talk at the British Computing Society. Busy week etc... ;-)

I'll try to take a look this weekend to dot the "i"s and cross the "t"s IYSWIM.

ntoll commented 4 years ago

@tannewt :wave: Hi... many apologies for the silence on my part. Last week (and Monday) was brutally busy and this week is looking like the same.

Looking at the code, I'm unsure what the outcome of using the extended advertisement with a non-extended device (since the devices I have all use extended advertisement - so I'm not able to check). The simplest solution would be to drop the length from the prefix, as you suggest, and just assume "it works" for extended advertising capable devices, with a warning of unknown behaviour and/or truncated messages for those devices only capable of non-extended advertisements (pending testing on our part).

Does this make sense?

tannewt commented 4 years ago

@ntoll I wouldn't let it hang you up now. Let's not worry about interop at this point and just try it out as you suggest.

ntoll commented 4 years ago

@tannewt done..! :-)

tannewt commented 4 years ago

Looks like it needs a doc string still: https://travis-ci.com/adafruit/Adafruit_CircuitPython_BLE/builds/141566787#L316

ntoll commented 4 years ago

@tannewt doh... sorry about that. I should have checked. More speed, less haste... etc. It's green now. ;-)