Add Gaming Audio Profile
#606
Conversation
bumble/profiles/gmap.py
Outdated
| # ----------------------------------------------------------------------------- | ||
| # Classes | ||
| # ----------------------------------------------------------------------------- | ||
| class GmapRole(OpenIntEnum): |
There was a problem hiding this comment.
This should be an enum.IntFlag type, since flags can be combined.
The same applies to UggFeatures, UtgFeatures, BgsFeatures and BgrFeatures below.
bumble/profiles/gmap.py
Outdated
| ) | ||
| characteristics.append(self.gmap_role) | ||
|
|
||
| if ugg_features: |
There was a problem hiding this comment.
This should check for ugg_features is not None, because you could pass a 0-valued ugg_features parameter, which would evaluate to False in the test, but you still want a characteristic to exist.
The same applies to the other characteristics below.
bumble/profiles/gmap.py
Outdated
| uuid=GATT_UGG_FEATURES_CHARACTERISTIC, | ||
| properties=Characteristic.Properties.READ, | ||
| permissions=Characteristic.Permissions.READABLE, | ||
| value=struct.pack('<B', ugg_features), |
There was a problem hiding this comment.
nit: no need for < in the struct pattern, since there's no byte order for just one byte.
Adds initial support for `Gaming Audio Service`.
| characteristics.append(self.gmap_role) | ||
|
|
||
| if ugg_features is not None: | ||
| self.ugg_features = Characteristic( |
There was a problem hiding this comment.
You still need to set a value when parameters are None, otherwise the attribute will be missing.
There was a problem hiding this comment.
Even if in given configuration a characteristic should not exist in the service?
As per the GMAP specification, section 4.7 Service characteristics, Table 4.1. GMAS characteristics, depending on the GMAP Role the other 4 characteristics are either mandatory or excluded, thus I added the role check before adding those conditional characteristics.
There was a problem hiding this comment.
Correct. It's not really related to Bluetooth, but from a readability perspective, you should always set a value for any attribute, especially when you annotate them as Optional, people will expect there is such an attribute even they are None.
Consider if you are using Java, such a pattern will probably be warned that you may access a member without initialization.
There was a problem hiding this comment.
Hi @zxzxwu , that would mean that we will have characteristics exposed which are not really needed.
Also from the testing point of view, I see value in having an option to create invalid test device which is missing some characteristics to verify proper error handling on the DUT.
Does it makes sense?
We also had an idea that some of the characteristic could be created as per spec (depends on the role) but in fact as it is now gives good test flexibility.
Other options would be to extend this constructor with a bool's which would tell if given characteristic should be created but this seems to be overkill.
There was a problem hiding this comment.
You don't need to create a characteristic. What is needed here is
| self.ugg_features = Characteristic( | |
| self.ugg_features = Characteristic( | |
| ... | |
| else: | |
| self.ugg_features = None |
Or set it like
class GamingAudioService(TemplateService):
UUID = GATT_GAMING_AUDIO_SERVICE
gmap_role: Characteristic
ugg_features: Optional[Characteristic] = None
ugt_features: Optional[Characteristic] = None
bgs_features: Optional[Characteristic] = None
bgr_features: Optional[Characteristic] = NoneThere was a problem hiding this comment.
Thank you, now it is clear. I added the latter suggestion.
Only adds a characteristic if the corresponding role has been set
Sets default values for characteristics if not specified explicitly
Adds initial support for
Gaming Audio Service.