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

Format Characteristics Value following Bluetooth SIG specification #237

Closed
Carglglz opened this issue Jul 1, 2020 · 19 comments
Closed

Format Characteristics Value following Bluetooth SIG specification #237

Carglglz opened this issue Jul 1, 2020 · 19 comments
Assignees
Labels
enhancement New feature or request Opinions Appreciated Please add an opinion on your desired resolution on this issue!

Comments

@Carglglz
Copy link
Contributor

Carglglz commented Jul 1, 2020

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. 👍

@Carglglz Carglglz changed the title Format characteristi Format Characteristics Value following Bluetooth SIG specification Jul 1, 2020
@hbldh hbldh self-assigned this Jul 2, 2020
@hbldh hbldh added enhancement New feature or request Opinions Appreciated Please add an opinion on your desired resolution on this issue! labels Jul 2, 2020
@hbldh
Copy link
Owner

hbldh commented Jul 2, 2020

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

Carglglz commented Jul 2, 2020

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:

  • No dependencies, just Python therefore platform agnostic
  • Compatibility out of the box with +250 SIG Bluetooth characteristics (this needs to be tested properly)

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

Carglglz commented Jul 7, 2020

@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
Copy link
Owner

hbldh commented Jul 9, 2020

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

Carglglz commented Jul 9, 2020

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

@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
Copy link
Owner

hbldh commented Jul 16, 2020

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

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

hbldh commented Jul 20, 2020

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

@polskafan
Copy link

polskafan commented Jul 31, 2020

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

Carglglz commented Jul 31, 2020

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

Carglglz commented Aug 13, 2020

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

hbldh commented Aug 17, 2020

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

Carglglz commented Aug 18, 2020

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

hbldh commented Aug 19, 2020

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

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

@hbldh
Copy link
Owner

hbldh commented Sep 2, 2020

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

@hbldh hbldh closed this as completed Sep 2, 2020
@Carglglz
Copy link
Contributor Author

Carglglz commented Sep 8, 2020

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

hbldh commented Sep 10, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Opinions Appreciated Please add an opinion on your desired resolution on this issue!
Projects
None yet
Development

No branches or pull requests

3 participants