From: Luiz Augusto von Dentz <[email protected]>
Send as RFC to collect for feedback regarding API decisions, the
question is if we shall expose gatt_db_service or let the user access
it via gatt_db_attrib since it is quite easy to access it internally.
Either way this will end up in heavy changes to android/gatt.c since it
is very dependent on access by handle.
---
src/shared/gatt-db.h | 97 ++++++++++++++++++++++++----------------------------
1 file changed, 45 insertions(+), 52 deletions(-)
diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index 15be67f..4d9bfc1 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -22,43 +22,62 @@
*/
struct gatt_db;
+struct gatt_db_service;
+struct gatt_db_attribute;
struct gatt_db *gatt_db_new(void);
void gatt_db_destroy(struct gatt_db *db);
-uint16_t gatt_db_add_service(struct gatt_db *db, const bt_uuid_t *uuid,
- bool primary, uint16_t num_handles);
-bool gatt_db_remove_service(struct gatt_db *db, uint16_t handle);
+struct gatt_db_service *gatt_db_add_service(struct gatt_db *db,
+ const bt_uuid_t *uuid,
+ bool primary,
+ uint16_t num_handles);
+bool gatt_db_remove_service(struct gatt_db *db,
+ struct gatt_db_service *service);
-typedef void (*gatt_db_read_t) (uint16_t handle, uint16_t offset,
- uint8_t att_opcode, bdaddr_t *bdaddr,
- void *user_data);
+uint16_t gatt_db_service_get_handle(struct gatt_db_service *service);
-typedef void (*gatt_db_write_t) (uint16_t handle, uint16_t offset,
- const uint8_t *value, size_t len,
- uint8_t att_opcode, bdaddr_t *bdaddr,
+typedef void (*gatt_db_attribute_read_t) (struct gatt_db_attribute *attrib,
+ int err, uint8_t *value, size_t length,
void *user_data);
-uint16_t gatt_db_add_characteristic(struct gatt_db *db, uint16_t handle,
- const bt_uuid_t *uuid,
- uint32_t permissions,
- uint8_t properties,
- gatt_db_read_t read_func,
- gatt_db_write_t write_func,
- void *user_data);
+typedef void (*gatt_db_read_t) (struct gatt_db_attribute *attrib,
+ uint16_t offset, uint8_t opcode,
+ bdaddr_t *bdaddr,
+ gatt_db_attribute_read_t func, void *func_data,
+ void *user_data);
-uint16_t gatt_db_add_char_descriptor(struct gatt_db *db, uint16_t handle,
- const bt_uuid_t *uuid,
- uint32_t permissions,
- gatt_db_read_t read_func,
- gatt_db_write_t write_func,
- void *user_data);
+typedef void (*gatt_db_attribute_write_t) (struct gatt_db_attribute *attrib,
+ int err, void *user_data);
+
+typedef void (*gatt_db_write_t) (struct gatt_db_attribute *attrib,
+ uint16_t offset, const uint8_t *value,
+ size_t len, uint8_t opcode, bdaddr_t *bdaddr,
+ gatt_db_attribute_write_t func, void *func_data,
+ void *user_data);
+
+struct gatt_db_attribute *
+gatt_db_service_add_characteristic(struct gatt_db_service *service,
+ const bt_uuid_t *uuid,
+ uint32_t permissions,
+ uint8_t properties,
+ gatt_db_read_t read_func,
+ gatt_db_write_t write_func,
+ void *user_data);
-uint16_t gatt_db_add_included_service(struct gatt_db *db, uint16_t handle,
- uint16_t included_handle);
+struct gatt_db_attribute *
+gatt_db_service_add_descriptor(struct gatt_db_service *service,
+ const bt_uuid_t *uuid,
+ uint32_t permissions,
+ gatt_db_read_t read_func,
+ gatt_db_write_t write_func,
+ void *user_data);
+
+struct gatt_db_attribute *
+gatt_db_service_add_included(struct gatt_db_service *service,
+ struct gatt_db_service *include);
-bool gatt_db_service_set_active(struct gatt_db *db, uint16_t handle,
- bool active);
+bool gatt_db_service_set_active(struct gatt_db_service *service, bool active);
void gatt_db_read_by_group_type(struct gatt_db *db, uint16_t start_handle,
uint16_t end_handle,
@@ -79,25 +98,6 @@ void gatt_db_find_information(struct gatt_db *db, uint16_t start_handle,
uint16_t end_handle,
struct queue *queue);
-bool gatt_db_read(struct gatt_db *db, uint16_t handle, uint16_t offset,
- uint8_t att_opcode, bdaddr_t *bdaddr,
- uint8_t **value, int *length);
-
-bool gatt_db_write(struct gatt_db *db, uint16_t handle, uint16_t offset,
- const uint8_t *value, size_t len,
- uint8_t att_opcode, bdaddr_t *bdaddr);
-
-const bt_uuid_t *gatt_db_get_attribute_type(struct gatt_db *db,
- uint16_t handle);
-
-uint16_t gatt_db_get_end_handle(struct gatt_db *db, uint16_t handle);
-bool gatt_db_get_service_uuid(struct gatt_db *db, uint16_t handle,
- bt_uuid_t *uuid);
-
-bool gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle,
- uint32_t *permissions);
-
-struct gatt_db_attribute;
struct gatt_db_attribute *gatt_db_get_attribute(struct gatt_db *db,
uint16_t handle);
@@ -116,17 +116,10 @@ bool gatt_db_attribute_get_service_handles(struct gatt_db_attribute *attrib,
bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
uint32_t *permissions);
-typedef void (*gatt_db_attribute_read_t) (struct gatt_db_attribute *attrib,
- int err, uint8_t *value, size_t length,
- void *user_data);
-
bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
uint8_t opcode, bdaddr_t *bdaddr,
gatt_db_attribute_read_t func, void *user_data);
-typedef void (*gatt_db_attribute_write_t) (struct gatt_db_attribute *attrib,
- int err, void *user_data);
-
bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
const uint8_t *value, size_t len,
uint8_t opcode, bdaddr_t *bdaddr,
--
1.9.3
Hi Arman,
On Mon, Nov 3, 2014 at 10:02 PM, Arman Uguray <[email protected]> wrote:
> Hi Luiz,
>
>> On Mon, Nov 3, 2014 at 8:06 AM, Luiz Augusto von Dentz <[email protected]> wrote:
>> From: Luiz Augusto von Dentz <[email protected]>
>>
>> Send as RFC to collect for feedback regarding API decisions, the
>> question is if we shall expose gatt_db_service or let the user access
>> it via gatt_db_attrib since it is quite easy to access it internally.
>>
>> Either way this will end up in heavy changes to android/gatt.c since it
>> is very dependent on access by handle.
>> ---
>> src/shared/gatt-db.h | 97 ++++++++++++++++++++++++----------------------------
>> 1 file changed, 45 insertions(+), 52 deletions(-)
>>
>> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
>> index 15be67f..4d9bfc1 100644
>> --- a/src/shared/gatt-db.h
>> +++ b/src/shared/gatt-db.h
>> @@ -22,43 +22,62 @@
>> */
>>
>> struct gatt_db;
>> +struct gatt_db_service;
>> +struct gatt_db_attribute;
>>
>> struct gatt_db *gatt_db_new(void);
>> void gatt_db_destroy(struct gatt_db *db);
>>
>> -uint16_t gatt_db_add_service(struct gatt_db *db, const bt_uuid_t *uuid,
>> - bool primary, uint16_t num_handles);
>> -bool gatt_db_remove_service(struct gatt_db *db, uint16_t handle);
>> +struct gatt_db_service *gatt_db_add_service(struct gatt_db *db,
>> + const bt_uuid_t *uuid,
>> + bool primary,
>> + uint16_t num_handles);
>> +bool gatt_db_remove_service(struct gatt_db *db,
>> + struct gatt_db_service *service);
>>
>
> Hmm, I'm a bit unsure about exposing a gatt_db_service, since I don't
> see the gain from it. There is also the whole ownership issue, since
> the object the pointer is referencing would be owned by gatt_db but by
> returning it from gatt_db_add_service, we're implying that profiles
> can hold on to it. So maybe we should return a handle from here and
> add a function like gatt_db_get_service(db, handle) to obtain the
> service pointer on demand? I still don't see the use though, since
> this is pretty much only used with gatt_db_add_characteristic and
> gatt_db_add_char_desc.
The point is to simplify the code and avoid unnecessary look-ups by
handle, and I did mention that perhaps gatt_db_attribute is enough,
for the ownership we might need to have proper reference count per
gatt_db_attribute anyway since it is now exposed.
>> -typedef void (*gatt_db_read_t) (uint16_t handle, uint16_t offset,
>> - uint8_t att_opcode, bdaddr_t *bdaddr,
>> - void *user_data);
>> +uint16_t gatt_db_service_get_handle(struct gatt_db_service *service);
>>
>> -typedef void (*gatt_db_write_t) (uint16_t handle, uint16_t offset,
>> - const uint8_t *value, size_t len,
>> - uint8_t att_opcode, bdaddr_t *bdaddr,
>> +typedef void (*gatt_db_attribute_read_t) (struct gatt_db_attribute *attrib,
>> + int err, uint8_t *value, size_t length,
>> void *user_data);
>>
>> -uint16_t gatt_db_add_characteristic(struct gatt_db *db, uint16_t handle,
>> - const bt_uuid_t *uuid,
>> - uint32_t permissions,
>> - uint8_t properties,
>> - gatt_db_read_t read_func,
>> - gatt_db_write_t write_func,
>> - void *user_data);
>> +typedef void (*gatt_db_read_t) (struct gatt_db_attribute *attrib,
>> + uint16_t offset, uint8_t opcode,
>> + bdaddr_t *bdaddr,
>> + gatt_db_attribute_read_t func, void *func_data,
>> + void *user_data);
>
> It's still not obvious by reading the typedefs and argument names that
> "func" of type "gatt_db_attribute_read_t" should be used by
> gatt_db_read_t to report the result of the read operation. Let's
> either change the argument names to "result_func" (or something like
> that if you have a better name in mind) or at least put in comments
> here that a gatt_db_read_t implementation must invoke "func" to signal
> the completion of the read procedure. IMO we should to make this
> really obvious so that when a developer wants to implement a plugin,
> they don't have to dig through existing profile code to figure this
> out. Same thing for the write callbacks.
Im actually reconsidering passing the callbacks directly and perhaps
replace with proper functions to signal the result e.g.
gatt_db_attribute_read_result/gatt_db_attribute_write_result that way
the db itself should keep track of calling the callback which is
already the case for values stored in the db itself, then I would
replace the callback and callback data with a pending id which should
be passed to the result to complete the operation.
> To that note, we should probably have a well documented example plugin
> to replace the outdated once and possibly a gatt client/server guide
> somewhere in doc/ eventually.
Indeed we should document this API, either with callbacks or having
dedicated functions we will need to document what it is the expected
behavior.
>
>>
>> -uint16_t gatt_db_add_char_descriptor(struct gatt_db *db, uint16_t handle,
>> - const bt_uuid_t *uuid,
>> - uint32_t permissions,
>> - gatt_db_read_t read_func,
>> - gatt_db_write_t write_func,
>> - void *user_data);
>> +typedef void (*gatt_db_attribute_write_t) (struct gatt_db_attribute *attrib,
>> + int err, void *user_data);
>> +
>> +typedef void (*gatt_db_write_t) (struct gatt_db_attribute *attrib,
>> + uint16_t offset, const uint8_t *value,
>> + size_t len, uint8_t opcode, bdaddr_t *bdaddr,
>> + gatt_db_attribute_write_t func, void *func_data,
>> + void *user_data);
>> +
>> +struct gatt_db_attribute *
>> +gatt_db_service_add_characteristic(struct gatt_db_service *service,
>> + const bt_uuid_t *uuid,
>> + uint32_t permissions,
>> + uint8_t properties,
>> + gatt_db_read_t read_func,
>> + gatt_db_write_t write_func,
>> + void *user_data);
>>
>> -uint16_t gatt_db_add_included_service(struct gatt_db *db, uint16_t handle,
>> - uint16_t included_handle);
>> +struct gatt_db_attribute *
>> +gatt_db_service_add_descriptor(struct gatt_db_service *service,
>> + const bt_uuid_t *uuid,
>> + uint32_t permissions,
>> + gatt_db_read_t read_func,
>> + gatt_db_write_t write_func,
>> + void *user_data);
>> +
>> +struct gatt_db_attribute *
>> +gatt_db_service_add_included(struct gatt_db_service *service,
>> + struct gatt_db_service *include);
>>
>> -bool gatt_db_service_set_active(struct gatt_db *db, uint16_t handle,
>> - bool active);
>> +bool gatt_db_service_set_active(struct gatt_db_service *service, bool active);
>>
>> void gatt_db_read_by_group_type(struct gatt_db *db, uint16_t start_handle,
>> uint16_t end_handle,
>> @@ -79,25 +98,6 @@ void gatt_db_find_information(struct gatt_db *db, uint16_t start_handle,
>> uint16_t end_handle,
>> struct queue *queue);
>>
>> -bool gatt_db_read(struct gatt_db *db, uint16_t handle, uint16_t offset,
>> - uint8_t att_opcode, bdaddr_t *bdaddr,
>> - uint8_t **value, int *length);
>> -
>> -bool gatt_db_write(struct gatt_db *db, uint16_t handle, uint16_t offset,
>> - const uint8_t *value, size_t len,
>> - uint8_t att_opcode, bdaddr_t *bdaddr);
>> -
>> -const bt_uuid_t *gatt_db_get_attribute_type(struct gatt_db *db,
>> - uint16_t handle);
>> -
>> -uint16_t gatt_db_get_end_handle(struct gatt_db *db, uint16_t handle);
>> -bool gatt_db_get_service_uuid(struct gatt_db *db, uint16_t handle,
>> - bt_uuid_t *uuid);
>> -
>> -bool gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle,
>> - uint32_t *permissions);
>> -
>> -struct gatt_db_attribute;
>>
>> struct gatt_db_attribute *gatt_db_get_attribute(struct gatt_db *db,
>> uint16_t handle);
>> @@ -116,17 +116,10 @@ bool gatt_db_attribute_get_service_handles(struct gatt_db_attribute *attrib,
>> bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
>> uint32_t *permissions);
>>
>> -typedef void (*gatt_db_attribute_read_t) (struct gatt_db_attribute *attrib,
>> - int err, uint8_t *value, size_t length,
>> - void *user_data);
>> -
>> bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
>> uint8_t opcode, bdaddr_t *bdaddr,
>> gatt_db_attribute_read_t func, void *user_data);
>>
>> -typedef void (*gatt_db_attribute_write_t) (struct gatt_db_attribute *attrib,
>> - int err, void *user_data);
>> -
>> bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>> const uint8_t *value, size_t len,
>> uint8_t opcode, bdaddr_t *bdaddr,
>> --
>> 1.9.3
>>
>> --
>> 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
>
> Cheers,
> Arman
--
Luiz Augusto von Dentz
Hi Luiz,
> On Mon, Nov 3, 2014 at 8:06 AM, Luiz Augusto von Dentz <[email protected]> wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> Send as RFC to collect for feedback regarding API decisions, the
> question is if we shall expose gatt_db_service or let the user access
> it via gatt_db_attrib since it is quite easy to access it internally.
>
> Either way this will end up in heavy changes to android/gatt.c since it
> is very dependent on access by handle.
> ---
> src/shared/gatt-db.h | 97 ++++++++++++++++++++++++----------------------------
> 1 file changed, 45 insertions(+), 52 deletions(-)
>
> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
> index 15be67f..4d9bfc1 100644
> --- a/src/shared/gatt-db.h
> +++ b/src/shared/gatt-db.h
> @@ -22,43 +22,62 @@
> */
>
> struct gatt_db;
> +struct gatt_db_service;
> +struct gatt_db_attribute;
>
> struct gatt_db *gatt_db_new(void);
> void gatt_db_destroy(struct gatt_db *db);
>
> -uint16_t gatt_db_add_service(struct gatt_db *db, const bt_uuid_t *uuid,
> - bool primary, uint16_t num_handles);
> -bool gatt_db_remove_service(struct gatt_db *db, uint16_t handle);
> +struct gatt_db_service *gatt_db_add_service(struct gatt_db *db,
> + const bt_uuid_t *uuid,
> + bool primary,
> + uint16_t num_handles);
> +bool gatt_db_remove_service(struct gatt_db *db,
> + struct gatt_db_service *service);
>
Hmm, I'm a bit unsure about exposing a gatt_db_service, since I don't
see the gain from it. There is also the whole ownership issue, since
the object the pointer is referencing would be owned by gatt_db but by
returning it from gatt_db_add_service, we're implying that profiles
can hold on to it. So maybe we should return a handle from here and
add a function like gatt_db_get_service(db, handle) to obtain the
service pointer on demand? I still don't see the use though, since
this is pretty much only used with gatt_db_add_characteristic and
gatt_db_add_char_desc.
> -typedef void (*gatt_db_read_t) (uint16_t handle, uint16_t offset,
> - uint8_t att_opcode, bdaddr_t *bdaddr,
> - void *user_data);
> +uint16_t gatt_db_service_get_handle(struct gatt_db_service *service);
>
> -typedef void (*gatt_db_write_t) (uint16_t handle, uint16_t offset,
> - const uint8_t *value, size_t len,
> - uint8_t att_opcode, bdaddr_t *bdaddr,
> +typedef void (*gatt_db_attribute_read_t) (struct gatt_db_attribute *attrib,
> + int err, uint8_t *value, size_t length,
> void *user_data);
>
> -uint16_t gatt_db_add_characteristic(struct gatt_db *db, uint16_t handle,
> - const bt_uuid_t *uuid,
> - uint32_t permissions,
> - uint8_t properties,
> - gatt_db_read_t read_func,
> - gatt_db_write_t write_func,
> - void *user_data);
> +typedef void (*gatt_db_read_t) (struct gatt_db_attribute *attrib,
> + uint16_t offset, uint8_t opcode,
> + bdaddr_t *bdaddr,
> + gatt_db_attribute_read_t func, void *func_data,
> + void *user_data);
It's still not obvious by reading the typedefs and argument names that
"func" of type "gatt_db_attribute_read_t" should be used by
gatt_db_read_t to report the result of the read operation. Let's
either change the argument names to "result_func" (or something like
that if you have a better name in mind) or at least put in comments
here that a gatt_db_read_t implementation must invoke "func" to signal
the completion of the read procedure. IMO we should to make this
really obvious so that when a developer wants to implement a plugin,
they don't have to dig through existing profile code to figure this
out. Same thing for the write callbacks.
To that note, we should probably have a well documented example plugin
to replace the outdated once and possibly a gatt client/server guide
somewhere in doc/ eventually.
>
> -uint16_t gatt_db_add_char_descriptor(struct gatt_db *db, uint16_t handle,
> - const bt_uuid_t *uuid,
> - uint32_t permissions,
> - gatt_db_read_t read_func,
> - gatt_db_write_t write_func,
> - void *user_data);
> +typedef void (*gatt_db_attribute_write_t) (struct gatt_db_attribute *attrib,
> + int err, void *user_data);
> +
> +typedef void (*gatt_db_write_t) (struct gatt_db_attribute *attrib,
> + uint16_t offset, const uint8_t *value,
> + size_t len, uint8_t opcode, bdaddr_t *bdaddr,
> + gatt_db_attribute_write_t func, void *func_data,
> + void *user_data);
> +
> +struct gatt_db_attribute *
> +gatt_db_service_add_characteristic(struct gatt_db_service *service,
> + const bt_uuid_t *uuid,
> + uint32_t permissions,
> + uint8_t properties,
> + gatt_db_read_t read_func,
> + gatt_db_write_t write_func,
> + void *user_data);
>
> -uint16_t gatt_db_add_included_service(struct gatt_db *db, uint16_t handle,
> - uint16_t included_handle);
> +struct gatt_db_attribute *
> +gatt_db_service_add_descriptor(struct gatt_db_service *service,
> + const bt_uuid_t *uuid,
> + uint32_t permissions,
> + gatt_db_read_t read_func,
> + gatt_db_write_t write_func,
> + void *user_data);
> +
> +struct gatt_db_attribute *
> +gatt_db_service_add_included(struct gatt_db_service *service,
> + struct gatt_db_service *include);
>
> -bool gatt_db_service_set_active(struct gatt_db *db, uint16_t handle,
> - bool active);
> +bool gatt_db_service_set_active(struct gatt_db_service *service, bool active);
>
> void gatt_db_read_by_group_type(struct gatt_db *db, uint16_t start_handle,
> uint16_t end_handle,
> @@ -79,25 +98,6 @@ void gatt_db_find_information(struct gatt_db *db, uint16_t start_handle,
> uint16_t end_handle,
> struct queue *queue);
>
> -bool gatt_db_read(struct gatt_db *db, uint16_t handle, uint16_t offset,
> - uint8_t att_opcode, bdaddr_t *bdaddr,
> - uint8_t **value, int *length);
> -
> -bool gatt_db_write(struct gatt_db *db, uint16_t handle, uint16_t offset,
> - const uint8_t *value, size_t len,
> - uint8_t att_opcode, bdaddr_t *bdaddr);
> -
> -const bt_uuid_t *gatt_db_get_attribute_type(struct gatt_db *db,
> - uint16_t handle);
> -
> -uint16_t gatt_db_get_end_handle(struct gatt_db *db, uint16_t handle);
> -bool gatt_db_get_service_uuid(struct gatt_db *db, uint16_t handle,
> - bt_uuid_t *uuid);
> -
> -bool gatt_db_get_attribute_permissions(struct gatt_db *db, uint16_t handle,
> - uint32_t *permissions);
> -
> -struct gatt_db_attribute;
>
> struct gatt_db_attribute *gatt_db_get_attribute(struct gatt_db *db,
> uint16_t handle);
> @@ -116,17 +116,10 @@ bool gatt_db_attribute_get_service_handles(struct gatt_db_attribute *attrib,
> bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
> uint32_t *permissions);
>
> -typedef void (*gatt_db_attribute_read_t) (struct gatt_db_attribute *attrib,
> - int err, uint8_t *value, size_t length,
> - void *user_data);
> -
> bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
> uint8_t opcode, bdaddr_t *bdaddr,
> gatt_db_attribute_read_t func, void *user_data);
>
> -typedef void (*gatt_db_attribute_write_t) (struct gatt_db_attribute *attrib,
> - int err, void *user_data);
> -
> bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
> const uint8_t *value, size_t len,
> uint8_t opcode, bdaddr_t *bdaddr,
> --
> 1.9.3
>
> --
> 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
Cheers,
Arman