Add prefer_notify option to gatt_client subscribe() #71
Conversation
|
Fix for #67 |
mogenson
force-pushed
the
prefer-notify
branch
from
bf551ee
to
56a76dc
Compare
mogenson
force-pushed
the
prefer-notify
branch
from
56a76dc
to
c51a4b1
Compare
bumble/gatt_client.py
Outdated
| bits |= ClientCharacteristicConfigurationBits.INDICATION | ||
| subscriber_sets.append(self.indication_subscribers.setdefault(characteristic.handle, set())) | ||
| def characteristic_is_notify(characteristic, prefer_notify): | ||
| if ( |
There was a problem hiding this comment.
suggest: more readable as: if characteristic.properties & Characteristic.NOTIFY and characteristic.properties & Characteristic.INDICATE
There was a problem hiding this comment.
parens aren't needed
There was a problem hiding this comment.
This is the formatting that black chose, so I'll stick with it.
bumble/gatt_client.py
Outdated
| if ( | ||
| characteristic.properties & (Characteristic.NOTIFY | Characteristic.INDICATE) | ||
| ) == (Characteristic.NOTIFY | Characteristic.INDICATE): | ||
| return True if prefer_notify else False |
There was a problem hiding this comment.
return prefer_notify would be sufficient
mogenson
force-pushed
the
prefer-notify
branch
from
c51a4b1
to
aa67dc6
Compare
| # emitting an 'update' event when a notification or indication is received | ||
| subscriber_set.add(characteristic) | ||
| subscriber_set = subscribers.setdefault(characteristic.handle, set()) | ||
| if subscriber is not None: |
There was a problem hiding this comment.
if subscriber: is sufficient
| @@ -558,23 +555,32 @@ async def subscribe(self, characteristic, subscriber=None): | |||
| logger.warning('subscribing to characteristic with no CCCD descriptor') | |||
There was a problem hiding this comment.
OOS, but this warning is wrong. we don't know if theres no CCCD as we just discovered descriptors and haven't checked if CCCD exists
There was a problem hiding this comment.
The line above gets the CCCD only if it exists:
cccd = characteristic.get_descriptor(GATT_CLIENT_CHARACTERISTIC_CONFIGURATION_DESCRIPTOR)
bumble/gatt_client.py
Outdated
|
|
||
| bits, subscribers = ( | ||
| (ClientCharacteristicConfigurationBits.NOTIFICATION, self.notification_subscribers) | ||
| if characteristic_is_notify(characteristic, prefer_notify) |
There was a problem hiding this comment.
i found this block (571-575) a bit hard to read. it's a tuple + if expression.
consider converting it to a if statement
There was a problem hiding this comment.
This may be a matter of opinion. I prefer the more succinct if-expression to the following if-statement.
bits = None
subscribers = None
if characteristic_is_notify(characteristic, prefer_notify):
bits = ClientCharacteristicConfigurationBits.Notification
subscribers = self.notification_subscribers
else:
bits = ClientCharacteristicConfigurationBits.INDICATION
subscribers = self.indication_subscribers
Since they both do exactly the same thing, I'll leave it as-is.
There was a problem hiding this comment.
You could also put the bits and subscribers assignments in the if/else statement used for characteristic_is_notify (and thus get rid of the local function).
Also, since there's always at least one branch taken, you don't need to initialize bits and subscribers to None, they'll always be set in one of the branches.
mogenson
force-pushed
the
prefer-notify
branch
2 times, most recently
from
5e718e3
to
accb03f
Compare
bumble/gatt_client.py
Outdated
| # emitting an 'update' event when a notification or indication is received | ||
| subscriber_set.add(characteristic) | ||
| subscriber_set = subscribers.setdefault(characteristic.handle, set()) | ||
| if subscriber: |
There was a problem hiding this comment.
comparing to None should be explicit here (if subscriber is not None:). Even though unlikely, a not-None argument could still be "falsy", and thus would be ignored here.
There was a problem hiding this comment.
Ok, I'll change it back.
If characteristic supports Notify and Indicate, the prefer_notify option will subscribe with Notify if True or Indicate if False. If characteristic only supports one property, Notify or Indicate, that mode will be selected, regardless of the prefer_notify setting. Tested with a characteristic that supports both Notify and Indicate and verified that prefer_notify sets the desired mode.
mogenson
force-pushed
the
prefer-notify
branch
from
accb03f
to
c140876
Compare
If characteristic supports Notify and Indicate, the prefer_notify option will subscribe with Notify if True or Indicate if False.
If characteristic only supports one property, Notify or Indicate, that mode will be selected, regardless of the prefer_notify setting.
Tested with a characteristic that supports both Notify and Indicate and verified that prefer_notify sets the desired mode.