search

hbldh / bleak

A cross platform Bluetooth Low Energy Client for Python using asyncio
MIT License
1.78k stars 295 forks source link

Format Characteristics Value following Bluetooth SIG specification #237

Closed Carglglz closed 4 years ago

Carglglz commented 4 years ago

Hi, this is an enhancement proposal.

The feature I'm proposing is to automatically format a characteristic value following Bluetooth SIG specification. This would work the same way as nRF Connect android app, e.g. in service_explorer.py example instead of:

if "read" in char.properties:
                    try:
                        value = bytes(await client.read_gatt_char(char.uuid))
                    except Exception as e:
                        value = str(e).encode()

where value is a bytearray, implementing a function like get_char_value:

if "read" in char.properties:
                    try:
                        value = bytes(await client.read_gatt_char(char.uuid))
                        formatted_value = get_char_value(value)
                    except Exception as e:
                        value = str(e).encode()

This function will format the bytearray according to the characteristic. For example reading the Temperature characteristic, the value in this case could be b'\xc4\t', then formatted_value wil be: {'Temperature':{'Value':25.00, 'Symbol': 'ºC'}}

I've already got this working, although I haven't test all the characteristic (there are around 283...) I think this looks promising, so If there is any interest in this feature I could make a PR this or the next week. 👍

hbldh commented 4 years ago

I like the idea, but I am not sure about adding it as default behaviour for the read method. It you would implement something like

def get_char_value(value: Union[bytes, bytestring], characteristic: BleakGATTCharacteristic) -> dict:
    ...

and add that to bleak.utils I would consider merging it. But I think this is a feature that the user should add themselves as a formatting on the return value of read_gatt_char.

Carglglz commented 4 years ago

Yes, this is not intended to replace the read method, it is just a utility to unpack/decode those bytes into the right variables. So I do think that bleak.utils would be the right place for this too.

But I think this is a feature that the user should add themselves as a formatting on the return value of read_gatt_char.

Yes this is in fact trivial with characteristics that encode a single value, like Temperature in the example above. However there are characteristics like Heart Rate Measurement that present bytes that are flags, and they need to be bit masked in other to get the proper unpacking format.

So this feature may save some work time when trying to read more complex characteristics. The pros are:

The only "downside" I see is that it needs the xml file of each characteristic, then they should be included in the installation (about 3.5 MB).

I will fork and implement this into the develop branch, then I will make a proper PR explaining everything in detail so you can decide and cherry-pick what is worthy. 👍

Carglglz commented 4 years ago

@hbldh I've just merged this feature into the develop branch of my fork, see Carglglz#1 so you can test this with the phone (nRF Connect) or another device (microbit, esp32, sensor tag, etc.)

hbldh commented 4 years ago

This is interesting indeed. Is it possible to trim down the xmls to what you actually need instead of including the entire files? This will grow the size of the bleak installation by many order of magnitudes, and I am reluctant to do so.

I think it would be better to extract this into a separate package (bleak_sigspec?) and release it separately on PyPI? Reference to that package could then be added to the documentation for people to install separately.

Carglglz commented 4 years ago

Is it possible to trim down the xmls to what you actually need instead of including the entire files?

I'm afraid it is not possible, I would say that almost 90% of the xml file is relevant , but this:

I think it would be better to extract this into a separate package (bleak_sigspec?) and release it separately on PyPI?

I think it is a nice solution, I could make a test repo and maybe add some documentation about how it is done and the limitations or future enhancements that it could have.

I will let you know when it is ready to test. 👍

Carglglz commented 4 years ago

@hbldh I've just made the test with bleak_sigspec package (see test repo in test_bleak_sigspec) and everything works great. I've also introduced some fixes/additions in my fork develop branch for #102, see Carglglz#4

hbldh commented 4 years ago

Nice! Regarding the fixes and addons, please make a pull request of it towards the develop branch and I will merge it if it does not pose any potential issues with existing code.

Feel free to include documentation changes referring people to your package in the documentation! It is enough relevant to also merit reference in the README, imo.

Carglglz commented 4 years ago

I will try to get this done by the end of this week, since I would like to double check everything and update the documentation accordingly. I will make a PR when it's done and I will let you know. 👍

hbldh commented 4 years ago

Sounds fine! I will not be available to do any work with the PR until middle of next week anyway I am afraid...

polskafan commented 4 years ago

Great Idea!

Tried to use bleak_sigspec and @Carglglz's development branch to decode data from my BLE heart rate monitor. I found two issues with it: a) Line 437 in utils.py there is a stray self in the signature of _complete_bytes, that needs to be removed to get the decoder working b) My heartrate monitor seems to send a varying number of RR-Interval fields. Right now it only seems to work if there are no RR-Interval fields or one. If there are more, then the decoder fails. I don't really know how to deal with this. There is a note in [1] stating, that the RR-Interval field can be repeated, but I can't see a machine readable element in the xml specification that indicates that.

