[PATCH BlueZ 1/3] thermometer: Fix device driver probe

[PATCH BlueZ 1/3] thermometer: Fix device driver probe

[Anderson Lizardo]

From: Bruna Moreira <bruna.moreira@openbossa.org>

The probe() function for GATT profiles should be transport agnostic.
There is a btd_device_get_primaries() method which can be used to return
a list of discovered GATT Primary Services, and it works for BR/EDR and
LE. It is already used for Proximity, for instance.

device_services_from_record() is BR/EDR specific and should not be used
by GATT profile code.

It also fixes a memory leak, given device_services_from_record() returns
a heap allocated GSList.
---
 thermometer/manager.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/thermometer/manager.c b/thermometer/manager.c
index 6b98bca..9dfbbe8 100644
--- a/thermometer/manager.c
+++ b/thermometer/manager.c
@@ -34,16 +34,27 @@
 
 static DBusConnection *connection = NULL;
 
+static gint primary_uuid_cmp(gconstpointer a, gconstpointer b)
+{
+	const struct att_primary *prim = a;
+	const char *uuid = b;
+
+	return g_strcmp0(prim->uuid, uuid);
+}
+
 static int thermometer_driver_probe(struct btd_device *device, GSList *uuids)
 {
 	struct att_primary *tattr;
-	GSList *list;
+	GSList *primaries, *l;
+
+	primaries = btd_device_get_primaries(device);
 
-	list = device_services_from_record(device, uuids);
-	if (list == NULL)
+	l = g_slist_find_custom(primaries, HEALTH_THERMOMETER_UUID,
+							primary_uuid_cmp);
+	if (l == NULL)
 		return -EINVAL;
 
-	tattr = list->data;
+	tattr = l->data;
 
 	return thermometer_register(connection, device, tattr);
 }
-- 
1.7.5.4

[PATCH BlueZ 2/3] thermometer: Fix handling of missing Temperature Type

[Anderson Lizardo]

According to HTS 1.0, it is not mandatory to have Temperature Type
information (the Characteristic and the indication field are both
unconditionally optional).

Given there is no default value specified on the spec, we assume the
thermometer is for general use, and set the type to "Body".
---
 thermometer/thermometer.c |    9 ++-------
 1 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index 9bf9881..7906d84 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -1086,13 +1086,8 @@ static void proc_measurement(struct thermometer *t, const uint8_t *pdu,
 		type = temptype2str(pdu[index]);
 	} else if (t->has_type)
 		type = temptype2str(t->type);
-	else {
-		DBG("Can't get temperature type");
-		return;
-	}
-
-	if (type == NULL)
-		return;
+	else
+		type = "Body";
 
 	m.type = g_strdup(type);
 	m.value = final ? "Final" : "Intermediate";
-- 
1.7.5.4

Re: [PATCH BlueZ 2/3] thermometer: Fix handling of missing Temperature Type

[Santiago Carot]

Hi Anderson,

2012/2/14 Anderson Lizardo <anderson.lizardo@openbossa.org>:
> According to HTS 1.0, it is not mandatory to have Temperature Type
> information (the Characteristic and the indication field are both
> unconditionally optional).
>

I'm afraid there is an slight conceptual error in this patch.
According to HTS 1.0, there are two *exclusive* methods to provide the
temperature type to a Colector: either by using a characteristic type
or providing it as a field within the measurement. This doesn't mean
that the temperature characteristic or field type are both
unconditionally optional because one of them *MUST* be used, but not
both of them at the same time. So there will always be a method to get
the temperature type either if it's a static value (characteristic
type) or a non-static value (temperature field). Health Thermometer
Service[3.2] ,[3.1] &  Health Thermometer Profile [4.7]


Regards.
Santiago

Re: [PATCH BlueZ 2/3] thermometer: Fix handling of missing Temperature Type

[Anderson Lizardo]

Hi Santiago,

On Wed, Feb 15, 2012 at 4:50 AM, Santiago Carot <sancane@gmail.com> wrote:
> Hi Anderson,
>
> 2012/2/14 Anderson Lizardo <anderson.lizardo@openbossa.org>:
>> According to HTS 1.0, it is not mandatory to have Temperature Type
>> information (the Characteristic and the indication field are both
>> unconditionally optional).
>>
>
> I'm afraid there is an slight conceptual error in this patch.
> According to HTS 1.0, there are two *exclusive* methods to provide the
> temperature type to a Colector: either by using a characteristic type
> or providing it as a field within the measurement. This doesn't mean
> that the temperature characteristic or field type are both
> unconditionally optional because one of them *MUST* be used, but not
> both of them at the same time. So there will always be a method to get
> the temperature type either if it's a static value (characteristic
> type) or a non-static value (temperature field). Health Thermometer
> Service[3.2] ,[3.1] &  Health Thermometer Profile [4.7]

