| Message ID | 20200401221320.12105-1-sonnysasaka@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Marcel Holtmann |
| Headers | show |
| Series | device: Add device type property | expand |
Hi Sonny, > This allows us to gather information about whether a device > supports BR/EDR, BLE, or both. It appears as DBus Property > "Type" on the org.bluez.Device1 interface. > --- > doc/device-api.txt | 5 +++++ > src/device.c | 31 +++++++++++++++++++++++++++++++ > 2 files changed, 36 insertions(+) my preference is not to combine API documentation patches with code. > > diff --git a/doc/device-api.txt b/doc/device-api.txt > index 65d8fee37..ceb68d2f6 100644 > --- a/doc/device-api.txt > +++ b/doc/device-api.txt > @@ -158,6 +158,11 @@ Properties string Address [readonly] > > The Bluetooth class of device of the remote device. > > + string Type [readonly, optional] > + > + The carriers supported by this remote device. If it > + exists, it can be one of "BR/EDR", "LE", or "DUAL". > + So all values need to be lower case. That is just how we design all API. If we do this, then I think the name “Bearer” might be better. Also it might be better as array{string} actually. Regards Marcel
Thanks for the feedback, Marcel. Will come back with the modified patches based on your suggestions. On Thu, Apr 9, 2020 at 11:12 AM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Sonny, > > > This allows us to gather information about whether a device > > supports BR/EDR, BLE, or both. It appears as DBus Property > > "Type" on the org.bluez.Device1 interface. > > --- > > doc/device-api.txt | 5 +++++ > > src/device.c | 31 +++++++++++++++++++++++++++++++ > > 2 files changed, 36 insertions(+) > > my preference is not to combine API documentation patches with code. > > > > > diff --git a/doc/device-api.txt b/doc/device-api.txt > > index 65d8fee37..ceb68d2f6 100644 > > --- a/doc/device-api.txt > > +++ b/doc/device-api.txt > > @@ -158,6 +158,11 @@ Properties string Address [readonly] > > > > The Bluetooth class of device of the remote device. > > > > + string Type [readonly, optional] > > + > > + The carriers supported by this remote device. If it > > + exists, it can be one of "BR/EDR", "LE", or "DUAL". > > + > > So all values need to be lower case. That is just how we design all API. > > If we do this, then I think the name “Bearer” might be better. Also it might be better as array{string} actually. > > Regards > > Marcel >
Hi Sonny, > Thanks for the feedback, Marcel. Will come back with the modified > patches based on your suggestions. I also meant to write that the name “Bearer” might not be the best either. I am open for suggestions. Also if it is an array, it needs to be plural of course. If we start exposing BR/EDR vs LE or its combination (and we might also consider HS even while not used these days), then we need to be consistent with our terminology across the APIs. Regards Marcel
Hi Marcel, After giving it more thought, I would like to propose that this additional property corresponds with Device Type as defined in Generic Access Profile: As stated in Bluetooth Core Specification Version 5.2, Vol 3, Part C (Generic Access Profile): This profile defines three device types based on the supported Core Configurations as defined in [Vol 0] Part B, Section 3.1. The device types are shown in Table 1.1: * BR/EDR * LE only * BR/EDR/LE Therefore, to be as close to the spec as possible, I propose that the property be named GAPDeviceType, and the possible values be "br/edr", "le-only", "br/edr/le". What do you think? Thanks for reviewing this proposal! On Thu, Apr 9, 2020 at 11:51 PM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Sonny, > > > Thanks for the feedback, Marcel. Will come back with the modified > > patches based on your suggestions. > > I also meant to write that the name “Bearer” might not be the best either. I am open for suggestions. Also if it is an array, it needs to be plural of course. If we start exposing BR/EDR vs LE or its combination (and we might also consider HS even while not used these days), then we need to be consistent with our terminology across the APIs. > > Regards > > Marcel >
Hi Sonny, > After giving it more thought, I would like to propose that this > additional property corresponds with Device Type as defined in Generic > Access Profile: > > As stated in Bluetooth Core Specification Version 5.2, Vol 3, Part C > (Generic Access Profile): > This profile defines three device types based on the supported Core > Configurations as defined in [Vol 0] Part B, Section 3.1. The device > types are shown in Table 1.1: > * BR/EDR > * LE only > * BR/EDR/LE > > Therefore, to be as close to the spec as possible, I propose that the > property be named GAPDeviceType, and the possible values be "br/edr", > "le-only", "br/edr/le". > > What do you think? Thanks for reviewing this proposal! maybe we should do SupportedTypes = [“le”, “bredr”, “hs”] since that also maps to what we expose in MGMT. And then add a Types = [ .. ] property with the same values. I don’t like using GAP in property names and repeating Device is also not needed since we are already in the Device interface. In addition I have a reservation with spec naming in this area since it has changed over time and there is a chance it might change again in future specs when new features come along. If you know the supported types of the hardware and the selected types, you can easily get to the GAP Device Type from the spec if you actually want to show it. If you wanted to, you could make the Types property writeable as well and users could change their device type via D-Bus. Regards Marcel
Hi Marcel, I am okay with Types = ["le", "bredr"]. However, I don't understand why a user should be able to change the Type, since this property describes the fact about a peer device, not a local adapter. What does it mean by a user changing the type of a peer device? Also, I don't understand why HS needs to be considered in that property, since I see org.bluez.Device1 objects as discovered devices either through Inquiry (in which case it'd be "bredr") or Advertisement (in which case it'd be "le"), or both. HS seems to be one of remote features rather than a type. Also the HS information is also not readily available in the struct btd_device, or even src/device.c, which suggests that it has been treated differently. On Tue, May 19, 2020 at 11:50 PM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Sonny, > > > After giving it more thought, I would like to propose that this > > additional property corresponds with Device Type as defined in Generic > > Access Profile: > > > > As stated in Bluetooth Core Specification Version 5.2, Vol 3, Part C > > (Generic Access Profile): > > This profile defines three device types based on the supported Core > > Configurations as defined in [Vol 0] Part B, Section 3.1. The device > > types are shown in Table 1.1: > > * BR/EDR > > * LE only > > * BR/EDR/LE > > > > Therefore, to be as close to the spec as possible, I propose that the > > property be named GAPDeviceType, and the possible values be "br/edr", > > "le-only", "br/edr/le". > > > > What do you think? Thanks for reviewing this proposal! > > maybe we should do SupportedTypes = [“le”, “bredr”, “hs”] since that also maps to what we expose in MGMT. And then add a Types = [ .. ] property with the same values. I don’t like using GAP in property names and repeating Device is also not needed since we are already in the Device interface. In addition I have a reservation with spec naming in this area since it has changed over time and there is a chance it might change again in future specs when new features come along. > > If you know the supported types of the hardware and the selected types, you can easily get to the GAP Device Type from the spec if you actually want to show it. If you wanted to, you could make the Types property writeable as well and users could change their device type via D-Bus. > > Regards > > Marcel >
Hi Sonny, > I am okay with Types = ["le", "bredr"]. However, I don't understand > why a user should be able to change the Type, since this property > describes the fact about a peer device, not a local adapter. What does > it mean by a user changing the type of a peer device? Also, I don't > understand why HS needs to be considered in that property, since I see > org.bluez.Device1 objects as discovered devices either through Inquiry > (in which case it'd be "bredr") or Advertisement (in which case it'd > be "le"), or both. HS seems to be one of remote features rather than a > type. Also the HS information is also not readily available in the > struct btd_device, or even src/device.c, which suggests that it has > been treated differently. you are correct. I was thinking about the local role. My bad. Regards Marcel
Thanks for confirming, Marcel. I will come back with the modified patch based on your feedback. On Thu, May 21, 2020 at 12:02 AM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Sonny, > > > I am okay with Types = ["le", "bredr"]. However, I don't understand > > why a user should be able to change the Type, since this property > > describes the fact about a peer device, not a local adapter. What does > > it mean by a user changing the type of a peer device? Also, I don't > > understand why HS needs to be considered in that property, since I see > > org.bluez.Device1 objects as discovered devices either through Inquiry > > (in which case it'd be "bredr") or Advertisement (in which case it'd > > be "le"), or both. HS seems to be one of remote features rather than a > > type. Also the HS information is also not readily available in the > > struct btd_device, or even src/device.c, which suggests that it has > > been treated differently. > > you are correct. I was thinking about the local role. My bad. > > Regards > > Marcel >
diff --git a/doc/device-api.txt b/doc/device-api.txt index 65d8fee37..ceb68d2f6 100644 --- a/doc/device-api.txt +++ b/doc/device-api.txt @@ -158,6 +158,11 @@ Properties string Address [readonly] The Bluetooth class of device of the remote device. + string Type [readonly, optional] + + The carriers supported by this remote device. If it + exists, it can be one of "BR/EDR", "LE", or "DUAL". + uint16 Appearance [readonly, optional] External appearance of device, as found on GAP service. diff --git a/src/device.c b/src/device.c index 5f9ad227d..ace9c348c 100644 --- a/src/device.c +++ b/src/device.c @@ -849,6 +849,35 @@ static gboolean dev_property_get_class(const GDBusPropertyTable *property, return TRUE; } +static gboolean dev_property_exists_type(const GDBusPropertyTable *property, + void *data) +{ + struct btd_device *device = data; + + return device->bredr || device->le; +} + +static gboolean dev_property_get_type(const GDBusPropertyTable *property, + DBusMessageIter *iter, void *data) +{ + struct btd_device *device = data; + const char *type; + + if (!device->bredr && !device->le) + return FALSE; + + if (!device->bredr) + type = "LE"; + else if (!device->le) + type = "BR/EDR"; + else + type = "DUAL"; + + dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &type); + + return TRUE; +} + static gboolean get_appearance(const GDBusPropertyTable *property, void *data, uint16_t *appearance) { @@ -2752,6 +2781,8 @@ static const GDBusPropertyTable device_properties[] = { { "Alias", "s", dev_property_get_alias, dev_property_set_alias }, { "Class", "u", dev_property_get_class, NULL, dev_property_exists_class }, + { "Type", "s", dev_property_get_type, NULL, + dev_property_exists_type }, { "Appearance", "q", dev_property_get_appearance, NULL, dev_property_exists_appearance }, { "Icon", "s", dev_property_get_icon, NULL,
From: Eric Caruso <ejcaruso@chromium.org> This allows us to gather information about whether a device supports BR/EDR, BLE, or both. It appears as DBus Property "Type" on the org.bluez.Device1 interface. --- doc/device-api.txt | 5 +++++ src/device.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+)