2011-06-01 15:22:44

by Anderson Lizardo

[permalink] [raw]
Subject: [RFC] New API for attribute read/write callback registration

Hi,

The current BlueZ mainline contains an initial API for attribute
callbacks which are called before reading and after writing to a ATT
attribute. This allows for filling attribute values which are not
static (or which come from an external source, e.g. battery level
measurements).

The current API works as this:

struct attribute *a;
a = attrib_db_add(<handle>, <uuid>, <read_permissions>,
<write_permissions>, <value>, <value_len>);
a->read_cb = <read_callback>;
a->write_cb = <write_callback>;
a->cb_user_data = <pointer_to_user_data>;

callbacks have signature:

uint8_t (*read_cb)(struct attribute *a, gpointer user_data);
uint8_t (*write_cb)(struct attribute *a, gpointer user_data);

This works well if attrib_db_add() is called from a point where we can
access the connection context data (i.e. GAttrib). This is most always
not the case, as attributes are registered during profile plugin load.

To allow to pass GAttrib to the read/write callbacks, we need to
decouple the callback information from struct attribute. This also
allows to easily extend with more operations other then read/write
(e.g. notifications). Therefore we plan to have this new API (already
implemented, and which we should send patches anytime soon):

struct attrib_cb *cb;
struct attribute *a;

a = attrib_db_add(<handle>, <uuid>, <read_permissions>,
<write_permissions>, <value>, <value_len>);
cb = g_new0(struct attrib_cb, 1);
/* event: ATTRIB_READ, ATTRIB_WRITE or ATTRIB_UPDATE */
cb->event = <event>;
/* cb->read, cb->write or cp->update, depending on the event type */
cb->write = <write_callback>;
attrib_add_callback(a, cb)

And the new signatures for callbacks:

uint8_t (*read)(GAttrib *attrib, struct attribute *a);
uint8_t (*write)(GAttrib *attrib, struct attribute *a);
uint8_t (*update)(GAttrib *attrib, struct attribute *a);

We use a union for the callback field, allowing different signatures
for different events.

Currently, the main issue which prevents us from sending the migration
to this new API is that attrib_db_update() uses g_try_realloc(), which
may change the allocated pointer and invalidate references to the old
struct attribute pointer.

Suggestions/comments are welcome. Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil


2011-06-02 12:52:26

by Anderson Lizardo

[permalink] [raw]
Subject: Re: [RFC] New API for attribute read/write callback registration

Hi Chen,

On Thu, Jun 2, 2011 at 2:31 AM, Ganir, Chen <[email protected]> wrote:
>> And the new signatures for callbacks:
>>
>> ? ? ? ? ? ? ?uint8_t (*read)(GAttrib *attrib, struct attribute *a);
>> ? ? ? ? ? ? ?uint8_t (*write)(GAttrib *attrib, struct attribute *a);
>> ? ? ? ? ? ? ?uint8_t (*update)(GAttrib *attrib, struct attribute
>> *a);
>>
> Can this be united into the same API ? Why do we need the cb->event
> if we have cb->write or cb->read ? For example :

Note that they are part of a C union, so a single callback can be
used. Think of it as each struct cb corresponding to a single "event"
(read, write or update), not multiple callbacks on the same struct.
This way you can attach multiple struct cb's to the same struct
attribute.

>
> cb = g_new0(struct attrib_cb, 1);
> memset(cb,0,sizeof(cb);
> cb->write = <write_callback>;
> cb->read = <read_callback>;
> a = attrib_db_add(<handle>, <uuid>, <read_permissions>,
> ?<write_permissions>, <value>, <value_len>,cb);
>
> In addition - what is the ATTRIB_UPDATE event ? When is it called ?

There is a attrib_db_update(). It is used by profiles which need to
update the value stored on the struct attribute. It also triggers
indications/notifications as necessary.

> Callbacks for read/write are meaningful only for characteristics values,
> and characteristics descriptors. What happens when someone provides a
> callback for the service UUID itself ? For example, from the
> plugins/gatt-example ?:
>
> ? ?/* Battery state service: primary service definition */
> ? ?bt_uuid16_create(&uuid, GATT_PRIM_SVC_UUID);
> ? ?att_put_u16(BATTERY_STATE_SVC_UUID, &atval[0]);
> ? ?attrib_db_add(h++, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 2);
>
> Is there any point in adding a callback here?

Callbacks are to be registered when the profile sees necessary. If for
some reason it wants to be notified when the primary service
definition is read , it can do so. We don't want to make the API too
rigid to allowing callbacks only on certain GATT structures. This is
an attribute level callback, not GATT.

> Maybe a more GATT like approach should be used here - encapsulate the
> attrib_db_add functions with GATT defined service, descriptor,
> characteristic. All GATT characteristics, services and descriptors are
> eventually ATT attributes. So For example :
> [snip]

This was an idea I proposed long time ago (December last year), but we
didn't have time to implement/design it. Feel free to propose APIs and
patches :). Sending it to the list is the best way to get feedback.