I've seen these sections. And they are exclusive, but none of them are
mandatory. The reason for this patch is that we have seen an LE
thermometer without any method to get its type (and the current
upstream code simply ignores its measurements). Looking at both
HTS/HTP and developer.bluetooth.org does not mention them as
"conditionally optional" (usually shown by a C<n>  note on the service
table on service specification).

Please feel free to refer to where this "must" (usually a sentence
using "shall" word) condition is described.

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

Re: [PATCH BlueZ 2/3] thermometer: Fix handling of missing Temperature Type

[Santiago Carot]

Hi Anderson

2012/2/15 Anderson Lizardo <anderson.lizardo@openbossa.org>:
> Hi Santiago,
>
> On Wed, Feb 15, 2012 at 4:50 AM, Santiago Carot <sancane@gmail.com> wrote:
>> Hi Anderson,
>>
>> 2012/2/14 Anderson Lizardo <anderson.lizardo@openbossa.org>:
>>> According to HTS 1.0, it is not mandatory to have Temperature Type
>>> information (the Characteristic and the indication field are both
>>> unconditionally optional).
>>>
>>
>> I'm afraid there is an slight conceptual error in this patch.
>> According to HTS 1.0, there are two *exclusive* methods to provide the
>> temperature type to a Colector: either by using a characteristic type
>> or providing it as a field within the measurement. This doesn't mean
>> that the temperature characteristic or field type are both
>> unconditionally optional because one of them *MUST* be used, but not
>> both of them at the same time. So there will always be a method to get
>> the temperature type either if it's a static value (characteristic
>> type) or a non-static value (temperature field). Health Thermometer
>> Service[3.2] ,[3.1] &  Health Thermometer Profile [4.7]
>
> I've seen these sections. And they are exclusive, but none of them are
> mandatory. The reason for this patch is that we have seen an LE
> thermometer without any method to get its type (and the current
> upstream code simply ignores its measurements). Looking at both
> HTS/HTP and developer.bluetooth.org does not mention them as
> "conditionally optional" (usually shown by a C<n>  note on the service
> table on service specification).
>

In any case, I don't think the best solution is to provide a value for
Type for wich we don't have enough information to determine what it
is, I think that in the case in wich Type is really optional, the
value provided in the measurement should be optional too. Under this
case it would be better not to provide anything in the
MeasurementReceived callback instead of providing a invented (Body)
value.

Regards

Re: [PATCH BlueZ 2/3] thermometer: Fix handling of missing Temperature Type

[Anderson Lizardo]

Hi Santiago,

On Wed, Feb 15, 2012 at 7:06 AM, Santiago Carot <sancane@gmail.com> wrote:
> In any case, I don't think the best solution is to provide a value for
> Type for wich we don't have enough information to determine what it
> is, I think that in the case in wich Type is really optional, the
> value provided in the measurement should be optional too. Under this
> case it would be better not to provide anything in the
> MeasurementReceived callback instead of providing a invented (Body)
> value.

I will update the patch to do this.

>
> Regards

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

Re: [PATCH BlueZ 2/3] thermometer: Fix handling of missing Temperature Type

[Anderson Lizardo]

Hi Santiago,

On Wed, Feb 15, 2012 at 7:06 AM, Santiago Carot <sancane@gmail.com> wrote:
> In any case, I don't think the best solution is to provide a value for
> Type for wich we don't have enough information to determine what it
> is, I think that in the case in wich Type is really optional, the
> value provided in the measurement should be optional too. Under this
> case it would be better not to provide anything in the
> MeasurementReceived callback instead of providing a invented (Body)
> value.

I just sent new patches. Only this one was modified, and I added more
details to the commit message so I hope it is clear now.

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

[PATCH BlueZ 3/3] thermometer: Fix re-enabling notification/indication on reconnection

[Anderson Lizardo]

If a result callback is not specified for gatt_write_char() it is
assumed that Write Command will be used. This is not valid for
Characteristic Descriptors, which only support Write Request.
---
 thermometer/thermometer.c |   35 +++++++++++++++++++++--------------
 1 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index 7906d84..d09c63c 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -331,6 +331,17 @@ static void valid_range_desc_cb(guint8 status, const guint8 *pdu, guint16 len,
 	change_property(desc->ch->t, "Minimum", &min);
 }
 
+static void measurement_cb(guint8 status, const guint8 *pdu,
+						guint16 len, gpointer user_data)
+{
+	char *msg = user_data;
+
+	if (status != 0)
+		error("%s failed", msg);
+
+	g_free(msg);
+}
+
 static void process_thermometer_desc(struct descriptor *desc)
 {
 	struct characteristic *ch = desc->ch;
@@ -342,6 +353,7 @@ static void process_thermometer_desc(struct descriptor *desc)
 	if (bt_uuid_cmp(&desc->uuid, &btuuid) == 0) {
 		uint8_t atval[2];
 		uint16_t val;
+		char *msg;
 
 		if (g_strcmp0(ch->attr.uuid,
 					TEMPERATURE_MEASUREMENT_UUID) == 0) {
@@ -349,21 +361,27 @@ static void process_thermometer_desc(struct descriptor *desc)
 				return;
 
 			val = ATT_CLIENT_CHAR_CONF_INDICATION;
+			msg = g_strdup("Enable Temperature Measurement "
+								"indication");
 		} else if (g_strcmp0(ch->attr.uuid,
 					INTERMEDIATE_TEMPERATURE_UUID) == 0) {
 			if (g_slist_length(ch->t->iwatchers) == 0)
 				return;
 
 			val = ATT_CLIENT_CHAR_CONF_NOTIFICATION;
+			msg = g_strdup("Enable Intermediate Temperature "
+								"notification");
 		} else if (g_strcmp0(ch->attr.uuid,
-					MEASUREMENT_INTERVAL_UUID) == 0)
+					MEASUREMENT_INTERVAL_UUID) == 0) {
 			val = ATT_CLIENT_CHAR_CONF_INDICATION;
-		else
+			msg = g_strdup("Enable Measurement Interval "
+								"indication");
+		} else
 			goto done;
 
 		att_put_u16(val, atval);
 		gatt_write_char(ch->t->attrib, desc->handle, atval, 2,
-								NULL, NULL);
+							measurement_cb, msg);
 		return;
 	}
 
@@ -655,17 +673,6 @@ static DBusMessage *set_property(DBusConnection *conn, DBusMessage *msg,
 	return write_attr_interval(t, msg, value);
 }
 
-static void measurement_cb(guint8 status, const guint8 *pdu,
-						guint16 len, gpointer user_data)
-{
-	char *msg = user_data;
-
-	if (status != 0)
-		error("%s failed", msg);
-
-	g_free(msg);
-}
-
 static void enable_final_measurement(struct thermometer *t)
 {
 	struct characteristic *ch;
-- 
1.7.5.4

Re: [PATCH BlueZ 3/3] thermometer: Fix re-enabling notification/indication on reconnection

[Santiago Carot]

Hi Anderson,

2012/2/14 Anderson Lizardo <anderson.lizardo@openbossa.org>:
> If a result callback is not specified for gatt_write_char() it is
> assumed that Write Command will be used. This is not valid for
> Characteristic Descriptors, which only support Write Request.

I'm not against this patch but from my point of view, I think this is
a little obscure behaviour wich would deserve an advice coment in
gatt.h due it isn't clear for anybody developping over GATT to guess
it. On the other hand, _I remember another functions in gatt.h wich
have similar double behaviour depending on the parameters provided in
there, suach as gatt_discovery_primary, in wich a discover ALL primary
service is done if the uuid is equal to NULL or disvovery service by
UUID is done if uuid is provided, I think gatt_discover_char works in
the same way. Maybe that having a good comments here could help to
avoid this kind of errors in the future.

Regards.
Santiago

Re: [PATCH BlueZ 3/3] thermometer: Fix re-enabling notification/indication on reconnection

[Anderson Lizardo]

Hi Santiago,

On Wed, Feb 15, 2012 at 5:03 AM, Santiago Carot <sancane@gmail.com> wrote:
> Hi Anderson,
>
> 2012/2/14 Anderson Lizardo <anderson.lizardo@openbossa.org>:
>> If a result callback is not specified for gatt_write_char() it is
>> assumed that Write Command will be used. This is not valid for
>> Characteristic Descriptors, which only support Write Request.
>
> I'm not against this patch but from my point of view, I think this is
> a little obscure behaviour wich would deserve an advice coment in
> gatt.h due it isn't clear for anybody developping over GATT to guess
> it. On the other hand, _I remember another functions in gatt.h wich
> have similar double behaviour depending on the parameters provided in
> there, suach as gatt_discovery_primary, in wich a discover ALL primary
> service is done if the uuid is equal to NULL or disvovery service by
> UUID is done if uuid is provided, I think gatt_discover_char works in
> the same way. Maybe that having a good comments here could help to
> avoid this kind of errors in the future.

I agree this API can be improved. Feel free to send patches either
splitting them into separate functions, or properly documenting this
behavior. I think at this point there is no problem in changing this
GATT API, but sending as RFC would not hurt :)

Either way, the current upstream code will send invalid ATT
operations, so the fix is still appropriate.

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

[PATCH v2 BlueZ 1/3] thermometer: Fix device driver probe

[Anderson Lizardo]

From: Bruna Moreira <bruna.moreira@openbossa.org>

The probe() function for GATT profiles should be transport agnostic.
There is a btd_device_get_primaries() method which can be used to return
a list of discovered GATT Primary Services, and it works for BR/EDR and
LE. It is already used for Proximity, for instance.

device_services_from_record() is BR/EDR specific and should not be used
by GATT profile code.

It also fixes a memory leak, given device_services_from_record() returns
a heap allocated GSList.
---
 thermometer/manager.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/thermometer/manager.c b/thermometer/manager.c
index 6b98bca..9dfbbe8 100644
--- a/thermometer/manager.c
+++ b/thermometer/manager.c
@@ -34,16 +34,27 @@
 
 static DBusConnection *connection = NULL;
 
+static gint primary_uuid_cmp(gconstpointer a, gconstpointer b)
+{
+	const struct att_primary *prim = a;
+	const char *uuid = b;
+
+	return g_strcmp0(prim->uuid, uuid);
+}
+
 static int thermometer_driver_probe(struct btd_device *device, GSList *uuids)
 {
 	struct att_primary *tattr;
-	GSList *list;
+	GSList *primaries, *l;
+
+	primaries = btd_device_get_primaries(device);
 
-	list = device_services_from_record(device, uuids);
-	if (list == NULL)
+	l = g_slist_find_custom(primaries, HEALTH_THERMOMETER_UUID,
+							primary_uuid_cmp);
+	if (l == NULL)
 		return -EINVAL;
 
-	tattr = list->data;
+	tattr = l->data;
 
 	return thermometer_register(connection, device, tattr);
 }
-- 
1.7.5.4

[PATCH v2 BlueZ 2/3] thermometer: Fix handling of missing Temperature Type

[Anderson Lizardo]

According to HTS 1.0 (Table 3.1, and Sections 3.1.1.4 and 3.2), the
Temperature Type Characteristic is optional. The only restriction is
that it shall not be present if the Temperature Type is non-static, for
which case the "Temperature Type" field shall be present on the
Temperature Measurement Characteristic value.

Given there is no default value specified when the Temperature Type is
static and unknown, the "Type" entry for MeasurementReceived() on the
Thermometer API was made optional.
---
 doc/thermometer-api.txt   |    3 ++-
 test/test-thermometer     |    3 ++-
 thermometer/thermometer.c |   11 +++--------
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/doc/thermometer-api.txt b/doc/thermometer-api.txt
index 6392d39..2271270 100644
--- a/doc/thermometer-api.txt
+++ b/doc/thermometer-api.txt
@@ -103,7 +103,8 @@ Methods		void MeasurementReceived(dict measure)
 			data. The time value is expressed in seconds since epoch.
 			The value represented is (mantissa) x (10**exponent)
 			See foot note for special values when treating with
