[PATCH] device: Add device type property

[PATCH] device: Add device type property

[Sonny Sasaka]

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(+)

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,
-- 
2.24.1

Re: [PATCH] device: Add device type property

[Marcel Holtmann]

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

Re: [PATCH] device: Add device type property

[Sonny Sasaka]

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
>

Re: [PATCH] device: Add device type property

[Marcel Holtmann]

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

Re: [PATCH] device: Add device type property

[Sonny Sasaka]

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
>

Re: [PATCH] device: Add device type property

[Marcel Holtmann]

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

Re: [PATCH] device: Add device type property

[Sonny Sasaka]

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
>

Re: [PATCH] device: Add device type property

[Marcel Holtmann]

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

Re: [PATCH] device: Add device type property

[Sonny Sasaka]

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
>

Re: [PATCH] device: Add device type property

[Luiz Augusto von Dentz]

Hi Szymon,

On Thu, Feb 9, 2017 at 10:07 AM, Szymon Janc <szymon.janc@codecoup.pl> wrot=
e:
> Hi,
>
> On 9 February 2017 at 08:37, Marcel Holtmann <marcel@holtmann.org> wrote:
>> Hi Eric,
>>
>>> 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.
>>>
>>> This is tested with the following steps:
>>> Scan for devices and request the type property of a specific remote dev=
ice,
>>> using:
>>>  # dbus-send --print-reply --system --dest=3Dorg.bluez <obj path> \
>>>    org.freedesktop.DBus.Properties.Get \
>>>      string:org.bluez.Device1 string:Type
>>> or request the type of all remote devices, using:
>>>  # dbus-send --print-reply --system --dest=3Dorg.bluez / \
>>>    org.freedesktop.DBus.ObjectManager.GetManagedObjects | \
>>>      grep -B1 -A2 Type
>>> and check for "BR/EDR", "LE", and "DUAL"
>>> ---
>>> doc/device-api.txt |  5 +++++
>>> src/device.c       | 31 +++++++++++++++++++++++++++++++
>>> 2 files changed, 36 insertions(+)
>>>
>>> diff --git a/doc/device-api.txt b/doc/device-api.txt
>>> index 13b28818e..2f3100cd5 100644
>>> --- a/doc/device-api.txt
>>> +++ b/doc/device-api.txt
>>> @@ -141,6 +141,11 @@ Properties       string Address [readonly]
>>>
>>>                       The Bluetooth class of device of the remote devic=
e.
>>>
>>> +             string Type [readonly, optional]
>>> +
>>> +                     The carriers supported by this remote device. If =
it
>>> +                     exists, it can be one of "BR/EDR", "LE", or "DUAL=
".
>>> +
>>
>> I don=E2=80=99t like upper-case for values of this type. We have kept th=
em normally all lower-case.
>>
>> Anyhow, actually you can deduct this information already from existing C=
lass and Appearance values. If Class exists it is BR/EDR and if Appearance =
exists it is LE. If both exist it is dual-mode device. Only trick part migh=
t be that Appearance is optional, but we could just fill it in with some ge=
neric value. Would need to look that up.
>
> I think that if we decide to go with dedicated prop for this, it
> should rather be an array of strings of supported technologies
> eg [LE, BR/EDR] instead of string for device type. That way we could
> leave room for extending those easily if needed in
> the future (eg with AMP ?).

+1, although Im not sure about AMP there might be some other bearers
coming that could use it. Btw, in the long term this should really be
different interfaces for each bearer so the client would be able to
detect the supported bearers by checking what interfaces as available.

--=20
Luiz Augusto von Dentz

Re: [PATCH] device: Add device type property

[Szymon Janc]

Hi,