> In this case, we do not allow setting a callback for the service handle.
> GATT does not define procedures for a client to read the service itself,
> just discover it, and then list it. I'm not sure if GATT permissions should
> apply here, for limiting find_by_type of find_by_uuid with authorization
> or authentication.

As I said before, I personally would prefer to not have the callback
mechanism to rigid. Leaving it on the attribute level allows many
usages, such as custom permission mechanisms (which are allowed but
the GATT denition when they leave it "implementation or profile
specific").

Besides, which benefits do you see on allowing callbacks only per
characteristic values and descriptors?

> We also hide the handles from the GATT user, since GATT server user does
> not really need to care about the some of the handles. We just need to
> make sure here that when a service is added, no other service is allowed
> to be added before all of the first service characteristics and descriptors
> were added.

Yes, a GATT level API would be much welcome. It would need to support
service includes as well, which needs start/end handles of the
included service.

> more examples :
> handle gatt_add_characteristic(<uuid>,<svc_size>,<read_perm>,
> <write_perm>,<value>,<len>,<write_cb>,<read_cb>);
>
> handle gatt_add_characteristic_client_config(<read_perm>,
> <write_perm>,<value>,<len>,<write_cb>,<read_cb>);
>
> The handle returned by some of the functions may be used for adding
> included services, or identifying the attribute when a single callback is
> used for all characteristics.

Note that on your proposal above you restrict callbacks to only
read/write. If we want to add callbacks/hooks for other "events"
later, we would need to change every caller of this function. This is
the main point why we are creating a pluggable "struct callback" which
can be easily extended for future use without needing to change every
current user.

Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

2011-06-02 06:31:58

by Ganir, Chen

[permalink] [raw]
Subject: RE: [RFC] New API for attribute read/write callback registration

Hi Anderson,


> To allow to pass GAttrib to the read/write callbacks, we need to
> decouple the callback information from struct attribute. This also
> allows to easily extend with more operations other then read/write
> (e.g. notifications). Therefore we plan to have this new API (already
> implemented, and which we should send patches anytime soon):
>
> struct attrib_cb *cb;
> struct attribute *a;
>
> a = attrib_db_add(<handle>, <uuid>, <read_permissions>,
> <write_permissions>, <value>, <value_len>);
> cb = g_new0(struct attrib_cb, 1);
> /* event: ATTRIB_READ, ATTRIB_WRITE or ATTRIB_UPDATE */
> cb->event = <event>;
> /* cb->read, cb->write or cp->update, depending on the event type */
> cb->write = <write_callback>;
> attrib_add_callback(a, cb)
>
> And the new signatures for callbacks:
>
> uint8_t (*read)(GAttrib *attrib, struct attribute *a);
> uint8_t (*write)(GAttrib *attrib, struct attribute *a);
> uint8_t (*update)(GAttrib *attrib, struct attribute
> *a);
>
Can this be united into the same API ? Why do we need the cb->event
if we have cb->write or cb->read ? For example :

cb = g_new0(struct attrib_cb, 1);
memset(cb,0,sizeof(cb);
cb->write = <write_callback>;
cb->read = <read_callback>;
a = attrib_db_add(<handle>, <uuid>, <read_permissions>,
<write_permissions>, <value>, <value_len>,cb);

In addition - what is the ATTRIB_UPDATE event ? When is it called ?

Callbacks for read/write are meaningful only for characteristics values,
and characteristics descriptors. What happens when someone provides a
callback for the service UUID itself ? For example, from the
plugins/gatt-example :

/* Battery state service: primary service definition */
bt_uuid16_create(&uuid, GATT_PRIM_SVC_UUID);
att_put_u16(BATTERY_STATE_SVC_UUID, &atval[0]);
attrib_db_add(h++, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 2);

Is there any point in adding a callback here?

Maybe a more GATT like approach should be used here - encapsulate the
attrib_db_add functions with GATT defined service, descriptor,
characteristic. All GATT characteristics, services and descriptors are
eventually ATT attributes. So For example :

service_handle gatt_add_prim_service(<uuid>,<svc_size>)
{
uint16_t h;
const int svc_size = 4;
uint32_t sdp_handle;
uint8_t atval[256];
bt_uuid_t uuid;

/* Do some calculations to find out the real service size
(considering the number of characteristics and descriptors
for example)
*/
h = attrib_db_find_avail(svc_size);

if (h == 0) {
error("Not enough free handles to register service");
return 0;
}

bt_uuid16_create(&uuid, GATT_PRIM_SVC_UUID);
att_put_u16(<uuid>, &atval[0]);
/* For GATT Service, we do not allow reading/writing or any
other Handle permissions. If we do want to allow GATT user
to set these values, we can add them to the function
prototype later.
*/
attrib_db_add(h, &uuid, ATT_NONE, ATT_NOT_PERMITTED, atval, 2);
return h;
}

gatt_add_primary_service(BATTERY_STATE_SVC_UUID, svc_size);

In this case, we do not allow setting a callback for the service handle.
GATT does not define procedures for a client to read the service itself,
just discover it, and then list it. I'm not sure if GATT permissions should
apply here, for limiting find_by_type of find_by_uuid with authorization
or authentication.

We also hide the handles from the GATT user, since GATT server user does
not really need to care about the some of the handles. We just need to
make sure here that when a service is added, no other service is allowed
to be added before all of the first service characteristics and descriptors
were added.

more examples :
handle gatt_add_characteristic(<uuid>,<svc_size>,<read_perm>,
<write_perm>,<value>,<len>,<write_cb>,<read_cb>);

handle gatt_add_characteristic_client_config(<read_perm>,
<write_perm>,<value>,<len>,<write_cb>,<read_cb>);

The handle returned by some of the functions may be used for adding
included services, or identifying the attribute when a single callback is
used for all characteristics.

Thanks,
Chen Ganir.


2011-06-01 18:11:31

by Jaikumar Ganesh

[permalink] [raw]
Subject: Re: [RFC] New API for attribute read/write callback registration

Hello Anderson,

On Wed, Jun 1, 2011 at 8:22 AM, Anderson Lizardo
<[email protected]> wrote:
> Hi,
>
> The current BlueZ mainline contains an initial API for attribute
> callbacks which are called before reading and after writing to a ATT
> attribute. This allows for filling attribute values which are not
> static (or which come from an external source, e.g. battery level
> measurements).
>
> The current API works as this:
>
> struct attribute *a;
> a = attrib_db_add(<handle>, <uuid>, <read_permissions>,
> <write_permissions>, <value>, <value_len>);
> a->read_cb = <read_callback>;
> a->write_cb = <write_callback>;
> a->cb_user_data = <pointer_to_user_data>;
>
> callbacks have signature:
>
> ? ? ? ?uint8_t (*read_cb)(struct attribute *a, gpointer user_data);
> ? ? ? ?uint8_t (*write_cb)(struct attribute *a, gpointer user_data);
>
> This works well if attrib_db_add() is called from a point where we can
> access the connection context data (i.e. GAttrib). This is most always
> not the case, as attributes are registered during profile plugin load.
>
> To allow to pass GAttrib to the read/write callbacks, we need to
> decouple the callback information from struct attribute. This also
> allows to easily extend with more operations other then read/write
> (e.g. notifications). Therefore we plan to have this new API (already
> implemented, and which we should send patches anytime soon):
>
> struct attrib_cb *cb;
> struct attribute *a;
>
> a = attrib_db_add(<handle>, <uuid>, <read_permissions>,
> <write_permissions>, <value>, <value_len>);
> cb = g_new0(struct attrib_cb, 1);
> /* event: ATTRIB_READ, ATTRIB_WRITE or ATTRIB_UPDATE */
> cb->event = <event>;

Are there any plans to add DBus APIs to allow finer grained read and
writes of attributes ?
For example when the application needs more control like reliable
writes, writes with response,
writes of long values.

Also whats the callback mechanism where the write fails because of
errors like lack of permissions,
we need to inform the application (using the attribute apis to
implement a GATT client ) of the actual
error.

> /* cb->read, cb->write or cp->update, depending on the event type */
> cb->write = <write_callback>;
> attrib_add_callback(a, cb)
>
> And the new signatures for callbacks:
>
> ? ? ? ? ? ? ? ?uint8_t (*read)(GAttrib *attrib, struct attribute *a);
> ? ? ? ? ? ? ? ?uint8_t (*write)(GAttrib *attrib, struct attribute *a);
> ? ? ? ? ? ? ? ?uint8_t (*update)(GAttrib *attrib, struct attribute *a);
>
> We use a union for the callback field, allowing different signatures
> for different events.
>
> Currently, the main issue which prevents us from sending the migration
> to this new API is that attrib_db_update() uses g_try_realloc(), which
> may change the allocated pointer and invalidate references to the old
> struct attribute pointer.
>
> Suggestions/comments are welcome. Regards,
> --
> Anderson Lizardo
> Instituto Nokia de Tecnologia - INdT
> Manaus - Brazil
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>