ukBaz / python-bluezero

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

IncludeTxPower is not longer valid since mid 2020 #333

Open tnarik opened 3 years ago

tnarik commented 3 years ago

See:

https://linuxlists.cc/l/15/linux-bluetooth/t/3585256/(patch_bluez)_test_example-advertisement:_fix_include_tx_power

ukBaz commented 3 years ago

A link to an archive without advertising: https://marc.info/?l=linux-bluetooth&m=159000784202676&w=2

The patch reads as follows:

List:       linux-bluetooth
Subject:    [PATCH BlueZ] test/example-advertisement: Fix include_tx_power
From:       Alvar Penning <post () 0x21 ! biz>
Date:       2020-05-20 20:44:44
Message-ID: 20200520204444.28994-1-post () 0x21 ! biz
[Download RAW message or body]

Adding the Tx Power Level is no longer done via IncludeTxPower, but via
the tx-power value in the Includes array. The previous code did not
throw an error, but neither led to the insertion of the value. As a
result of this change, include_tx_power now adds the Tx Power Level
again.
---
 test/example-advertisement | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/test/example-advertisement b/test/example-advertisement
index f116893b6..96e410683 100755
--- a/test/example-advertisement
+++ b/test/example-advertisement
@@ -57,7 +57,7 @@ class Advertisement(dbus.service.Object):
         self.solicit_uuids = None
         self.service_data = None
         self.local_name = None
-        self.include_tx_power = None
+        self.include_tx_power = False
         self.data = None
         dbus.service.Object.__init__(self, bus, self.path)

@@ -78,8 +78,8 @@ class Advertisement(dbus.service.Object):
                                                         signature='sv')
         if self.local_name is not None:
             properties['LocalName'] = dbus.String(self.local_name)
-        if self.include_tx_power is not None:
-            properties['IncludeTxPower'] = dbus.Boolean(self.include_tx_power)
+        if self.include_tx_power:
+            properties['Includes'] = dbus.Array(["tx-power"], signature='s')

         if self.data is not None:
             properties['Data'] = dbus.Dictionary(
-- 
2.25.4

This appears to be a change introduced in BlueZ 5.47.

The possible values for Includes on interface org.bluez.LEAdvertisement1 are tx-power, appearance, and local-name. Today Bluezero doesn't set a value for Includes at all yet setting values for Appearance and LocalName results in them being included so I'm not sure how all of these interact.

@tnarik: Have to done any testing of this? Are you planning on submitting a PR to Bluezero with these updates?

tnarik commented 3 years ago

I'm currently monkey-patching on my application to get the power. Wasn't sure how you want to handle this. In my case I'm mainly using a Peripheral and then manipulating the peri.advert to get manufacturer data and power advertised.

Do not have much experience with d-bus (started playing with the whole Bluez, Bluetooth on Pi and D-Bus last Saturday).

ukBaz commented 3 years ago

Be interested to see how you are monkey-patching it on your application. I am assuming it is peri.advert.Set('org.bluez.LEAdvertisement1', 'Includes', '["tx-power"]' Or are you having to do something more complicated? (as a side note for my understanding of how people use the library: If you adding all this information to the advert, are you not over the 28 bytes limit? Also, what company id are you using for the manufacturer data?)

The way for the library to address this is to modify the file https://github.com/ukBaz/python-bluezero/blob/main/bluezero/advertisement.py

Ideally the Bluezero advertisement API would stay the same i.e. keep the include_tx_power getter and setter https://github.com/ukBaz/python-bluezero/blob/392a115ace80a6d7257b76708ec24b99ec9e2e5d/bluezero/advertisement.py#L149-L158

It would be the props values that would need to change: https://github.com/ukBaz/python-bluezero/blob/392a115ace80a6d7257b76708ec24b99ec9e2e5d/bluezero/advertisement.py#L75-L86

Replace IncludeTxPower with Includes. e.g:

        self.props = {
            constants.LE_ADVERTISEMENT_IFACE: {
                'Type': ad_type,
                'ServiceUUIDs': None,
                'ManufacturerData': None,
                'SolicitUUIDs': None,
                'ServiceData': None,
                'Includes': set(),
                'Appearance': None,
                'LocalName': None
            }
        }

And the getter/setter might be:

    @property
    def include_tx_power(self):
        """Include TX power in advert (Different from beacon packet)"""
        includes = self.Get(constants.LE_ADVERTISEMENT_IFACE, 'Includes')
        return 'tx-power' in includes

    @include_tx_power.setter
    def include_tx_power(self, state):
        if state:
            includes = self.props[interface_name]['Includes'].add('tx-power')
        else:
            includes = self.props[interface_name]['Includes'].remove('tx-power')
        return self.Set(constants.LE_ADVERTISEMENT_IFACE, includes)

The other thing that looks like it would need to change is the GetAll. https://github.com/ukBaz/python-bluezero/blob/392a115ace80a6d7257b76708ec24b99ec9e2e5d/bluezero/advertisement.py#L219-L220

To maybe:

        response['Includes'] = dbus.Array(
            self.props[interface_name]['Includes'], signature='s')

The above is all untested and just me sharing my thoughts. I would also like to understand about how Includes interacts with Appearance, LocalName, and LEAdvertisingManager.SupportedIncludes

If I don't hear that you are going to do a PR for this, then I'll take a look in a couple of weeks time.

tnarik commented 3 years ago

Had to do this:

    thing.advert.props['org.bluez.LEAdvertisement1']['Includes'] = []

    old_getall = Advertisement.GetAll
    @dbus.service.method('org.freedesktop.DBus.Properties',
                         in_signature='s',
                         out_signature='a{sv}')
    def new_getall(self, interface_name):
        # print('CALLING THE PATCHED 2')
        response = old_getall(self, interface_name)
        # Our patch
        if 'Includes' in self.props[interface_name]:
            if self.props[interface_name]['Includes'] is not None:
               response['Includes'] = self.props[interface_name]['Includes']
        return response
    Advertisement.GetAll = new_getall

    # pprint(thing.advert.GetAll('org.bluez.LEAdvertisement1'))
    thing.advert.Set('org.bluez.LEAdvertisement1', 'Includes', dbus.Array(["tx-power"], signature='s'))

But it is also the first time I try to monkey-patch Python. I was not seeing the Includes on the via D-Bus without adding it to GetAll.

Regarding the data limit, I am still playing around: put something in, take something out. And configuring several advertisements to check what is possible. As company ID I was using the 0xFFFF but also Canon's and Apple's, just to see if nRF was responding to that.

tnarik commented 3 years ago

Let me try to produce a PR during the weekend.

ukBaz commented 3 years ago

Yes you are correct, GetAll would need to be monkey-patched also for it to work.

Thanks for having a look at the PR. Happy to help with any issues or questions that arise.