Add Volume Offset Control Service
#585
Conversation
bumble/profiles/vocs.py
Outdated
| attribute=self.audio_channel_location, | ||
| value=self.audio_location_bytes, | ||
| ) | ||
| self.emit('audio_location', self.audio_location) |
There was a problem hiding this comment.
Typically, it is preferable to not repeat in the event the changed state if that state already exists as a property of the emitter.
Here, for instance (the same idea applies to the other placed in this PR where events are emitted), a listener can access audio_location from the emitter, so there's no need to include that in the event.
a listener would do:
service.on('audio_location_change', ...)
and in the event handler, retrieve that actual values as service.audio_location.
There was a problem hiding this comment.
Thank you for the explanation. I removed these unnecessary properties from the events.
There was a problem hiding this comment.
In the current version of the service I was not sure how to handle events, so I removed them completely. If they are necessary, could you please give me some example?
bumble/profiles/vocs.py
Outdated
| '<hB', | ||
| ) | ||
|
|
||
| self.audio_location = gatt.PackedCharacteristicAdapter( |
There was a problem hiding this comment.
audio_location values are enums. We have an enum type for is already, in bap.AudioLocation. This could use a DelegatedCharacteristicAdapter to encode/decode to/from that type.
There was a problem hiding this comment.
Now I used these adapters in conjunction with from_bytes method which handles the enum type conversion. Is such usage correct?
bumble/profiles/vocs.py
Outdated
| super().__init__(characteristics=characteristics, primary=False) | ||
|
|
||
| @property | ||
| def volume_offset_state_bytes(self) -> bytes: |
There was a problem hiding this comment.
(this comment also applies to the other characteristics in this service)
Instead of having customized bytes serialization methods here, you should have shared adapters, used both for the service and service proxy.
Look at the heart_rate_service.py for an example of how this may be done.
There was a problem hiding this comment.
I simplified the service implementation and added standalone classes for each characteristic with shared methods as you suggested.
bumble/profiles/vocs.py
Outdated
| MAX_VOLUME_OFFSET = 255 | ||
| CHANGE_COUNTER_MAX_VALUE = 0xFF | ||
|
|
||
| SET_VOLUME_OFFSET_OPCODE = 0x01 |
There was a problem hiding this comment.
Though it might never extend, an enum opcode is more extendable.
There was a problem hiding this comment.
Okay, I changed that to an enum.
Adds initial support for VOCS.
Adds initial support for VOCS.