ukBaz / python-bluezero

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

Writing a peripheral characteristic sends back data #382

Open Digitelektro opened 2 years ago

Digitelektro commented 2 years ago

Dear ukBaz!

Scenario: I have a characteristic with Write and and Indicate properties. When a central writes this characteristic, the peripheral notifies back the data. I think it is because the code updates the characteristic value which eventually triggers the write_value. This is not good for at least two reasons:

I have a command characteristic where every command sent from central gets back an ACK response from the peripheral. But Central gets back the same command first that it has just sent... I hope you get what is the problem with it.

I commented out a line from localGATT.py:

def WriteValue(self, value, options):  # pylint: disable=invalid-name
        """
        DBus method for setting the characteristic value

        :return: value
        """
        if self.write_callback:
            self.write_callback(dbus_tools.dbus_to_python(value),
                                dbus_tools.dbus_to_python(options))
        # This is commented out: 
        # self.Set(constants.GATT_CHRC_IFACE, 'Value', value)

This solves the problem for me, but I don't know if it doesn't mess up something else.

ukBaz commented 2 years ago

I take you point @Digitelektro about wanting control of the update of the notification. The design idea behind what has been done is to have a property that could be be written and read to/from.

A more typical design pattern is to have one characteristic for write and then another for notifications e.g. https://github.com/ukBaz/python-bluezero/blob/main/examples/ble_uart.py

Which is based on the Nordic UART service which is detailed here: https://learn.adafruit.com/introducing-the-adafruit-bluefruit-le-uart-friend/uart-service and here https://lancaster-university.github.io/microbit-docs/resources/bluetooth/bluetooth_profile.html

I'm not sure how to balance your specific case and the more common case.

Digitelektro commented 2 years ago

Hi @ukBaz ! I was expecting this answer :) I have been working with BLE in the last 3 years, and none of the libraries worked on this way! But I can accept that this implementation is lets say a higher level API. Yes, the nordic uart service based implementation could work, but it still won't help to reduce the packet overhead! If you want to achive the highest data rate, this will reduce performance a lot. Which is the case in my scenario. I can live with it now, I just wanted you to consider it! Thank you!

ukBaz commented 1 year ago

I think this reached a conclusion so closing

chadrockey commented 8 months ago

@ukBaz I'm seeing this issue as well.

There is an app (of not my design) that I am working to make a compatible bluezero application with. I'm getting error pop ups on iOS because BlueZero echoes the original packet and it isn't expecting that.

Here is their design pattern that is similar to the above: https://developer.nordicsemi.com/nRF_Connect_SDK/doc/latest/nrf/libraries/bluetooth_services/services/wifi_prov.html#service-characteristics

Is it as described above where bluezero setting the last known received value causes bluezero to echo that value back to the central?

ukBaz commented 8 months ago

Hi @chadrockey,

I'm not sure how to respond to this. IIRC the behaviour is caused by the setting of the characteristic property always firing a PropertiesChanged notification. I've not tested this recently but I'm assuming you need notifications on for this to be an issue.

Reference: https://github.com/ukBaz/python-bluezero/blob/2b0aba891655bae44c1f281852d5669d5dc9db19/bluezero/localGATT.py#L195

This was done somewhat deliberately at the time to support the design pattern to support the UART service on the micro:bit. And making getting up and running easy.

Do you have a proposal for how you would like to move this forward? Should we move this to a new issue and just reference this one as this one is closed.

chadrockey commented 8 months ago

It's totally your call whether to reopen this issue or if you'd like to discuss on a new issue. My understanding is that this is the exact same issue given that commenting out the Set line within WriteValue appears to work without issue in this situation.

I did find other situations where write+notify can coexist (with read too), one example is the Curent Time Service https://www.bluetooth.com/specifications/specs/current-time-service-1-1/

Given that the app I'm working on is also from Nordic themselves, I feel as if we should determine what the correct behavior should be and find a way to make it work. We may need to find hardware that supports current time service and I may need to implement test services on an nRF52840 I have floating around.

My current questions are: since the user will write values with set_value is WriteValue exclusively for when an external device writes to our characteristic?

Does bluezero have any internal memory for the read/write functions? Could you explain further on:

The design idea behind what has been done is to have a property that could be be written and read to/from.

I'm not clear on the requirement. Is there a use case where you'd like an external device to write a value, then our bluezero code later reads our own characteristic without the user needing to cache the value themselves? Does this not use the write_callback?

Do dbus propertieschanged come with an originating source or other way to know it wasn't our bluezero app that did the write? In this case we could skip propertieschanged if we understand it wasn't us originating the data.

ukBaz commented 8 months ago

I'm not ignoring this, I just haven't had the time to dig in to the details and refresh myself.

Given that the app I'm working on is also from Nordic themselves, I feel as if we should determine what the correct behavior should be and find a way to make it work.

I very much agree with this statement. I would very much like to proceed by determining the correct behaviour and make it work that way.

One of the big challenges with this library is having tests that ensure the correct behaviour is being adhered to. Having a hardware test case is great, it is also difficult to share with other contributors. I accept that building a hardware test case is probably the first step; it would also be great if you could keep in the back of your mind if something could be done to make this easy for other people to test also.