Skip to content

prefix building #82

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

Closed
dhalbert opened this issue May 14, 2020 · 11 comments · Fixed by #89
Closed

prefix building #82

dhalbert opened this issue May 14, 2020 · 11 comments · Fixed by #89
Assignees
Labels
enhancement New feature or request

Comments

@dhalbert
Copy link
Collaborator

dhalbert commented May 14, 2020

Currently the advertising prefix mechanism requires counting bytes and manually including a length. I was thinking about some helper functions for this. Straw ideas:

class SomeAdvertisement(Advertisement):
    prefix = Advertisement.prefixes(<prefix>, <prefix>, ...)

The class method would add length headers and merge into a single bytes
and maybe also include any/all matching designation in a Prefix object:

    prefix = Advertisement.Prefix.match_any(...)
...
    prefix = Advertisement.Prefix.match_all(...)

The prefix class-variable name might be renamed to prefixes or match_prefixes, since it's actually multiple prefixes.

@tannewt
Copy link
Member

tannewt commented May 14, 2020

The first makes sense to me. I'm not sure you can do the any or all currently with the native _bleio filtering. I believe it's always any.

@dhalbert
Copy link
Collaborator Author

I'm not sure you can do the any or all currently with the native _bleio filtering. I believe it's always any.

Right, I was thinking only of the Python-based filtering. Right now you have to override the matches() function to provide any, and I was trying to think of an easier way. But it would be confusing to have only one for native and two for non-native.

@kevinjwalters
Copy link

Can you show how AdafruitColor would look with this new approach?

@dhalbert dhalbert self-assigned this May 30, 2020
@dhalbert
Copy link
Collaborator Author

dhalbert commented May 30, 2020

I'm working on a PR that includes this, and after working through a couple of ideas, I'm now just using a tuple of bytes, e.g.

    match_prefixes = (b'...', b'...', ...)

For AdafruitColor, I have this, but haven't tested it yet:

    match_prefixes = (
        struct.pack(
            "<BHBH",
            _MANUFACTURING_DATA_ADT,
            _ADAFRUIT_COMPANY_ID,
            struct.calcsize("<HI"),
            _COLOR_DATA_ID,
        ),
    )

Basically, you don't have to concatenate the prefixes yourself, and you don't have to do length-counting yourself (which reminds me of FORTRAN Hollerith counting: 7HTHE END). The concatenated format with length fields is still used internally, but you don't need to see that.

@kevinjwalters
Copy link

I've been building up a few more observations in #79 (comment). I have just noticed that setting tx_power breaks the start_scan matching for an Advertisement sub-class I've made. Does your proposed matching solve that? I was also wondering if the "<" are better applied by the library code?

@dhalbert
Copy link
Collaborator Author

There is a bug (adafruit/circuitpython#2973) that manifests when the prefix matching is "all" rather than "any". I don't know if that would cover your case here. Could you point to your code? Could you show the raw advertisement and scan response that do not work with the current matching?

The library could add the <, but that means we'd need a wrapper function for struct.pack(), and I think that would obfuscate what was going on. If the need for < is well-documented, I think that would be OK. In most cases < is redundant, because the underlying platform is little-endian anyway.

ManufacturerDataField (as opposed to ManufacturerData) is our own encoding of the the mfr data segment of the advertisement. Perhaps calling it AdafruitManufacturerDataField or similar might make it clearer. Manufacturer data can be anything (hence the name). I don't know if @tannewt took this multi-field-with-length encoding from some third-party examples or invented it for this particular set of examples.

For the problem you're trying to solve, would using connected services rather than advertisements also work? Also, have you looked at https://github.com/adafruit/Adafruit_CircuitPython_BLE_Radio, which would push the parsing and building to your code, but might make things clearer because of that?

@kevinjwalters
Copy link

kevinjwalters commented May 31, 2020

I didn't know about https://github.com/adafruit/Adafruit_CircuitPython_BLE_Radio . I had a glance at the code and it's interesting to see that it's still based on Advertisements. I thought when it mentioned channel numbers that it was allowing a specific Bluetooth Low Energy channel to be used but the range is too big AFAIUI. In the code it just looks like a data field in the messages. It also has a MAX_LENGTH of 248 in it. A lot of docs mentioned 31 and I think this relates to which Bluetooth version is supported. I've also seen code which has support for both (https://github.com/adafruit/Adafruit_CircuitPython_BLE_BroadcastNet/blob/master/adafruit_ble_broadcastnet.py#L46-L56) so I'm wondering if both is required here?

@dhalbert
Copy link
Collaborator Author

Yes, the "channel" numbers" are just an ID, not a frequency channel. Extended advertising can work on devices that support it. This is meant to be between CircuitPython devices. If it doesn't work, let us know, of course.

@kevinjwalters
Copy link

I've not read about the extended Advertising support, do all CircuitPython devices support it? In terms of devices that would cover the Raspberry Pis too, wouldn't it? I always forget people use the CP libraries there. Is this the feature from Bluetooth 4.2 than enables it http://software-dl.ti.com/lprf/sdg-latest/html/ble-stack-3.x/data-length-extensions.html ?

@dhalbert
Copy link
Collaborator Author

dhalbert commented Jun 1, 2020

The nRF52840 (and therefore CPy) do support this. The two sides of a connection negotiate this.

@kevinjwalters
Copy link

I had not realised the prefix could be multiple ones concatenated. That's not obvious. There's an example of this in

class ProvideServicesAdvertisement(Advertisement):
"""Advertise what services that the device makes available upon connection."""
# This is four prefixes, one for each ADT that can carry service UUIDs.
prefix = b"\x01\x02\x01\x03\x01\x06\x01\x07"
services = ServiceList(standard_services=[0x02, 0x03], vendor_services=[0x06, 0x07])
"""List of services the device can provide."""
def __init__(self, *services):
super().__init__()
if services:
self.services.extend(services)
self.connectable = True
self.flags.general_discovery = True
self.flags.le_only = True
@classmethod
def matches(cls, entry):
return entry.matches(cls.prefix, all=False)

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

Successfully merging a pull request may close this issue.

3 participants