search

ukBaz / python-bluezero

A simple Python interface to Bluez
MIT License
394 stars 112 forks source link

Passing a 'flags' dictionary to 'write_value' in GATT.py ends in exception #413

Open AntonKlimenkov opened 1 month ago

AntonKlimenkov commented 1 month ago

https://github.com/ukBaz/python-bluezero/blob/e1202855bf25ee145d5bd6941afddcda88998808/bluezero/GATT.py#L204 Passing any flags specified in https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/doc/org.bluez.GattCharacteristic.rst to 'write_value' in GATT.py as a python dictionary leads to an exception:

2024-07-29 14:29:40,982 [ERROR] [connection.py:628] Unable to set arguments (b'\x02\x00\x17\x15\x02\x08\x00\x00\x00\x02\x01\x05\x12$\x07)\x03\x00\x00\x00\x17\xec\x00\x00\x17\xec\x00\xbe)\x03', dbus.Array(['type'], signature=None)) according to signature 'aya{sv}': <class 'TypeError'>: list indices must be integers or slices, not str
Traceback (most recent call last):
 ......
  File "***", line 195, in write
    self._write_char.write_value(data[:chunk_size], flags={'type': 'command'})
  File "/path/venvs/everything/lib/python3.10/site-packages/bluezero/GATT.py", line 214, in write_value
    self.characteristic_methods.WriteValue(value, dbus.Array(flags))
  File "/path/venvs/everything/lib/python3.10/site-packages/dbus/proxies.py", line 141, in __call__
    return self._connection.call_blocking(self._named_service,
  File "/path/venvs/everything/lib/python3.10/site-packages/dbus/connection.py", line 625, in call_blocking
    message.append(signature=signature, *args)
TypeError: list indices must be integers or slices, not str
ukBaz commented 1 month ago

Do you have a minimal reproducible example piece of code I could use for testing?

Looking at this now, it doesn't seem to be well documented in the library, but shouldn't it be something like:

write_value(b'\x02\x00...', {'type': 'command'})
AntonKlimenkov commented 1 month ago

I don't have a minimal example yet. Will try to come up with it.. But I think the issue is that in your example {'type': 'command'} will be converted to a dbus.Array before it's passed to WriteValue in localGatt.py.

`    def write_value(self, value, flags=''):
        """
        Write a new value to the characteristic.

        :param value: A list of byte values
        :param flags: Optional dictionary.
            Typically empty. Values defined at:
            https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/doc/gatt-api.txt
        """
        try:
            self.characteristic_methods.WriteValue(value, dbus.Array(flags))
        except AttributeError:
            logger.error('Service: %s with Characteristic: %s not defined '
                         'on device: %s. Cannot write_value',  self.srv_uuid,
                         self.chrc_uuid, self.device_addr)`

And it seems that dbus.Array() loses the values of the dictionary.

In [14]: dbus.Array({'type': 'command'})
Out[14]: dbus.Array(['type'], signature=None)
ukBaz commented 1 month ago

I agree with you. It looks like it should be:

dbus.Dictionary({'type': 'command'}, signature=dbus.Signature('sv'))

I've looked through the test cases in Bluezero and there doesn't seem to be anything that tests this.

Are you able to take a look on your setup as to what works?

AntonKlimenkov commented 1 month ago

Minimal example to catch exception:

from bluezero import adapter, central, constants, peripheral

dongle = next(adapter.Adapter.available())
dongle.nearby_discovery()

device_addr = 'SOME_ADDR'
service_uuid = 'SOME_SERV_UUI'
write_uuid = 'SOME_WRITE_UUI'

monitor = central.Central(device_addr=device_addr)
monitor.connect()
write_char = monitor.add_characteristic(service_uuid, write_uuid)
monitor.load_gatt()

write_char.write_value(b'asdad', flags={'type': 'command'})

Changing dbus.Array(flags) to dbus.Dictionary(flags) or passing flags directly works: as in data does get send in 'command' mode = not waiting for acks.

ukBaz commented 1 month ago

I've spent a little time on this issue and considering how this should look and fit with the API complexity goals of Bluezero

I'm not sure I'm happy for this to be a final solution, but to give visibility of what I've tried.

I created a little script to exercise some different values using a Micro:Bit I had laying around.

from random import randint

import dbus

from bluezero import central

ubit_addr = 'E1:4B:6C:22:56:F0'
temperature_srvc = 'e95d6100-251d-470a-a062-fa1922dfa9a8'
temperature_period = 'e95d1b25-251d-470a-a062-fa1922dfa9a8'

def get_rand_period():
    rand_value = randint(1000, 1500)
    return int(rand_value).to_bytes(2, byteorder='little', signed=False)

ubit = central.Central(device_addr=ubit_addr)
rw_char = ubit.add_characteristic(temperature_srvc, temperature_period)
ubit.connect()

test_write_options = [
    None,
    {},
    {'type': 'request', 'offset': 0},
    {'type': dbus.String('request'), 'offset': dbus.UInt16(0)},
]

print('Test write flags:')

for flags in test_write_options:
    period_rand = get_rand_period()
    print(f"\twriting {int.from_bytes(period_rand, byteorder='little', signed=False)} with {flags}")
    rw_char.write_value(period_rand, flags=flags)

test_read_options = [
    None,
    {},
    {'offset': 0},
    {'offset': dbus.UInt16(0)}
]
print("Test read flags")
for flags in test_read_options:
    result = rw_char.read_raw_value(flags=flags)
    print(f"\tRead value: {int.from_bytes(result, byteorder='little', signed=False)} with {flags}")

ubit.disconnect()

And then changed the functions in GATT.py to be consistent with this:

    def read_raw_value(self, flags=None):
        """
        Return this characteristic's value (if allowed).

        :param flags: "offset": Start offset
                      "device": Device path (Server only)
        :return:

        Possible Errors:    org.bluez.Error.Failed
                            org.bluez.Error.InProgress
                            org.bluez.Error.NotPermitted
                            org.bluez.Error.InvalidValueLength
                            org.bluez.Error.NotAuthorized
                            org.bluez.Error.NotSupported
        """
        _flags = {}
        if flags is None:
            flags = {}
        else:
            for k, v in flags.items():
                if k in ['offset', 'mtu']:
                    _flags[k] = dbus.UInt16(v)
                if k == 'device':
                    _flags[k] = dbus.ObjectPath(v)
        try:
            return self.characteristic_methods.ReadValue(dbus.Dictionary(_flags))
        except AttributeError:
            logger.error('Service: %s with Characteristic: %s not defined on '
                         'device: %s', self.srv_uuid, self.chrc_uuid,
                         self.device_addr)
            return []
    def write_value(self, value, flags=None):
        """
        Write a new value to the characteristic.

        :param value: A list of byte values
        :param flags: Optional dictionary.
            Typically empty. Values defined at:
            https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/doc/gatt-api.txt
        """
        _flags = {}
        if flags is None:
            _flags = {}
        else:
            for k, v in flags.items():
                if k in ['offset', 'mtu']:
                    _flags[k] = dbus.UInt16(v)
                if k == ['type', 'link']:
                    _flags[k] = dbus.String(v)
                if k == 'device':
                    _flags[k] = dbus.ObjectPath(v)
        try:
            self.characteristic_methods.WriteValue(value, dbus.Dictionary(_flags))
        except AttributeError:
            logger.error('Service: %s with Characteristic: %s not defined '
                         'on device: %s. Cannot write_value',  self.srv_uuid,
                         self.chrc_uuid, self.device_addr)

I have not looked at what would be required for localGATT.py

Example oputput from test run:

Test write flags:
    writing 1444 with None
    writing 1190 with {}
    writing 1225 with {'type': 'request', 'offset': 0}
    writing 1381 with {'type': dbus.String('request'), 'offset': dbus.UInt16(0)}
Test read flags
    Read value: 1381 with None
    Read value: 1381 with {}
    Read value: 1381 with {'offset': 0}
    Read value: 1381 with {'offset': dbus.UInt16(0)}
AntonKlimenkov commented 1 month ago

Thank you for the investigation! So you would feel reluctant introducing this change?

Another approach that would satisfy my needs would be to pass the same flags as parameters to add_characteristic of Central class in the way it's passed to Peripheral class.

ukBaz commented 1 month ago

So you would feel reluctant introducing this change?

I'm reluctant to make it a permanent change to the API at this time without getting more feedback on it.

Another approach that would satisfy my needs would be to pass the same flags as parameters to add_characteristic of Central class in the way it's passed to Peripheral class.

Values for mtu and offset could be set differently per read so I think moving to the add_characteristic command doesn't feel right for the library generally. Although I'm open to the idea if you have convincing evidence.

I guess there are two different needs here.

  1. The a fix for the library where this has never worked correctly. This is the first time it has been raised as an issue so I'm assuming this is an "advanced" usage corner case. While I am keen to fix it in the library, I would like to not break other things, and having some unit tests would be a good way of upping the confidence levels. The goal of this library is to help people get started with BLE and move away from the library when they are more knowledgable and want more control.

  2. The other need is that you want a quick fix so you can get on with your project.

AntonKlimenkov commented 1 month ago

Could I possibly provide more feedback/testing?

And please disregard my suggestion about add_characteristic. I've realised Central and Peripherla use different Characteristic class underneath, so it'd not be just passing parameters to the next function.

ukBaz commented 1 month ago

Could I possibly provide more feedback/testing?

Absolutely!

I'm pushed the changes to GATT.py to the read_write_options branch.

The feedback I'm interested in:

AntonKlimenkov commented 1 month ago

I think the following 230 if k == ['type', 'link']: should be 230 if k in ['type', 'link']: Tried to create a PR into the branch, but understandably I don't have the rights.

ukBaz commented 1 month ago

Added fix with 86afd98

AntonKlimenkov commented 1 month ago

Thanks for the fixes! What I can say from trying them:

Since in my experience passing anything to 'flags' on main branch leads to an exception, I'd assume every existing user of the library is not passing anything to 'flags'. So this change would not break anything for them.