2014-07-19 06:06:47

by Arman Uguray

[permalink] [raw]
Subject: [RFC 0/1] doc/gatt-api: New API properties and methods for the GATT D-Bus API.

In the upcoming beta release of Chrome OS (which uses BlueZ as its Bluetooth
stack) we have implemented the chrome.bluetoothLowEnergy JavaScript API for
Chrome browser applications
(https://developer.chrome.com/apps/bluetoothLowEnergy). For Chrome OS, we wrote
an implementation of the proposed GATT D-Bus API for bluetoothd, during which we
ran into some issues with the current proposal and changed things up a bit.

This set includes one patch that proposes changes for the currently
unimplemented GATT D-Bus API for desktop bluetoothd. The following changes are
proposed:

* The addition of the following new properties:

- org.bluez.GattService1: "Primary", "Device", "Characteristics"
- org.bluez.GattCharacteristic1: "Notifying", "Descriptors"

While all characteristic and descriptor objects become available via
ObjectManager as soon as they get discovered, the "Characteristics" and
"Descriptors" properties of service and characteristic objects do not get set
until all characteristics and descriptors of a service have been discovered.
This is used to let applications know that discovery of a service is complete
and that the service is ready to interact with.

* Removal of the "Permissions" property from org.bluez.GattDescriptor1.

* Removal of the "Value" property from org.bluez.GattCharacteristic1 and
GattDescriptor1. The introduction of the following asynchronous methods and
signals:

- ReadValue method, which always reads the value of a characteristic or
descriptor from the remote device and returns the result asynchronously.
This removes the problems of the org.freedesktop.DBus.Properties.Get
method when the value is implemented as a property:

-- The Get and GetAll methods are synchronous. This causes problems when a
round-trip is required to obtain the value from the remote device.
-- The ambiguity involved in the role of Get/GetAll when a characteristic
doesn't support reads. Some characteristics are only writable and some
make their values known via notifications and not reads.

- WriteValue method, which is used to write the value of a characteristic or
descriptor.

- ValueUpdated signal, emitted when a characteristic notification or
indication is received.

* Addition of the StartNotify and StopNotify methods to
org.bluez.GattCharacteristic1. This allows multiple applications to enable
notifications from a characteristic without interfering with each other and
applications are not permitted to directly write to the "Client
Characteristic Configuration" descriptor.


Arman Uguray (1):
doc/gatt-api: New API properties and methods for the GATT D-Bus API.

doc/gatt-api.txt | 118 ++++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 104 insertions(+), 14 deletions(-)

--
2.0.0.526.g5318336



2014-07-21 22:07:23

by Arman Uguray

[permalink] [raw]
Subject: Re: [RFC 1/1] doc/gatt-api: New API properties and methods for the GATT D-Bus API.

Hi Marcel,

>>>> I am not sure this is needed. What do you want to use it for?
>>
>> This is very useful to determine which device a service belongs to
>> when an InterfacesAdded signal is sent for a service. We have an
>> "Adapter" property for org.bluez.Device1, so why not a "Device"
>> property for org.bluez.GattService1?
>
> the back reference should only be added if someone really uses them.
>

Well, we (the Chrome OS team) do, so that counts for something right
(:P)? We used it in our implementation and Chaojie also seems to be
interested in having this property so I think we should have it.

> So this is problem that InterfacesAdded is only reporting for a single ob=
ject path. Maybe this will actually work out as you described it.
>

Exactly.

> Okay, then make the error really simple. Maybe even on a high-level like =
"not paired". Since the only action from the user here is really to start p=
airing with the device.
>

Sounds good to me.

>> Yeah, I came up with these names pretty quickly while I was
>> time-pressed for the Chrome 37 release. I don't have a strong opinion
>> here; maybe EnableNotifications & DisableNotifications? Or maybe
>> something that contains the word "session" since these methods behave
>> like StartDiscovery and StopDiscovery on org.bluez.Adapter1.
>
> I leave it to others to comment here as well. It is an easy change in the=
end.
>

I'll leave it as it is for now, unless people object.

>> I added this signal because I got rid of the property. Even if we keep
>> the property, I'm still not convinced that using the PropertyChanged
>> signal to mean both "cached value updated after read" and
>> "notification/indication received" is the right idea. I sort of prefer
>> having a signal that only gets called when there's a notification,
>> though if we keep this property then I guess we're going to have to
>> work with that.
>
> My thinking is that it really does not matter how the value got updated. =
Either by a manual read or by a notification or even by a write. The import=
ance is to get the new value notified to all other applications that are no=
t involved. Doing it only one way seems to make sense. And the applications=
all already have to listen to the PropertiesChanged signal anyway. Less si=
gnals to listen on the the better.
>

Ok, I guess this isn't so bad.

>> To be honest, I don't think this property is that necessary. GATT/ATT
>> don't really define a way to obtain these requirements in the
>> characteristic declaration and leave them up to the upper layer
>> profile and the only way to find out about these requirements is to
>> issue a read/write request and see if there's an error. If we properly
>> report errors to the client on calls to ReadValue and WriteValue, then
>> this property wouldn't really be needed. Let me know if I'm missing
>> something though.
>
> We might actually update it from the property does not exist to we have f=
igured this out since someone read the property and thus we are reporting i=
t now. I might be wrong, but I think there was some part in GATT that provi=
ded these details.

Yeah, I guess this depends on how badly an application might need this
property. We can just automatically update it after a read/write
request. If we do want to keep it, then this property needs to be
added to org.bluez.GattCharacteristic1 as well. I say we leave this
property out for now and add it later if people feel that it's
valuable to have it.

-Arman

2014-07-21 21:32:17

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [RFC 1/1] doc/gatt-api: New API properties and methods for the GATT D-Bus API.

Hi Arman,

>> We are working on GATT D-Bus API for Tizen LE GATT Client too. For your proposal, I totally agree your part of proposal such as add Primary property into org.bluez.GattService1, because all the hierarchy services cannot differentiate between primary and included service. And device property is also very useful because It can help user to trace the services belong to which device.
>> However , you add Write/Read/Notify method into GATT-API , I think it is good proposal but not essential. Because it is about implementation method problem, not the GATT DBus API problem.
>
> I have to disagree here. I think making the DBus.Properties interface
> work for this particular case doesn't make the API particularly
> simpler, in fact I think it adds unnecessary complexity and ambiguity
> to the semantics involved. It's not clear, for instance, if Get should
> always return the cached value or if it should issue a Read request to
> the remote end. You suggest making an initial read request and storing
> that value and updating it on notifications. What if the
> characteristic doesn't support notifications but it's value can
> change? What if the client is implementing a profile that warrants a
> read request to the remote end to get the freshest value on demand?
>
> For example, take the "Device Name" characteristic of the Generic
> Access service, which has to support reads, optionally supports
> writes, and doesn't support notifications/indications. The client can
> write its value but won't be notified of the change. Now, the daemon
> could remember that the value was written and update the property
> right upon success, but I think this is unnecessary magic.
>
> Same with control point characteristics, that support writes but no
> notifications or reads. The external application can call "Set" to
> write to the characteristic but does it make sense to send a
> PropertiesChanged signal for a value that we cannot read? Should the
> daemon cache the write value and return that on Get? Or should it
> return an empty value?
>
> The same goes for the PropertyChanged signal, which we will end up
> emitting after a successful read, notifications/indications, and
> portentially writes. I think it's cleaner to have a distinct signal
> that gets only sent on notifications.
>
> We end up with a read-write property that doesn't return a consistent
> value on Get and Set calls. We end up making the property interface
> work for semantics that it wasn't exactly built for.
>
> I want to have an API where the external application has a clear way
> of saying "send a remote read/write request". I'm not opposed to
> having a "Value" property but I'd much rather it represent a read-only
> property that only returns the most recently cached value.
>
>>>> + object Device [read-only, optional]
>>>> +
>>>> + Object path of the Bluetooth device the service
>>>> + belongs to. Only present on services from remote
>>>> + devices.
>>>
>>> I am not sure this is needed. What do you want to use it for?
>
> This is very useful to determine which device a service belongs to
> when an InterfacesAdded signal is sent for a service. We have an
> "Adapter" property for org.bluez.Device1, so why not a "Device"
> property for org.bluez.GattService1?

the back reference should only be added if someone really uses them.

>
>>
>>>> +
>>>> + array{object} Characteristics [read-only]
>>>> +
>>>> + Array of object paths representing the characteristics
>>>> + of this service. This property is set only when the
>>>> + characteristic discovery has been completed, however the
>>>> + characteristic objects will become available via
>>>> + ObjectManager as soon as they get discovered.
>>>> +
>>>
>>> I really wonder if this is a good idea. Why not delay announcing any service or characteristic via D-Bus as long as the service discovery is still running. I think it makes more sense to allow doing it one way and one way only.
>>
>
> I guess I should have said "This property is set only when the
> characteristic discovery has been completed AND an InterfacesAdded
> signal for all characteristics and descriptors have been sent to
> clients". In short, even if we wait until discovery is complete, we
> will create service, characteristic, and descriptor objects in some
> order but I want to have a way for a client to know that all objects
> associated with a service are available via D-Bus. I'm open to
> suggestions :)

