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

[AntonKlimenkov]

python-bluezero/bluezero/GATT.py

Line 204 in e120285

def write_value(self, value, flags=''):

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]

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]

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]

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]

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]

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]

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]

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]

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]

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:

• Does it work as expected/needed?
• Does it break code that used to work?
• Is it consistent with other parts of the Bluezero library?
• How can this be tested with the automated tests?

[AntonKlimenkov]

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]

Added fix with 86afd98

[AntonKlimenkov]

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

• Passing flags=None to write_value works as before for me. Slow.
• Passing flags={'type': 'command'} actually sends 'commands' and speeds things up considerably.

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.