On 9 February 2017 at 08:37, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Eric,
>
>> 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.
>>
>> This is tested with the following steps:
>> Scan for devices and request the type property of a specific remote devi=
ce,
>> using:
>>  # dbus-send --print-reply --system --dest=3Dorg.bluez <obj path> \
>>    org.freedesktop.DBus.Properties.Get \
>>      string:org.bluez.Device1 string:Type
>> or request the type of all remote devices, using:
>>  # dbus-send --print-reply --system --dest=3Dorg.bluez / \
>>    org.freedesktop.DBus.ObjectManager.GetManagedObjects | \
>>      grep -B1 -A2 Type
>> and check for "BR/EDR", "LE", and "DUAL"
>> ---
>> doc/device-api.txt |  5 +++++
>> src/device.c       | 31 +++++++++++++++++++++++++++++++
>> 2 files changed, 36 insertions(+)
>>
>> diff --git a/doc/device-api.txt b/doc/device-api.txt
>> index 13b28818e..2f3100cd5 100644
>> --- a/doc/device-api.txt
>> +++ b/doc/device-api.txt
>> @@ -141,6 +141,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 i=
t
>> +                     exists, it can be one of "BR/EDR", "LE", or "DUAL"=
.
>> +
>
> I don=E2=80=99t like upper-case for values of this type. We have kept the=
m normally all lower-case.
>
> Anyhow, actually you can deduct this information already from existing Cl=
ass and Appearance values. If Class exists it is BR/EDR and if Appearance e=
xists it is LE. If both exist it is dual-mode device. Only trick part might=
 be that Appearance is optional, but we could just fill it in with some gen=
eric value. Would need to look that up.

I think that if we decide to go with dedicated prop for this, it
should rather be an array of strings of supported technologies
eg [LE, BR/EDR] instead of string for device type. That way we could
leave room for extending those easily if needed in
the future (eg with AMP ?).

--=20
pozdrawiam
Szymon K. Janc

Re: [PATCH] device: Add device type property

[Marcel Holtmann]

Hi Eric,

> 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.
> 
> This is tested with the following steps:
> Scan for devices and request the type property of a specific remote device,
> using:
>  # dbus-send --print-reply --system --dest=org.bluez <obj path> \
>    org.freedesktop.DBus.Properties.Get \
>      string:org.bluez.Device1 string:Type
> or request the type of all remote devices, using:
>  # dbus-send --print-reply --system --dest=org.bluez / \
>    org.freedesktop.DBus.ObjectManager.GetManagedObjects | \
>      grep -B1 -A2 Type
> and check for "BR/EDR", "LE", and "DUAL"
> ---
> doc/device-api.txt |  5 +++++
> src/device.c       | 31 +++++++++++++++++++++++++++++++
> 2 files changed, 36 insertions(+)
> 
> diff --git a/doc/device-api.txt b/doc/device-api.txt
> index 13b28818e..2f3100cd5 100644
> --- a/doc/device-api.txt
> +++ b/doc/device-api.txt
> @@ -141,6 +141,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".
> +

I don’t like upper-case for values of this type. We have kept them normally all lower-case.

Anyhow, actually you can deduct this information already from existing Class and Appearance values. If Class exists it is BR/EDR and if Appearance exists it is LE. If both exist it is dual-mode device. Only trick part might be that Appearance is optional, but we could just fill it in with some generic value. Would need to look that up.

Regards

Marcel

[PATCH] device: Add device type property

[mcchou]

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.

This is tested with the following steps:
Scan for devices and request the type property of a specific remote device,
using:
  # dbus-send --print-reply --system --dest=org.bluez <obj path> \
    org.freedesktop.DBus.Properties.Get \
      string:org.bluez.Device1 string:Type
or request the type of all remote devices, using:
  # dbus-send --print-reply --system --dest=org.bluez / \
    org.freedesktop.DBus.ObjectManager.GetManagedObjects | \
      grep -B1 -A2 Type
and check for "BR/EDR", "LE", and "DUAL"
---
 doc/device-api.txt |  5 +++++
 src/device.c       | 31 +++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/doc/device-api.txt b/doc/device-api.txt
index 13b28818e..2f3100cd5 100644
--- a/doc/device-api.txt
+++ b/doc/device-api.txt
@@ -141,6 +141,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 8693eb826..14c62e127 100644
--- a/src/device.c
+++ b/src/device.c
@@ -788,6 +788,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)
 {
@@ -2541,6 +2570,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,
--
2.11.0.483.g087da7b7c-goog