So this is problem that InterfacesAdded is only reporting for a single object path. Maybe this will actually work out as you described it.

>
>>>
>>> I have objection to adding these two method. However the error codes should be a bit more descriptive on the level AuthenticationRequired or MissingAuthentication. Then again, I think this detail might be better hidden in the daemon.
>>
>> this should read "no objection". Seems I am swallowing words again ;)
>>
>> For example if you are not encrypted and the characteristic requires an encrypted link, we should upgrade the security level and try again. If no LTK is available, then org.bluez.Error.NoEncryptionKey or similar should be returned. I think it is important that we handle all these details in the daemon to make it transparent for the end user.
>>
>
> I think these should be handled by the daemon too. Then again we
> should report errors to the clients when we can. So yes, I agree.

Okay, then make the error really simple. Maybe even on a high-level like "not paired". Since the only action from the user here is really to start pairing with the device.

>
>
>>> +
>>> + void StartNotify()
>>> +
>>> + Starts a notification session from this characteristic
>>> + if it supports value notifications or indications.
>>> +
>>> + Possible Errors: org.bluez.Error.Failed
>>> + org.bluez.Error.InProgress
>>> + org.bluez.Error.NotSupported
>>> +
>>> + void StopNotify()
>>> +
>>> + This method will cancel any previous StartNotify
>>> + transaction. Note that notifications from a
>>> + characteristic are shared between sessions thus
>>> + calling StopNotify will release a single session.
>>> +
>>> + Possible Errors: org.bluez.Error.Failed
>>
>> Seems sensible as well to me. However I am little bit debating the naming of the method a bit. I would have to read the spec in detail again which is the best name. EnableNotification comes to mind as well.
>>
>
> Yeah, I came up with these names pretty quickly while I was
> time-pressed for the Chrome 37 release. I don't have a strong opinion
> here; maybe EnableNotifications & DisableNotifications? Or maybe
> something that contains the word "session" since these methods behave
> like StartDiscovery and StopDiscovery on org.bluez.Adapter1.

I leave it to others to comment here as well. It is an easy change in the end.

>
>
>>> +
>>> +Signals void ValueUpdated(array{bytes} value)
>>> +
>>> + This signal is launched when a characteristic handle
>>> + value notification or indication is received.
>>
>> Why is just sending a PropertyChanged not good enough here? I would consider the property itself the cached value. Extra signals are costing extra overhead.
>>
>
> I added this signal because I got rid of the property. Even if we keep
> the property, I'm still not convinced that using the PropertyChanged
> signal to mean both "cached value updated after read" and
> "notification/indication received" is the right idea. I sort of prefer
> having a signal that only gets called when there's a notification,
> though if we keep this property then I guess we're going to have to
> work with that.

My thinking is that it really does not matter how the value got updated. Either by a manual read or by a notification or even by a write. The importance is to get the new value notified to all other applications that are not involved. Doing it only one way seems to make sense. And the applications all already have to listen to the PropertiesChanged signal anyway. Less signals to listen on the the better.

>
>
>>> + boolean Notifying [read-only]
>>>
>>> - Value read from the remote Bluetooth device or from
>>> - the external application implementing GATT services.
>>> + True, if notifications or indications on this
>>> + characteristic are currently enabled.
>>
>> As said above, I would leave the Value here as well. Maybe make it optional in case it has not yet been read or does not allow reading at all. It would represent the cached value.
>>
>
> I'm fine with leaving it. I'm ok with changing it to a read-only &
> optional property that represents the cached value.
>
>> I am not fully at ease with the name Notifying yet. Need to think about it a bit or some good convincing.
>>
>
> "NotificationsEnabled" maybe? That would make sense if change the
> methods to EnableNotifications & DisableNotifications. I guess we all
> suck at coming up with names :P

We can keep the current names, but I might object later if I come up with something better.
>
>
>>> + array{object} Descriptors [read-only]
>>> +
>>> + Array of object paths representing the descriptors
>>> + of this service. This property is set only when the
>>> + descriptor discovery has been completed, however the
>>> + descriptor objects will become available via
>>> + ObjectManager as soon as they get discovered.
>>> +
>>
>> Why is this needed?
>>
>
> Same reason as the "Characteristics" property on GattService1 above.
> It's not essential but we should have it for consistency if we keep
> "Characteristics".

Okay. Fair enough.

>
>>> -
>>> - string Permissions [read-only]: To be defined
>>> -
>>> - Defines read/write authentication and authorization
>>> - requirements.
>>
>> Why is this removed now?
>
> To be honest, I don't think this property is that necessary. GATT/ATT
> don't really define a way to obtain these requirements in the
> characteristic declaration and leave them up to the upper layer
> profile and the only way to find out about these requirements is to
> issue a read/write request and see if there's an error. If we properly
> report errors to the client on calls to ReadValue and WriteValue, then
> this property wouldn't really be needed. Let me know if I'm missing
> something though.

We might actually update it from the property does not exist to we have figured this out since someone read the property and thus we are reporting it now. I might be wrong, but I think there was some part in GATT that provided these details.

Regards

Marcel


2014-07-21 21:11:28

by Arman Uguray

[permalink] [raw]
Subject: Re: [RFC 1/1] doc/gatt-api: New API properties and methods for the GATT D-Bus API.

Hi Marcel, Chaojie,

See my responses inline.

> We are working on GATT D-Bus API for Tizen LE GATT Client too. F=
or your proposal, I totally agree your part of proposal such as add Primary=
property into org.bluez.GattService1, because all the hierarchy services c=
annot differentiate between primary and included service. And device proper=
ty is also very useful because It can help user to trace the services belon=
g to which device.
> However , you add Write/Read/Notify method into GATT-API , I thin=
k it is good proposal but not essential. Because it is about implementation=
method problem, not the GATT DBus API problem.

I have to disagree here. I think making the DBus.Properties interface
work for this particular case doesn't make the API particularly
simpler, in fact I think it adds unnecessary complexity and ambiguity
to the semantics involved. It's not clear, for instance, if Get should
always return the cached value or if it should issue a Read request to
the remote end. You suggest making an initial read request and storing
that value and updating it on notifications. What if the
characteristic doesn't support notifications but it's value can
change? What if the client is implementing a profile that warrants a
read request to the remote end to get the freshest value on demand?

For example, take the "Device Name" characteristic of the Generic
Access service, which has to support reads, optionally supports
writes, and doesn't support notifications/indications. The client can
write its value but won't be notified of the change. Now, the daemon
could remember that the value was written and update the property
right upon success, but I think this is unnecessary magic.

Same with control point characteristics, that support writes but no
notifications or reads. The external application can call "Set" to
write to the characteristic but does it make sense to send a
PropertiesChanged signal for a value that we cannot read? Should the
daemon cache the write value and return that on Get? Or should it
return an empty value?

The same goes for the PropertyChanged signal, which we will end up
emitting after a successful read, notifications/indications, and
portentially writes. I think it's cleaner to have a distinct signal
that gets only sent on notifications.

We end up with a read-write property that doesn't return a consistent
value on Get and Set calls. We end up making the property interface
work for semantics that it wasn't exactly built for.

I want to have an API where the external application has a clear way
of saying "send a remote read/write request". I'm not opposed to
having a "Value" property but I'd much rather it represent a read-only
property that only returns the most recently cached value.

> >> + object Device [read-only, optional]
> >> +
> >> + Object path of the Bluetooth device the service
> >> + belongs to. Only present on services from remote
> >> + devices.
> >
> > I am not sure this is needed. What do you want to use it for?

This is very useful to determine which device a service belongs to
when an InterfacesAdded signal is sent for a service. We have an
"Adapter" property for org.bluez.Device1, so why not a "Device"
property for org.bluez.GattService1?

>
> >> +
> >> + array{object} Characteristics [read-only]
> >> +
> >> + Array of object paths representing the characteri=
stics
> >> + of this service. This property is set only when t=
he
> >> + characteristic discovery has been completed, howe=
ver the
> >> + characteristic objects will become available via
> >> + ObjectManager as soon as they get discovered.
> >> +
> >
> > I really wonder if this is a good idea. Why not delay announcing any se=
rvice or characteristic via D-Bus as long as the service discovery is still=
running. I think it makes more sense to allow doing it one way and one way=
only.
>

I guess I should have said "This property is set only when the
characteristic discovery has been completed AND an InterfacesAdded
signal for all characteristics and descriptors have been sent to
clients". In short, even if we wait until discovery is complete, we
will create service, characteristic, and descriptor objects in some
order but I want to have a way for a client to know that all objects
associated with a service are available via D-Bus. I'm open to
suggestions :)

> >
> > I have objection to adding these two method. However the error codes sh=
ould be a bit more descriptive on the level AuthenticationRequired or Missi=
ngAuthentication. Then again, I think this detail might be better hidden in=
the daemon.
>
> this should read "no objection". Seems I am swallowing words again ;)
>
> For example if you are not encrypted and the characteristic requires an e=
ncrypted link, we should upgrade the security level and try again. If no LT=
K is available, then org.bluez.Error.NoEncryptionKey or similar should be r=
eturned. I think it is important that we handle all these details in the da=
emon to make it transparent for the end user.
>

I think these should be handled by the daemon too. Then again we
should report errors to the clients when we can. So yes, I agree.


>> +
>> + void StartNotify()
>> +
>> + Starts a notification session from this characteri=
stic
>> + if it supports value notifications or indications.
>> +
>> + Possible Errors: org.bluez.Error.Failed
>> + org.bluez.Error.InProgress
>> + org.bluez.Error.NotSupported
>> +
>> + void StopNotify()
>> +
>> + This method will cancel any previous StartNotify
>> + transaction. Note that notifications from a
>> + characteristic are shared between sessions thus
>> + calling StopNotify will release a single session.
>> +
>> + Possible Errors: org.bluez.Error.Failed
>
> Seems sensible as well to me. However I am little bit debating the naming=
of the method a bit. I would have to read the spec in detail again which i=
s the best name. EnableNotification comes to mind as well.
>

Yeah, I came up with these names pretty quickly while I was
time-pressed for the Chrome 37 release. I don't have a strong opinion
here; maybe EnableNotifications & DisableNotifications? Or maybe
something that contains the word "session" since these methods behave
like StartDiscovery and StopDiscovery on org.bluez.Adapter1.


>> +
>> +Signals void ValueUpdated(array{bytes} value)
>> +
>> + This signal is launched when a characteristic hand=
le
>> + value notification or indication is received.
>
> Why is just sending a PropertyChanged not good enough here? I would consi=
der the property itself the cached value. Extra signals are costing extra o=
verhead.
>

I added this signal because I got rid of the property. Even if we keep
the property, I'm still not convinced that using the PropertyChanged
signal to mean both "cached value updated after read" and
"notification/indication received" is the right idea. I sort of prefer
having a signal that only gets called when there's a notification,
though if we keep this property then I guess we're going to have to
work with that.


>> + boolean Notifying [read-only]
>>
>> - Value read from the remote Bluetooth device or fro=
m
>> - the external application implementing GATT service=
s.
>> + True, if notifications or indications on this
>> + characteristic are currently enabled.
>
> As said above, I would leave the Value here as well. Maybe make it option=
al in case it has not yet been read or does not allow reading at all. It wo=
uld represent the cached value.
>

I'm fine with leaving it. I'm ok with changing it to a read-only &
optional property that represents the cached value.

> I am not fully at ease with the name Notifying yet. Need to think about i=
t a bit or some good convincing.
>

"NotificationsEnabled" maybe? That would make sense if change the
methods to EnableNotifications & DisableNotifications. I guess we all
suck at coming up with names :P


>> + array{object} Descriptors [read-only]
>> +
>> + Array of object paths representing the descriptors
>> + of this service. This property is set only when th=
e
>> + descriptor discovery has been completed, however t=
he
>> + descriptor objects will become available via
>> + ObjectManager as soon as they get discovered.
>> +
>
> Why is this needed?
>

Same reason as the "Characteristics" property on GattService1 above.
It's not essential but we should have it for consistency if we keep
"Characteristics".

>> -
>> - string Permissions [read-only]: To be defined
>> -
>> - Defines read/write authentication and authorizatio=
n
>> - requirements.
>
> Why is this removed now?

To be honest, I don't think this property is that necessary. GATT/ATT
don't really define a way to obtain these requirements in the
characteristic declaration and leave them up to the upper layer
profile and the only way to find out about these requirements is to
issue a read/write request and see if there's an error. If we properly
report errors to the client on calls to ReadValue and WriteValue, then
this property wouldn't really be needed. Let me know if I'm missing
something though.

Cheers all,
Arman

2014-07-21 14:13:30

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [RFC 1/1] doc/gatt-api: New API properties and methods for the GATT D-Bus API.

Hi Arman,

>> This patch proposes changes to the currently unimplemented GATT D-Bus API for
>> desktop bluetoothd. This is the first step in implementing a GATT client layer
>> for bluetoothd that will change the way remote attributes are accessed via
>> bluetoothd plugins and external applications.
>> ---
>> doc/gatt-api.txt | 118 ++++++++++++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 104 insertions(+), 14 deletions(-)
>>
>> diff --git a/doc/gatt-api.txt b/doc/gatt-api.txt
>> index 8c7975c..bec9674 100644
>> --- a/doc/gatt-api.txt
>> +++ b/doc/gatt-api.txt
>> @@ -32,6 +32,25 @@ Properties string UUID [read-only]
>>
>> 128-bit service UUID.
>>
>> + boolean Primary [read-only]
>> +
>> + Indicates whether or not this GATT service is a
>> + primary service. If false, the service is secondary.
>
> this makes total sense. We most likely never worried about it since all devices these days have everything set as primary service.
>
>> +
>> + object Device [read-only, optional]
>> +
>> + Object path of the Bluetooth device the service
>> + belongs to. Only present on services from remote
>> + devices.
>
> I am not sure this is needed. What do you want to use it for?
>
>> +
>> + array{object} Characteristics [read-only]
>> +
>> + Array of object paths representing the characteristics
>> + of this service. This property is set only when the
>> + characteristic discovery has been completed, however the
>> + characteristic objects will become available via
>> + ObjectManager as soon as they get discovered.
>> +
>
> I really wonder if this is a good idea. Why not delay announcing any service or characteristic via D-Bus as long as the service discovery is still running. I think it makes more sense to allow doing it one way and one way only.
>
>> array{object} Includes [read-only]: Not implemented
>>
>> Array of object paths representing the included
>> @@ -48,6 +67,54 @@ Service org.bluez
>> Interface org.bluez.GattCharacteristic1 [Experimental]
>> Object path [variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX/serviceXX/charYYYY
>>
>> +Methods array{byte} ReadValue()
>> +
>> + Issues a request to read the value of the
>> + characteristic and returns the value if the
>> + operation was successful.
>> +
>> + Possible Errors: org.bluez.Error.Failed
>> + org.bluez.Error.InProgress
>> + org.bluez.Error.ReadNotPermitted
>> + org.bluez.Error.Authentication
>> + org.bluez.Error.Authorization
>> + org.bluez.Error.Encryption
>> +
>> + void WriteValue(array{byte} value)
>> +
>> + Issues a request to write the value of the
>> + characteristic.
>> +
>> + Possible Errors: org.bluez.Error.Failed
>> + org.bluez.Error.InProgress
>> + org.bluez.Error.WriteNotPermitted
>> + org.bluez.Error.Authentication
>> + org.bluez.Error.Authorization
>> + org.bluez.Error.Encryption
>
> I have objection to adding these two method. However the error codes should be a bit more descriptive on the level AuthenticationRequired or MissingAuthentication. Then again, I think this detail might be better hidden in the daemon.

this should read "no objection". Seems I am swallowing words again ;)

Regards

Marcel


2014-07-21 13:46:42

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [RFC 1/1] doc/gatt-api: New API properties and methods for the GATT D-Bus API.

Hi Arman,

> This patch proposes changes to the currently unimplemented GATT D-Bus API for
> desktop bluetoothd. This is the first step in implementing a GATT client layer
> for bluetoothd that will change the way remote attributes are accessed via
> bluetoothd plugins and external applications.
> ---
> doc/gatt-api.txt | 118 ++++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 104 insertions(+), 14 deletions(-)
>
> diff --git a/doc/gatt-api.txt b/doc/gatt-api.txt
> index 8c7975c..bec9674 100644
> --- a/doc/gatt-api.txt
> +++ b/doc/gatt-api.txt
> @@ -32,6 +32,25 @@ Properties string UUID [read-only]
>
> 128-bit service UUID.
>
> + boolean Primary [read-only]
> +
> + Indicates whether or not this GATT service is a
> + primary service. If false, the service is secondary.

this makes total sense. We most likely never worried about it since all devices these days have everything set as primary service.

> +
> + object Device [read-only, optional]
> +
> + Object path of the Bluetooth device the service
> + belongs to. Only present on services from remote
> + devices.

I am not sure this is needed. What do you want to use it for?

> +
> + array{object} Characteristics [read-only]
> +
> + Array of object paths representing the characteristics
> + of this service. This property is set only when the
> + characteristic discovery has been completed, however the
> + characteristic objects will become available via
> + ObjectManager as soon as they get discovered.
> +

I really wonder if this is a good idea. Why not delay announcing any service or characteristic via D-Bus as long as the service discovery is still running. I think it makes more sense to allow doing it one way and one way only.

> array{object} Includes [read-only]: Not implemented
>
> Array of object paths representing the included
> @@ -48,6 +67,54 @@ Service org.bluez
> Interface org.bluez.GattCharacteristic1 [Experimental]
> Object path [variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX/serviceXX/charYYYY
>
> +Methods array{byte} ReadValue()
> +
> + Issues a request to read the value of the
> + characteristic and returns the value if the
> + operation was successful.
> +
> + Possible Errors: org.bluez.Error.Failed
> + org.bluez.Error.InProgress
> + org.bluez.Error.ReadNotPermitted
> + org.bluez.Error.Authentication
> + org.bluez.Error.Authorization
> + org.bluez.Error.Encryption
> +
> + void WriteValue(array{byte} value)
> +
> + Issues a request to write the value of the
> + characteristic.
> +
> + Possible Errors: org.bluez.Error.Failed
> + org.bluez.Error.InProgress
> + org.bluez.Error.WriteNotPermitted
> + org.bluez.Error.Authentication
> + org.bluez.Error.Authorization
> + org.bluez.Error.Encryption

I have objection to adding these two method. However the error codes should be a bit more descriptive on the level AuthenticationRequired or MissingAuthentication. Then again, I think this detail might be better hidden in the daemon.

For example if you are not encrypted and the characteristic requires an encrypted link, we should upgrade the security level and try again. If no LTK is available, then org.bluez.Error.NoEncryptionKey or similar should be returned. I think it is important that we handle all these details in the daemon to make it transparent for the end user.

> +
> + void StartNotify()
> +
> + Starts a notification session from this characteristic
> + if it supports value notifications or indications.
> +
> + Possible Errors: org.bluez.Error.Failed
> + org.bluez.Error.InProgress
> + org.bluez.Error.NotSupported
> +
> + void StopNotify()
> +
> + This method will cancel any previous StartNotify
> + transaction. Note that notifications from a
> + characteristic are shared between sessions thus
> + calling StopNotify will release a single session.
> +
> + Possible Errors: org.bluez.Error.Failed

Seems sensible as well to me. However I am little bit debating the naming of the method a bit. I would have to read the spec in detail again which is the best name. EnableNotification comes to mind as well.

> +
> +Signals void ValueUpdated(array{bytes} value)
> +
> + This signal is launched when a characteristic handle
> + value notification or indication is received.

Why is just sending a PropertyChanged not good enough here? I would consider the property itself the cached value. Extra signals are costing extra overhead.

> +
> Properties string UUID [read-only]
>
> 128-bit characteristic UUID.
> @@ -57,12 +124,12 @@ Properties string UUID [read-only]
> Object path of the GATT service the characteristc
> belongs to.
>
> - array{byte} Value [read-write]
> + boolean Notifying [read-only]
>
> - Value read from the remote Bluetooth device or from
> - the external application implementing GATT services.
> + True, if notifications or indications on this
> + characteristic are currently enabled.

As said above, I would leave the Value here as well. Maybe make it optional in case it has not yet been read or does not allow reading at all. It would represent the cached value.

I am not fully at ease with the name Notifying yet. Need to think about it a bit or some good convincing.

>
> - array{string} Flags [read-only, optional]
> + array{string} Flags [read-only]
>
> Defines how the characteristic value can be used. See
> Core spec "Table 3.5: Characteristic Properties bit
> @@ -79,6 +146,14 @@ Properties string UUID [read-only]
> "reliable-write"
> "writable-auxiliaries"
>
> + array{object} Descriptors [read-only]
> +
> + Array of object paths representing the descriptors
> + of this service. This property is set only when the
> + descriptor discovery has been completed, however the
> + descriptor objects will become available via
> + ObjectManager as soon as they get discovered.
> +

Why is this needed?

>
> Characteristic Descriptors hierarchy
> ====================================
> @@ -89,6 +164,31 @@ Service org.bluez
> Interface org.bluez.GattDescriptor1 [Experimental]
> Object path [variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX/serviceXX/charYYYY/descriptorZZZ
>
> +Methods array{byte} ReadValue()
> +
> + Issues a request to read the value of the
> + characteristic and returns the value if the
> + operation was successful.
> +
> + Possible Errors: org.bluez.Error.Failed
> + org.bluez.Error.InProgress
> + org.bluez.Error.ReadNotPermitted
> + org.bluez.Error.Authentication
> + org.bluez.Error.Authorization
> + org.bluez.Error.Encryption
> +
> + void WriteValue(array{byte} value)
> +
> + Issues a request to write the value of the
> + characteristic.
> +
> + Possible Errors: org.bluez.Error.Failed
> + org.bluez.Error.InProgress
> + org.bluez.Error.WriteNotPermitted
> + org.bluez.Error.Authentication
> + org.bluez.Error.Authorization
> + org.bluez.Error.Encryption
> +
> Properties string UUID [read-only]
>
> 128-bit descriptor UUID.
> @@ -98,16 +198,6 @@ Properties string UUID [read-only]
> Object path of the GATT characteristc the descriptor
> belongs to.
>
> - array{byte} Value [read-write]
> -
> - Raw characteristic descriptor value read from the
> - remote Bluetooth device or from the external
> - application implementing GATT services.

Same comments as above. I think the Value should stay and maybe made optional.

> -
> - string Permissions [read-only]: To be defined
> -
> - Defines read/write authentication and authorization
> - requirements.

Why is this removed now?

Regards

Marcel


2014-07-21 01:50:32

by Gu, Chao Jie

[permalink] [raw]
Subject: RE: [RFC 1/1] doc/gatt-api: New API properties and methods for the GATT D-Bus API.

Hi, arman
We are working on GATT D-Bus API for Tizen LE GATT Client too. For your proposal, I totally agree your part of proposal such as add Primary property into org.bluez.GattService1, because all the hierarchy services cannot differentiate between primary and included service. And device property is also very useful because It can help user to trace the services belong to which device.
However , you add Write/Read/Notify method into GATT-API , I think it is good proposal but not essential. Because it is about implementation method problem, not the GATT DBus API problem.
For example, The Get and GetAll methods are synchronous, So you can read value from remote device asynchronously when you setup the DBus hierarchy.
And you add notify method , I think it should put into LE profile to do this such as heartrate profile. Gatt DBus Hierarchy just listen this event and emit propertychanged signal when remote characteristic Value updated.

Thanks
Chaojie

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Arman Uguray
Sent: Saturday, July 19, 2014 2:07 PM
To: [email protected]
Cc: Arman Uguray
Subject: [RFC 1/1] doc/gatt-api: New API properties and methods for the GATT D-Bus API.

This patch proposes changes to the currently unimplemented GATT D-Bus API for desktop bluetoothd. This is the first step in implementing a GATT client layer for bluetoothd that will change the way remote attributes are accessed via bluetoothd plugins and external applications.
---
doc/gatt-api.txt | 118 ++++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 104 insertions(+), 14 deletions(-)

diff --git a/doc/gatt-api.txt b/doc/gatt-api.txt index 8c7975c..bec9674 100644
--- a/doc/gatt-api.txt
+++ b/doc/gatt-api.txt
@@ -32,6 +32,25 @@ Properties string UUID [read-only]

128-bit service UUID.

+ boolean Primary [read-only]
+
+ Indicates whether or not this GATT service is a
+ primary service. If false, the service is secondary.
+
+ object Device [read-only, optional]
+
+ Object path of the Bluetooth device the service
+ belongs to. Only present on services from remote
+ devices.
+
+ array{object} Characteristics [read-only]
+
+ Array of object paths representing the characteristics
+ of this service. This property is set only when the
+ characteristic discovery has been completed, however the
+ characteristic objects will become available via
+ ObjectManager as soon as they get discovered.
+
array{object} Includes [read-only]: Not implemented

Array of object paths representing the included
@@ -48,6 +67,54 @@ Service org.bluez
Interface org.bluez.GattCharacteristic1 [Experimental]
Object path [variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX/serviceXX/charYYYY

+Methods array{byte} ReadValue()
+
+ Issues a request to read the value of the
+ characteristic and returns the value if the
+ operation was successful.
+
+ Possible Errors: org.bluez.Error.Failed
+ org.bluez.Error.InProgress
+ org.bluez.Error.ReadNotPermitted
+ org.bluez.Error.Authentication
+ org.bluez.Error.Authorization
+ org.bluez.Error.Encryption
+
+ void WriteValue(array{byte} value)
+
+ Issues a request to write the value of the
+ characteristic.
+
+ Possible Errors: org.bluez.Error.Failed
+ org.bluez.Error.InProgress
+ org.bluez.Error.WriteNotPermitted
+ org.bluez.Error.Authentication
+ org.bluez.Error.Authorization
+ org.bluez.Error.Encryption
+
+ void StartNotify()
+
+ Starts a notification session from this characteristic
+ if it supports value notifications or indications.
+
+ Possible Errors: org.bluez.Error.Failed
+ org.bluez.Error.InProgress
+ org.bluez.Error.NotSupported
+
+ void StopNotify()
+
+ This method will cancel any previous StartNotify
+ transaction. Note that notifications from a
+ characteristic are shared between sessions thus
+ calling StopNotify will release a single session.
+
+ Possible Errors: org.bluez.Error.Failed
+
+Signals void ValueUpdated(array{bytes} value)
+
+ This signal is launched when a characteristic handle
+ value notification or indication is received.
+
Properties string UUID [read-only]

128-bit characteristic UUID.
@@ -57,12 +124,12 @@ Properties string UUID [read-only]
Object path of the GATT service the characteristc
belongs to.

- array{byte} Value [read-write]
+ boolean Notifying [read-only]

- Value read from the remote Bluetooth device or from
- the external application implementing GATT services.
+ True, if notifications or indications on this
+ characteristic are currently enabled.

- array{string} Flags [read-only, optional]
+ array{string} Flags [read-only]

Defines how the characteristic value can be used. See
Core spec "Table 3.5: Characteristic Properties bit
@@ -79,6 +146,14 @@ Properties string UUID [read-only]
"reliable-write"
"writable-auxiliaries"

+ array{object} Descriptors [read-only]
+
+ Array of object paths representing the descriptors
+ of this service. This property is set only when the
+ descriptor discovery has been completed, however the
+ descriptor objects will become available via
+ ObjectManager as soon as they get discovered.
+

Characteristic Descriptors hierarchy
====================================
@@ -89,6 +164,31 @@ Service org.bluez
Interface org.bluez.GattDescriptor1 [Experimental]
Object path [variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX/serviceXX/charYYYY/descriptorZZZ

+Methods array{byte} ReadValue()
+
+ Issues a request to read the value of the
+ characteristic and returns the value if the
+ operation was successful.
+
+ Possible Errors: org.bluez.Error.Failed
+ org.bluez.Error.InProgress
+ org.bluez.Error.ReadNotPermitted
+ org.bluez.Error.Authentication
+ org.bluez.Error.Authorization
+ org.bluez.Error.Encryption
+
+ void WriteValue(array{byte} value)
+
+ Issues a request to write the value of the
+ characteristic.
+
+ Possible Errors: org.bluez.Error.Failed
+ org.bluez.Error.InProgress
+ org.bluez.Error.WriteNotPermitted
+ org.bluez.Error.Authentication
+ org.bluez.Error.Authorization
+ org.bluez.Error.Encryption
+
Properties string UUID [read-only]

128-bit descriptor UUID.
@@ -98,16 +198,6 @@ Properties string UUID [read-only]
Object path of the GATT characteristc the descriptor
belongs to.

- array{byte} Value [read-write]
-
- Raw characteristic descriptor value read from the
- remote Bluetooth device or from the external
- application implementing GATT services.
-
- string Permissions [read-only]: To be defined
-
- Defines read/write authentication and authorization
- requirements.

Service Manager hierarchy
=============================
--
2.0.0.526.g5318336

2014-07-19 06:06:48

by Arman Uguray

[permalink] [raw]
Subject: [RFC 1/1] doc/gatt-api: New API properties and methods for the GATT D-Bus API.

This patch proposes changes to the currently unimplemented GATT D-Bus API for
desktop bluetoothd. This is the first step in implementing a GATT client layer
for bluetoothd that will change the way remote attributes are accessed via
bluetoothd plugins and external applications.
---
doc/gatt-api.txt | 118 ++++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 104 insertions(+), 14 deletions(-)

diff --git a/doc/gatt-api.txt b/doc/gatt-api.txt
index 8c7975c..bec9674 100644
--- a/doc/gatt-api.txt
+++ b/doc/gatt-api.txt
@@ -32,6 +32,25 @@ Properties string UUID [read-only]

128-bit service UUID.

+ boolean Primary [read-only]
+
+ Indicates whether or not this GATT service is a
+ primary service. If false, the service is secondary.
+
+ object Device [read-only, optional]
+
+ Object path of the Bluetooth device the service
+ belongs to. Only present on services from remote
+ devices.
+
+ array{object} Characteristics [read-only]
+
+ Array of object paths representing the characteristics
+ of this service. This property is set only when the
+ characteristic discovery has been completed, however the
+ characteristic objects will become available via
+ ObjectManager as soon as they get discovered.
+
array{object} Includes [read-only]: Not implemented

Array of object paths representing the included
@@ -48,6 +67,54 @@ Service org.bluez
Interface org.bluez.GattCharacteristic1 [Experimental]
Object path [variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX/serviceXX/charYYYY

+Methods array{byte} ReadValue()
+
+ Issues a request to read the value of the
+ characteristic and returns the value if the
+ operation was successful.
+
+ Possible Errors: org.bluez.Error.Failed
+ org.bluez.Error.InProgress
+ org.bluez.Error.ReadNotPermitted
+ org.bluez.Error.Authentication
+ org.bluez.Error.Authorization
+ org.bluez.Error.Encryption
+
+ void WriteValue(array{byte} value)
+
+ Issues a request to write the value of the
+ characteristic.
+
+ Possible Errors: org.bluez.Error.Failed
+ org.bluez.Error.InProgress
+ org.bluez.Error.WriteNotPermitted
+ org.bluez.Error.Authentication
+ org.bluez.Error.Authorization
+ org.bluez.Error.Encryption
+
+ void StartNotify()
+
+ Starts a notification session from this characteristic
+ if it supports value notifications or indications.
+
+ Possible Errors: org.bluez.Error.Failed
+ org.bluez.Error.InProgress
+ org.bluez.Error.NotSupported
+
+ void StopNotify()
+
+ This method will cancel any previous StartNotify
+ transaction. Note that notifications from a
+ characteristic are shared between sessions thus
+ calling StopNotify will release a single session.
+
+ Possible Errors: org.bluez.Error.Failed
+
+Signals void ValueUpdated(array{bytes} value)
+
+ This signal is launched when a characteristic handle
+ value notification or indication is received.
+
Properties string UUID [read-only]

128-bit characteristic UUID.
@@ -57,12 +124,12 @@ Properties string UUID [read-only]
Object path of the GATT service the characteristc
belongs to.

- array{byte} Value [read-write]
+ boolean Notifying [read-only]

- Value read from the remote Bluetooth device or from
- the external application implementing GATT services.
+ True, if notifications or indications on this
+ characteristic are currently enabled.

- array{string} Flags [read-only, optional]
+ array{string} Flags [read-only]

Defines how the characteristic value can be used. See
Core spec "Table 3.5: Characteristic Properties bit
@@ -79,6 +146,14 @@ Properties string UUID [read-only]
"reliable-write"
"writable-auxiliaries"

+ array{object} Descriptors [read-only]
+
+ Array of object paths representing the descriptors
+ of this service. This property is set only when the
+ descriptor discovery has been completed, however the
+ descriptor objects will become available via
+ ObjectManager as soon as they get discovered.
+

Characteristic Descriptors hierarchy
====================================
@@ -89,6 +164,31 @@ Service org.bluez
Interface org.bluez.GattDescriptor1 [Experimental]
Object path [variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX/serviceXX/charYYYY/descriptorZZZ

+Methods array{byte} ReadValue()
+
+ Issues a request to read the value of the
+ characteristic and returns the value if the
+ operation was successful.
+
+ Possible Errors: org.bluez.Error.Failed
+ org.bluez.Error.InProgress
+ org.bluez.Error.ReadNotPermitted
+ org.bluez.Error.Authentication
+ org.bluez.Error.Authorization
+ org.bluez.Error.Encryption
+
+ void WriteValue(array{byte} value)
+
+ Issues a request to write the value of the
+ characteristic.
+
+ Possible Errors: org.bluez.Error.Failed
+ org.bluez.Error.InProgress
+ org.bluez.Error.WriteNotPermitted
+ org.bluez.Error.Authentication
+ org.bluez.Error.Authorization
+ org.bluez.Error.Encryption
+
Properties string UUID [read-only]

128-bit descriptor UUID.
@@ -98,16 +198,6 @@ Properties string UUID [read-only]
Object path of the GATT characteristc the descriptor
belongs to.

- array{byte} Value [read-write]
-
- Raw characteristic descriptor value read from the
- remote Bluetooth device or from the external
- application implementing GATT services.
-
- string Permissions [read-only]: To be defined
-
- Defines read/write authentication and authorization
- requirements.

Service Manager hierarchy
=============================
--
2.0.0.526.g5318336