[1] = https://github.com/Carglglz/bleak_sigspec/blob/master/bleak_sigspec/characteristics_xml/heart_rate_measurement.xml

Carglglz commented 4 years ago

Hi @polskafan I'm still working on this. I think I fixed a) yesterday and improved characteristics references see Carglglz#9. It's nice to see that this kind of works with other devices (I just have my phone and esp32 to check).

About b) The good news is that I think is doable. The bad news is I that I will need to introduce some changes to check for this specific characteristic and use its own method. I can't do this right now but I will point you in the right direction:

Since this characteristic has a Flags field, it is unpacked to see which other fields are present: e.g. (hrm_value, is the result with just one RR interval field, to get the RR flag)

# Heart Rate Measurement
hrm = struct.pack('BBHHH', eval('0b00011000'), 60, 50, 1, 1)

# This should be implemented in _get_multiple_fields function 

_FIELDS_TO_READ = []
more_than_one = hrm_value[1]['RR-Interval bit'] # RR-Interval bit flag
if more_than_one == 'One or more RR-Interval values are present.':
    ctype_global = 'BBHH' # this is the global type that detects
    print("Global Unpack Format: {}".format(ctype_global))
    print("Global unpack format length: {}, Bytes: {}".format(struct.calcsize(ctype_global), len(hrm)))
    while len(hrm) > struct.calcsize(ctype_global):  # this is the correction
        ctype_global += 'H'
        _FIELDS_TO_READ.append('RR-Interval')
    print("Global unpack format length: {}, Bytes: {}".format(struct.calcsize(ctype_global), len(hrm)))
    print("Global Unpack Format: {}".format(ctype_global))

raw_values = struct.unpack(ctype_global, hrm)
print(raw_values)
Global Unpack Format: BBHH
Global unpack format length: 6, Bytes: 8
Global unpack format length: 8, Bytes: 8
Global Unpack Format: BBHHH
(24, 60, 50, 1, 1)

So that's it, just check len(hrm) > struct.calcsize(ctype_global) and add 'H' while true. I will try to implement this next week I guess. 👍

Carglglz commented 4 years ago

Well It took me longer than expected, but I think this is almost done, now I have to write the documentation #266. To sum up what will include this PR:

This should add compatibility with any characteristic with a proper xml file and a compatible encoding format. There may be some exceptions like heart_rate_measurement mentioned above, but in any case it should be fixable. 👍

hbldh commented 4 years ago

This looks very interesting, but I just want to request some modifications on how this is brought into Bleak:

  • Add missing uuids to uuid16_dict and uuid128_dict

  • Add descriptions to characteristics and descriptors (Linux, MacOS)

These two I would like to have added as a separate PR, related to uuids. The CHAR_XML class should however be moved to the bleak_sigspec module imo. (see notes at the bottom)

  • Fix for #102, with a timeout to event.wait() that prevents from blocking indefinitely. (MacOS)

  • Fix for notify by handle, notify callback now accepts three inputs (handle, value, cUUID) (MacOS)

  • Add method to get/update the RSSI value of the connected Peripheral (MacOS)

The two fixes and the added RSSI could be one PR, addressing macOS issues.

  • Format characteristics Value following Bluetooth SIG specification: (all OS)

  • New module bleak.formatter with SuperStruct class which adds

These two I had planned to present in the bleak_sigspec package, not in bleak. Nothing in Bleak shoudl import from bleak_sigspec. It is an external addon, not an extras inside the bleak package. I would prefer to keep the import xml outside of bleak, making it import as little as possible of non-strictly necessary packages. This might seem like nitpicking, but I want to keep bleak as small as possible in memory, to still keep it usable for very small iot devices...

Carglglz commented 4 years ago

I agree, this in fact would make things easier, so here are the PRs:

I've already moved the xml parser and characteristics formatter to bleak_sigspec So next thing will be updating the docs.

hbldh commented 4 years ago

Thank you! This is a better way of reviewing the changes. I merged #272 directly, but #273 requires some modifications before I can review it properly and test it.

Carglglz commented 4 years ago

Fixed #273 and PR is ready. Also I put together a 'preview' of bleak_sigspec documentation in https://bleak_sigspec.readthedocs.io.

hbldh commented 4 years ago

It looks good! I regard this as done and will make sure reference to bleak_sigspec will be present when writing documentation in #266.

Carglglz commented 4 years ago

I just want to mention that the documentation is now done and the package is already on PYPI. https://pypi.org/project/bleak-sigspec/

Also if help is needed with #266 , I'm in.

And finally I'm very pleased with bleak, I've been testing it (on MacOS) for over a month with the bleak_sigspec development and the BLE utility I'm working on and I have to say it is production ready, so I won't be surprised if bleak becomes the go-to library for BLE development. 🥇

hbldh commented 4 years ago

Very nice. I will get the 0.8.0 release out first and focus on 266 after that. There is a feature/wtd branch where I have laid out the basic structure of the new documentation.