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

IncludeTxPower is not longer valid since mid 2020 #333

Open
tnarik opened this issue Jun 3, 2021 · 6 comments
Open

IncludeTxPower is not longer valid since mid 2020 #333

tnarik opened this issue Jun 3, 2021 · 6 comments

Comments

@tnarik
Copy link

tnarik commented Jun 3, 2021

See:

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

@ukBaz
Copy link
Owner

ukBaz commented Jun 4, 2021

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

tnarik commented Jun 4, 2021

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

ukBaz commented Jun 4, 2021

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

@property
def include_tx_power(self):
"""Include TX power in advert (Different from beacon packet)"""
return self.Get(constants.LE_ADVERTISEMENT_IFACE,
'IncludeTxPower')
@include_tx_power.setter
def include_tx_power(self, state):
return self.Set(constants.LE_ADVERTISEMENT_IFACE,
'IncludeTxPower', state)

It would be the props values that would need to change:

self.props = {
constants.LE_ADVERTISEMENT_IFACE: {
'Type': ad_type,
'ServiceUUIDs': None,
'ManufacturerData': None,
'SolicitUUIDs': None,
'ServiceData': None,
'IncludeTxPower': False,
'Appearance': None,
'LocalName': None
}
}

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.

response['IncludeTxPower'] = dbus.Boolean(
self.props[interface_name]['IncludeTxPower'])

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

tnarik commented Jun 4, 2021

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

tnarik commented Jun 4, 2021

Let me try to produce a PR during the weekend.

@ukBaz
Copy link
Owner

ukBaz commented Jun 4, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants