[PATCH 1/2] Provide return status in gatt_service_add function

[PATCH 1/2] Provide return status in gatt_service_add function

[Santiago Carot-Nemesio]

Service plugins using the new GATT API may need to know if their attributes
were successfuly retgistered. In this way, plugins might abort loading
operation if they weren't registered.
---
 attrib/gatt-service.c |   15 +++++++++------
 attrib/gatt-service.h |    2 +-
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/attrib/gatt-service.c b/attrib/gatt-service.c
index ead3f8e..b904ce6 100644
--- a/attrib/gatt-service.c
+++ b/attrib/gatt-service.c
@@ -244,7 +244,7 @@ static void free_gatt_info(void *data)
 	g_free(info);
 }
 
-void gatt_service_add(uint16_t uuid, uint16_t svc_uuid, gatt_option opt1, ...)
+gboolean gatt_service_add(uint16_t uuid, uint16_t svc_uuid, gatt_option opt1, ...)
 {
 	uint16_t start_handle, h;
 	unsigned int size;
@@ -265,7 +265,8 @@ void gatt_service_add(uint16_t uuid, uint16_t svc_uuid, gatt_option opt1, ...)
 	start_handle = attrib_db_find_avail(size);
 	if (start_handle == 0) {
 		error("Not enough free handles to register service");
-		goto done;
+		g_slist_free_full(chrs, free_gatt_info);
+		return FALSE;
 	}
 
 	DBG("New service: handle 0x%04x, UUID 0x%04x, %d attributes",
@@ -282,13 +283,15 @@ void gatt_service_add(uint16_t uuid, uint16_t svc_uuid, gatt_option opt1, ...)
 		struct gatt_info *info = l->data;
 
 		DBG("New characteristic: handle 0x%04x", h);
-		if (!add_characteristic(&h, info))
-			goto done;
+		if (!add_characteristic(&h, info)) {
+			g_slist_free_full(chrs, free_gatt_info);
+			return FALSE;
+		}
 	}
 
 	g_assert(size < USHRT_MAX);
 	g_assert(h - start_handle == (uint16_t) size);
-
-done:
 	g_slist_free_full(chrs, free_gatt_info);
+
+	return TRUE;
 }
diff --git a/attrib/gatt-service.h b/attrib/gatt-service.h
index 6525dc0..95064c0 100644
--- a/attrib/gatt-service.h
+++ b/attrib/gatt-service.h
@@ -47,4 +47,4 @@ typedef enum {
 	ATTRIB_WRITE,
 } attrib_event_t;
 
-void gatt_service_add(uint16_t uuid, uint16_t svc_uuid, gatt_option opt1, ...);
+gboolean gatt_service_add(uint16_t uuid, uint16_t svc_uuid, gatt_option opt1, ...);
-- 
1.7.7.4

[PATCH 2/2] Update gatt-exmaple to check if attributes were registered.

[Santiago Carot-Nemesio]

---
 plugins/gatt-example.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/plugins/gatt-example.c b/plugins/gatt-example.c
index 1456284..6d9b6b8 100644
--- a/plugins/gatt-example.c
+++ b/plugins/gatt-example.c
@@ -28,6 +28,7 @@
 
 #include <glib.h>
 #include <bluetooth/uuid.h>
+#include <errno.h>
 
 #include "plugin.h"
 #include "hcid.h"
@@ -68,9 +69,9 @@ static uint8_t battery_state_read(struct attribute *a, gpointer user_data)
 	return 0;
 }
 
-static void register_battery_service(void)
+static gboolean register_battery_service(void)
 {
-	gatt_service_add(GATT_PRIM_SVC_UUID, BATTERY_STATE_SVC_UUID,
+	return gatt_service_add(GATT_PRIM_SVC_UUID, BATTERY_STATE_SVC_UUID,
 			/* battery state characteristic */
 			GATT_OPT_CHR_UUID, BATTERY_STATE_UUID,
 			GATT_OPT_CHR_PROPS, ATT_CHAR_PROPER_READ |
@@ -445,7 +446,11 @@ static int gatt_example_init(void)
 		return -1;
 	}
 
-	register_battery_service();
+	if (!register_battery_service()) {
+		DBG("Battery service could not be registered");
+		return -EIO;
+	}
+
 	register_manuf1_service(manuf1_range);
 	register_manuf2_service(manuf2_range);
 	register_termometer_service(manuf1_range, manuf2_range);
-- 
1.7.7.4

Re: [PATCH 1/2] Provide return status in gatt_service_add function

[Johan Hedberg]

Hi Santiago,

On Thu, Nov 24, 2011, Santiago Carot-Nemesio wrote:
> Service plugins using the new GATT API may need to know if their attributes
> were successfuly retgistered. In this way, plugins might abort loading
> operation if they weren't registered.
> ---
>  attrib/gatt-service.c |   15 +++++++++------
>  attrib/gatt-service.h |    2 +-
>  2 files changed, 10 insertions(+), 7 deletions(-)

Both patches applied after fixing typos in your commit messages. Thanks.

Johan

Re: [PATCH 1/2] Provide return status in gatt_service_add function

[Santiago Carot]

Hi Johan,

2011/12/2 Johan Hedberg <johan.hedberg@gmail.com>:
> Hi Santiago,
>
> On Thu, Nov 24, 2011, Santiago Carot-Nemesio wrote:
>> Service plugins using the new GATT API may need to know if their attributes
>> were successfuly retgistered. In this way, plugins might abort loading
>> operation if they weren't registered.
>> ---
>>  attrib/gatt-service.c |   15 +++++++++------
>>  attrib/gatt-service.h |    2 +-
>>  2 files changed, 10 insertions(+), 7 deletions(-)
>
> Both patches applied after fixing typos in your commit messages. Thanks.
>

When I was working in this stuff, I was wondering if there may exist
the case when a plugin registers several services and the last one
fails, Do you think it would have sense to provide the service ID as
return parameter to allow plugins remove their attributes before
aborting the loading operation?, On the other hand, this feature would
raise the posibility of finding gaps in the distribution of handlers
assigned in GATT due that plugins could add and remove services
dynamically, I'm not sure if that is expected in GATT.

Waiting comments.
Regards

Re: [PATCH 1/2] Provide return status in gatt_service_add function

[Anderson Lizardo]

Hi Santiago,

On Fri, Dec 2, 2011 at 7:43 AM, Santiago Carot <sancane@gmail.com> wrote:
> When I was working in this stuff, I was wondering if there may exist
> the case when a plugin registers several services and the last one
> fails, Do you think it would have sense to provide the service ID as
> return parameter to allow plugins remove their attributes before
> aborting the loading operation?, On the other hand, this feature would
> raise the posibility of finding gaps in the distribution of handlers
> assigned in GATT due that plugins could add and remove services
> dynamically, I'm not sure if that is expected in GATT.

I think that, before doing anything like this, we should ensure the
handles allocated for GATT servers do not change after registration
(i.e. they are kept in storage and reloaded when bluez starts). If
possible, can you contribute on this regard?

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

RE: [PATCH 1/2] Provide return status in gatt_service_add function

[Ganir, Chen]

Anderson,

> -----Original Message-----
> From: linux-bluetooth-owner@vger.kernel.org [mailto:linux-bluetooth-
> owner@vger.kernel.org] On Behalf Of Anderson Lizardo
> Sent: Friday, December 02, 2011 5:37 PM
> To: Santiago Carot
> Cc: linux-bluetooth@vger.kernel.org
> Subject: Re: [PATCH 1/2] Provide return status in gatt_service_add
> function
> 
> Hi Santiago,
> 
> On Fri, Dec 2, 2011 at 7:43 AM, Santiago Carot <sancane@gmail.com>
> wrote:
> > When I was working in this stuff, I was wondering if there may exist
> > the case when a plugin registers several services and the last one
> > fails, Do you think it would have sense to provide the service ID as
> > return parameter to allow plugins remove their attributes before
> > aborting the loading operation?, On the other hand, this feature
> would
> > raise the posibility of finding gaps in the distribution of handlers
> > assigned in GATT due that plugins could add and remove services
> > dynamically, I'm not sure if that is expected in GATT.
> 
> I think that, before doing anything like this, we should ensure the
> handles allocated for GATT servers do not change after registration
> (i.e. they are kept in storage and reloaded when bluez starts). If
> possible, can you contribute on this regard?
> 
> Thanks,
> --
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

How do you ensure that? Do you assume that all servers will always initialize at bluetoothd startup, and always in the same order? Who's responsibility is it to send out the "service changed" notification when something changed? What is the proposed logic for this ?

Thanks,
Chen Ganir

Re: [PATCH 1/2] Provide return status in gatt_service_add function

[Anderson Lizardo]

Hi Chen,

On Sun, Dec 4, 2011 at 2:51 AM, Ganir, Chen <chen.ganir@ti.com> wrote:
> How do you ensure that? Do you assume that all servers will always initialize at bluetoothd startup, and always in the same order? Who's responsibility is it to send out the "service changed" notification when something changed? What is the proposed logic for this ?

To make sure the handles do not change between bluetoothd restarts
(and is not dependent on any loading order), we need to save/restore
attribute handles, like is done for other BlueZ parts.

"Service Changed" is not necessary to be sent if attribute handle
storage is implemented. We have no plans to implement "Service
Changed" now, but feel free to send patches :)

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

RE: [PATCH 1/2] Provide return status in gatt_service_add function

[Ganir, Chen]

Anderson,

> -----Original Message-----
> From: Anderson Lizardo [mailto:anderson.lizardo@openbossa.org]
> Sent: Monday, December 05, 2011 12:56 PM
> To: Ganir, Chen
> Cc: Santiago Carot; linux-bluetooth@vger.kernel.org
> Subject: Re: [PATCH 1/2] Provide return status in gatt_service_add
> function
> 
> Hi Chen,
> 
> On Sun, Dec 4, 2011 at 2:51 AM, Ganir, Chen <chen.ganir@ti.com> wrote:
> > How do you ensure that? Do you assume that all servers will always
> initialize at bluetoothd startup, and always in the same order? Who's
> responsibility is it to send out the "service changed" notification
> when something changed? What is the proposed logic for this ?
> 
> To make sure the handles do not change between bluetoothd restarts
> (and is not dependent on any loading order), we need to save/restore
> attribute handles, like is done for other BlueZ parts.
> 
> "Service Changed" is not necessary to be sent if attribute handle
> storage is implemented. We have no plans to implement "Service
> Changed" now, but feel free to send patches :)
> 
> Regards,
> --
> Anderson Lizardo
> Instituto Nokia de Tecnologia - INdT
> Manaus - Brazil

What do you mean restoring the attribute handles ? Each of the profiles registers its own services when the plugin is loaded, according to the load order (which I assume will not change). When a profile registers its attributes, it can specify callbacks for read/write which are actually function pointers. How to you plan to support this? In addition, who will be responsible for reloading the database and populating the values (profile plugin or bluetoothd core) ? how do you synchronize them?

Do you have an RFC or a simple design description which will allow reviewing?

Thanks,
Chen Ganir.
 


Thanks,
Chen Ganir

Re: [PATCH 1/2] Provide return status in gatt_service_add function

[Anderson Lizardo]

Hi Chen,

On Mon, Dec 5, 2011 at 7:10 AM, Ganir, Chen <chen.ganir@ti.com> wrote:
> What do you mean restoring the attribute handles ? Each of the profiles registers its own services when the plugin is loaded, according to the load order (which I assume will not change).

There is no guarantee in the load order. As we add more plugins, you
can't predict the order the plugins will be loaded.

> When a profile registers its attributes, it can specify callbacks for read/write which are actually function pointers. How to you plan to support this?

Only attribute *handles* are to be stored (for now). The mapping could
be based on service+characteristic UUIDs (but we need to pay attention
if we have multiple services with same service and characteristic
UUIDs; for this case, I'm not even sure how the client knows which one
to use).

> In addition, who will be responsible for reloading the database and populating the values (profile plugin or bluetoothd core) ? how do you synchronize them?

My suggestion would be to implement this in attrib/gatt-service.c so
gatt_service_add() could load attribute handles from storage
transparently. This could also avoid changing every profile.

Note this is only for *server* roles (e.g. PAS server, TIP server, PXP
reporter etc.).

I'm not sure what you mean with synchronization. The attribute handles
are to be stored when the service is registered (and you can't change
them after registration).

> Do you have an RFC or a simple design description which will allow reviewing?

No. I am not working on this currently. This discussion started from
an idea from Santiago to how to handle registration failures (see
initial post), to which I responded that we may want first to address
attribute handle storage to make sure handles stay the same during
device lifetime (until it is factory reset).

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

RE: [PATCH 1/2] Provide return status in gatt_service_add function

[Ganir, Chen]

Anderson,

> -----Original Message-----
> From: Anderson Lizardo [mailto:anderson.lizardo@openbossa.org]
> Sent: Monday, December 05, 2011 1:37 PM
> To: Ganir, Chen
> Cc: Santiago Carot; linux-bluetooth@vger.kernel.org
> Subject: Re: [PATCH 1/2] Provide return status in gatt_service_add
> function
> 
> Hi Chen,
> 
> On Mon, Dec 5, 2011 at 7:10 AM, Ganir, Chen <chen.ganir@ti.com> wrote:
> > What do you mean restoring the attribute handles ? Each of the
> profiles registers its own services when the plugin is loaded,
> according to the load order (which I assume will not change).
> 
> There is no guarantee in the load order. As we add more plugins, you
> can't predict the order the plugins will be loaded.
> 
> > When a profile registers its attributes, it can specify callbacks for
> read/write which are actually function pointers. How to you plan to
> support this?
> 
> Only attribute *handles* are to be stored (for now). The mapping could
> be based on service+characteristic UUIDs (but we need to pay attention
> if we have multiple services with same service and characteristic
> UUIDs; for this case, I'm not even sure how the client knows which one
> to use).
> 
> > In addition, who will be responsible for reloading the database and
> populating the values (profile plugin or bluetoothd core) ? how do you
> synchronize them?
> 
> My suggestion would be to implement this in attrib/gatt-service.c so
> gatt_service_add() could load attribute handles from storage
> transparently. This could also avoid changing every profile.
> 
> Note this is only for *server* roles (e.g. PAS server, TIP server, PXP
> reporter etc.).
> 
> I'm not sure what you mean with synchronization. The attribute handles
> are to be stored when the service is registered (and you can't change
> them after registration).
> 
> > Do you have an RFC or a simple design description which will allow
> reviewing?
> 
> No. I am not working on this currently. This discussion started from
> an idea from Santiago to how to handle registration failures (see
> initial post), to which I responded that we may want first to address
> attribute handle storage to make sure handles stay the same during
> device lifetime (until it is factory reset).
> 
> Regards,
> --
> Anderson Lizardo
> Instituto Nokia de Tecnologia - INdT
> Manaus - Brazil

Thanks.

Chen Ganir.

Re: [PATCH 1/2] Provide return status in gatt_service_add function

[Santiago Carot]

Hi Chen, Lizardo,

2011/12/5 Anderson Lizardo <anderson.lizardo@openbossa.org>:
> Hi Chen,
>
> On Mon, Dec 5, 2011 at 7:10 AM, Ganir, Chen <chen.ganir@ti.com> wrote:
>> What do you mean restoring the attribute handles ? Each of the profiles registers its own services when the plugin is loaded, according to the load order (which I assume will not change).
>
> There is no guarantee in the load order. As we add more plugins, you
> can't predict the order the plugins will be loaded.
>
>> When a profile registers its attributes, it can specify callbacks for read/write which are actually function pointers. How to you plan to support this?
>
> Only attribute *handles* are to be stored (for now). The mapping could
> be based on service+characteristic UUIDs (but we need to pay attention
> if we have multiple services with same service and characteristic
> UUIDs; for this case, I'm not even sure how the client knows which one
> to use).
>

I was thinking in using a sort of server_id to distinguish between
services registered among different plugins. That a first idea I have,
I'm going to try to send a RFCs patches during these days. As always,
I'll be updating my git repository at github if you want to follow my
progress.


>> In addition, who will be responsible for reloading the database and populating the values (profile plugin or bluetoothd core) ? how do you synchronize them?
>
> My suggestion would be to implement this in attrib/gatt-service.c so
> gatt_service_add() could load attribute handles from storage
> transparently. This could also avoid changing every profile.
>
> Note this is only for *server* roles (e.g. PAS server, TIP server, PXP
> reporter etc.).
>
> I'm not sure what you mean with synchronization. The attribute handles
> are to be stored when the service is registered (and you can't change
> them after registration).
>
>> Do you have an RFC or a simple design description which will allow reviewing?
>
> No. I am not working on this currently. This discussion started from
> an idea from Santiago to how to handle registration failures (see
> initial post), to which I responded that we may want first to address
> attribute handle storage to make sure handles stay the same during
> device lifetime (until it is factory reset).
>

I'll put my hands on this issue.

Regards