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

Does advertising.standard.ManufacturerDataField.field_names currently serve a purpose? #114

Open
alexwhittemore opened this issue Dec 28, 2020 · 7 comments

Comments

@alexwhittemore
Copy link
Contributor

alexwhittemore commented Dec 28, 2020

Correct me where I'm wrong:

The two possible ways to pack multiple bits of (manufacturer) data into an advertisement are

  1. create multiple ManufacturerDataField() items in your ad class.
    Pro: you get to access that data with AlexsAd.firstdataitem syntax, and you get to include-or-not items as you see fit per-broadcast.
    Con: Costs a few bytes to include the data type ID before the data item in the ad.
  2. create a single ManufacturerDataField() item, with a format that includes however many fields, all of which must be populated for any given ad.
    Pro: Saves having to spend a couple bytes per field
    Con: Have to access with AlexsAd.data[index] where you simply have to know the relevance of data at a given index, and data must have some rational "null" value that indicates "I never set that" like 0 for battery voltage, or -1 for "firefighters on scene"

Assuming I understand that architecture right, what purpose does the self.field_names object serve in advertising.standard.ManufacturerDataField on

?

It seems to add some structure and convenience about what the different indices in a ManufacturerDataField tuple mean, but also there doesn't seem to be any actual way to use that. The field_names must be defined at the class level for the class to function right, but then the values passed serve no purpose from that point on other than to consume memory (do they even?).

@tannewt
Copy link
Member

tannewt commented Dec 29, 2020

You are correct about 1 and 2.

I think field_names was meant to be used in a named tuple but never implemented. You could either implement it or remove it if it's causing trouble.

@alexwhittemore
Copy link
Contributor Author

I never even knew about named tuples. I think in a perfect world that would make case 2 above a bit more convenient, without breaking anything as-is. I'll see what I can whip up re: implementing it. It looks like collections.namedtuple exists in CP 6 on my nRF52840 Feather Sense - is that universally the case across all supported platforms?

@dhalbert
Copy link
Collaborator

Any build that supports BLE should have namedtuples.

@alexwhittemore
Copy link
Contributor Author

It looks like there aren't actually any unit tests for this codebase - if that's not true or of I biff something but it happens to work for me, is there any backstop I'm missing?

@tannewt
Copy link
Member

tannewt commented Dec 30, 2020

Nope, we're bad about testing because it's hard to do on real hardware.

@alexwhittemore
Copy link
Contributor Author

Darn - that's something I'd like to figure out how to do better myself. Pigweed looks cool but I'm still desperately confused by it. Also Renode.

Ok, I'll try to put together a pull request that takes it to fruition in the next couple days!

@alexwhittemore
Copy link
Contributor Author

Ok, so: I figured that when I define a subclass of Advertisement, such as AdafruitColor, the arbitrary-named ManufacturerDataField objects (such as color in that example) must get populated on matched advertisement receipt by the getter that returns a tuple on line

For instance, the AdafruitColor class surely must get its color instance variable by that getter, right?

... Wait. Darnit. This rubber duck exercise totally worked. I just realized I never hit 292 simply because that getter returns earlier and I just didn't fully consider it. Posting this anyway mostly for my own notes, but I'm on my way.

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

No branches or pull requests

3 participants