Skip to content

Add an AdafruitRadio Advertisement class. #33

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Dec 19, 2019
Merged

Add an AdafruitRadio Advertisement class. #33

merged 6 commits into from
Dec 19, 2019

Conversation

ntoll
Copy link
Contributor

@ntoll ntoll commented Nov 18, 2019

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
Copy link
Contributor

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

@tannewt
Copy link
Member

tannewt commented Nov 19, 2019

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
Copy link
Contributor Author

ntoll commented Nov 19, 2019

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
Copy link
Member

tannewt commented Nov 19, 2019

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
Copy link
Contributor Author

ntoll commented Nov 25, 2019

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
Copy link
Contributor Author

ntoll commented Nov 25, 2019

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

supercalifragilisticexpialidotious

@ntoll ntoll mentioned this pull request Nov 25, 2019
Merged
@tannewt
Copy link
Member

tannewt commented Nov 26, 2019

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
Copy link
Member

tannewt commented Dec 6, 2019

@ntoll Has this dropped off of your radar?

@ntoll
Copy link
Contributor Author

ntoll commented Dec 6, 2019

@tannewt 👋 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
Copy link
Contributor Author

ntoll commented Dec 17, 2019

@tannewt 👋 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
Copy link
Member

tannewt commented Dec 17, 2019

@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
Copy link
Contributor Author

ntoll commented Dec 18, 2019

@tannewt done..! :-)

tannewt
tannewt approved these changes Dec 18, 2019
View reviewed changes
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This is good for now. In the future we can make the field itself variable length so make the advertisements shorter.

@tannewt
Copy link
Member

tannewt commented Dec 18, 2019

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

@ntoll
Copy link
Contributor Author

ntoll commented Dec 19, 2019

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

tannewt
tannewt approved these changes Dec 19, 2019
View reviewed changes
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem. :-)

@tannewt tannewt merged commit 0b5f0bd into adafruit:master Dec 19, 2019
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jan 14, 2020
@adafruit-adabot
Automated update by Adabot (adafruit/adabot@42bfc73)
12905ba 
Updating https://github.com/adafruit/Adafruit_CircuitPython_BLE to 4.0.0 from 3.1.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_BLE#53 from dherrada/master
  > Merge pull request adafruit/Adafruit_CircuitPython_BLE#49 from adafruit/dherrada-patch-1
  > Merge pull request adafruit/Adafruit_CircuitPython_BLE#51 from adafruit/dherrada-patch-2
  > Merge pull request adafruit/Adafruit_CircuitPython_BLE#33 from ntoll/master
  > Merge pull request adafruit/Adafruit_CircuitPython_BLE#48 from tannewt/remove_magic_light
  > Merge pull request adafruit/Adafruit_CircuitPython_BLE#47 from tannewt/remove_ancs

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Added the following libraries: Adafruit_CircuitPython_BLE
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tannewt tannewt tannewt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ntoll @jerryneedell @tannewt