-			health devices.
+			health devices. The Type entry is only present if the
+			measurement type is known. Otherwise, it is undefined.
 
 			Dict is defined as below:
 			{
diff --git a/test/test-thermometer b/test/test-thermometer
index 5f9742a..bf3b83e 100755
--- a/test/test-thermometer
+++ b/test/test-thermometer
@@ -24,7 +24,8 @@ class Watcher(dbus.service.Object):
 		if measure.has_key("Time"):
 			print "Time: ", measure["Time"]
 
-		print "Type: ", measure["Type"]
+		if measure.has_key("Type"):
+			print "Type: ", measure["Type"]
 
 def property_changed(name, value):
 
diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index 9bf9881..7aa621f 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -1086,15 +1086,10 @@ static void proc_measurement(struct thermometer *t, const uint8_t *pdu,
 		type = temptype2str(pdu[index]);
 	} else if (t->has_type)
 		type = temptype2str(t->type);
-	else {
-		DBG("Can't get temperature type");
-		return;
-	}
-
-	if (type == NULL)
-		return;
+	else
+		type = NULL;
 
-	m.type = g_strdup(type);
+	m.type = type ? g_strdup(type) : NULL;
 	m.value = final ? "Final" : "Intermediate";
 
 	recv_measurement(t, &m);
-- 
1.7.5.4

[PATCH v2 BlueZ 3/3] thermometer: Fix re-enabling notification/indication on reconnection

[Anderson Lizardo]

If a result callback is not specified for gatt_write_char() it is
assumed that Write Command will be used. This is not valid for
Characteristic Descriptors, which only support Write Request.
---
 thermometer/thermometer.c |   35 +++++++++++++++++++++--------------
 1 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index 7aa621f..3cc2454 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -331,6 +331,17 @@ static void valid_range_desc_cb(guint8 status, const guint8 *pdu, guint16 len,
 	change_property(desc->ch->t, "Minimum", &min);
 }
 
+static void measurement_cb(guint8 status, const guint8 *pdu,
+						guint16 len, gpointer user_data)
+{
+	char *msg = user_data;
+
+	if (status != 0)
+		error("%s failed", msg);
+
+	g_free(msg);
+}
+
 static void process_thermometer_desc(struct descriptor *desc)
 {
 	struct characteristic *ch = desc->ch;
@@ -342,6 +353,7 @@ static void process_thermometer_desc(struct descriptor *desc)
 	if (bt_uuid_cmp(&desc->uuid, &btuuid) == 0) {
 		uint8_t atval[2];
 		uint16_t val;
+		char *msg;
 
 		if (g_strcmp0(ch->attr.uuid,
 					TEMPERATURE_MEASUREMENT_UUID) == 0) {
@@ -349,21 +361,27 @@ static void process_thermometer_desc(struct descriptor *desc)
 				return;
 
 			val = ATT_CLIENT_CHAR_CONF_INDICATION;
+			msg = g_strdup("Enable Temperature Measurement "
+								"indication");
 		} else if (g_strcmp0(ch->attr.uuid,
 					INTERMEDIATE_TEMPERATURE_UUID) == 0) {
 			if (g_slist_length(ch->t->iwatchers) == 0)
 				return;
 
 			val = ATT_CLIENT_CHAR_CONF_NOTIFICATION;
+			msg = g_strdup("Enable Intermediate Temperature "
+								"notification");
 		} else if (g_strcmp0(ch->attr.uuid,
-					MEASUREMENT_INTERVAL_UUID) == 0)
+					MEASUREMENT_INTERVAL_UUID) == 0) {
 			val = ATT_CLIENT_CHAR_CONF_INDICATION;
-		else
+			msg = g_strdup("Enable Measurement Interval "
+								"indication");
+		} else
 			goto done;
 
 		att_put_u16(val, atval);
 		gatt_write_char(ch->t->attrib, desc->handle, atval, 2,
-								NULL, NULL);
+							measurement_cb, msg);
 		return;
 	}
 
@@ -655,17 +673,6 @@ static DBusMessage *set_property(DBusConnection *conn, DBusMessage *msg,
 	return write_attr_interval(t, msg, value);
 }
 
-static void measurement_cb(guint8 status, const guint8 *pdu,
-						guint16 len, gpointer user_data)
-{
-	char *msg = user_data;
-
-	if (status != 0)
-		error("%s failed", msg);
-
-	g_free(msg);
-}
-
 static void enable_final_measurement(struct thermometer *t)
 {
 	struct characteristic *ch;
-- 
1.7.5.4

Re: [PATCH v2 BlueZ 1/3] thermometer: Fix device driver probe

[Johan Hedberg]

Hi Lizardo,

On Wed, Feb 15, 2012, Anderson Lizardo wrote:
> The probe() function for GATT profiles should be transport agnostic.
> There is a btd_device_get_primaries() method which can be used to return
> a list of discovered GATT Primary Services, and it works for BR/EDR and
> LE. It is already used for Proximity, for instance.
> 
> device_services_from_record() is BR/EDR specific and should not be used
> by GATT profile code.
> 
> It also fixes a memory leak, given device_services_from_record() returns
> a heap allocated GSList.
> ---
>  thermometer/manager.c |   19 +++++++++++++++----
>  1 files changed, 15 insertions(+), 4 deletions(-)

All three patches have been applied. Thanks.

Johan