Hi Arman, On Mon, Nov 3, 2014 at 10:02 PM, Arman Uguray <armansito@xxxxxxxxxxxx> wrote: > Hi Luiz, > >> On Mon, Nov 3, 2014 at 8:06 AM, Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote: >> From: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx> >> >> 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 majordomo@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > Cheers, > Arman -- Luiz Augusto von Dentz -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
