adafruit / Adafruit_CircuitPython_BLE

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

prefix building #82

Closed dhalbert closed 4 years ago

dhalbert commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

dhalbert commented 4 years ago

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 commented 4 years ago

I've been building up a few more observations in https://github.com/adafruit/Adafruit_CircuitPython_BLE/issues/79#issuecomment-635607039. 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 commented 4 years ago

There is a bug (https://github.com/adafruit/circuitpython/issues/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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

kevinjwalters commented 4 years ago

I had not realised the prefix could be multiple ones concatenated. That's not obvious. There's an example of this in https://github.com/adafruit/Adafruit_CircuitPython_BLE/blob/ffd1f1d9f08dc9df6ea1a4ac08338e999b052f75/adafruit_ble/advertising/standard.py#L168-L186