Health Thermometer Profile (HTP)

Health Thermometer Profile (HTP)

[Santiago Carot-Nemesio]

This is a new set of patches to enhance thermometer plugin fuctionality.
These patches enable watchers to receiave final/intermediate measurements
from thermometers devices. They also includes latest changes suggested
in the list.

[PATCH 1/6] Manage GATT attribute indications in handle callback.
[PATCH 2/6] Parse final measurement indication
[PATCH 3/6] Add org.bluez.ThermometerWatcher interface to default
[PATCH 4/6] Implement EnableIntermediateMeasurement D-Bus method
[PATCH 5/6] Implement DisableIntermediateMeasurement D-Bus method
[PATCH 6/6] Notify intermediate measurements

[PATCH 1/6] Manage GATT attribute indications in handle callback.

[Santiago Carot-Nemesio]

---
 thermometer/thermometer.c |   51 ++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 50 insertions(+), 1 deletions(-)

diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index 5b0e30a..69ae708 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -163,6 +163,14 @@ static gint cmp_char_uuid(gconstpointer a, gconstpointer b)
 	return g_strcmp0(ch->attr.uuid, uuid);
 }
 
+static gint cmp_char_val_handle(gconstpointer a, gconstpointer b)
+{
+	const struct characteristic *ch = a;
+	const uint16_t *handle = b;
+
+	return ch->attr.value_handle - *handle;
+}
+
 static gint cmp_descriptor(gconstpointer a, gconstpointer b)
 {
 	const struct descriptor *desc = a;
@@ -703,9 +711,50 @@ static GDBusSignalTable thermometer_signals[] = {
 	{ }
 };
 
+static void proc_measurement(struct thermometer *t, const uint8_t *pdu,
+						uint16_t len, gboolean final)
+{
+	DBG("TODO: Process measurement indication");
+}
+
+static void proc_measurement_interval(struct thermometer *t, const uint8_t *pdu,
+								uint16_t len)
+{
+	DBG("TODO: Process measurements interval indication");
+}
+
 static void ind_handler(const uint8_t *pdu, uint16_t len, gpointer user_data)
 {
-	/* TODO: Process indication */
+	struct thermometer *t = user_data;
+	const struct characteristic *ch;
+	uint8_t opdu[ATT_MAX_MTU];
+	uint16_t handle, olen;
+	GSList *l;
+
+	if (len < 3) {
+		DBG("Bad pdu received");
+		return;
+	}
+
+	handle = att_get_u16(&pdu[1]);
+	l = g_slist_find_custom(t->chars, &handle, cmp_char_val_handle);
+	if (l == NULL) {
+		DBG("Unexpected handle: 0x%04x", handle);
+		return;
+	}
+
+	ch = l->data;
+
+	if (g_strcmp0(ch->attr.uuid, TEMPERATURE_MEASUREMENT_UUID) == 0)
+		proc_measurement(t, pdu, len, TRUE);
+	else if (g_strcmp0(ch->attr.uuid, MEASUREMENT_INTERVAL_UUID) == 0)
+		proc_measurement_interval(t, pdu, len);
+
+	olen = enc_confirmation(opdu, sizeof(opdu));
+
+	if (olen > 0)
+		g_attrib_send(t->attrib, 0, opdu[0], opdu, olen, NULL, NULL,
+									NULL);
 }
 
 static void attio_connected_cb(GAttrib *attrib, gpointer user_data)
-- 
1.7.7.3

[PATCH 2/6] Parse final measurement indication

[Santiago Carot-Nemesio]

---
 thermometer/thermometer.c |  163 ++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 162 insertions(+), 1 deletions(-)

diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index 69ae708..b35c3aa 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -47,6 +47,13 @@
 #define INTERMEDIATE_TEMPERATURE_UUID	"00002a1e-0000-1000-8000-00805f9b34fb"
 #define MEASUREMENT_INTERVAL_UUID	"00002a21-0000-1000-8000-00805f9b34fb"
 
+/* Temperature measurement flag fields */
+#define TEMP_UNITS		0x01
+#define TEMP_TIME_STAMP		0x02
+#define TEMP_TYPE		0x04
+
+#define FLOAT_MAX_MANTISSA	16777216 /* 2^24 */
+
 struct thermometer {
 	DBusConnection		*conn;		/* The connection to the bus */
 	struct btd_device	*dev;		/* Device reference */
@@ -84,8 +91,40 @@ struct watcher {
 	gchar			*path;
 };
 
+struct measurement {
+	gint16		exp;
+	gint32		mant;
+	guint64		time;
+	gboolean	suptime;
+	gchar		*unit;
+	gchar		*type;
+	gchar		*value;
+};
+
 static GSList *thermometers = NULL;
 
+const char *temp_type[] = {
+	"<reserved>",
+	"Armpit",
+	"Body",
+	"Ear",
+	"Finger",
+	"Intestines",
+	"Mouth",
+	"Rectum",
+	"Toe",
+	"Tympanum"
+};
+
+static const gchar *temptype2str(uint8_t value)
+{
+	 if (value > 0 && value < G_N_ELEMENTS(temp_type))
+		return temp_type[value];
+
+	error("Temperature type %d reserved for future use", value);
+	return NULL;
+}
+
 static void destroy_watcher(gpointer user_data)
 {
 	struct watcher *watcher = user_data;
@@ -711,10 +750,132 @@ static GDBusSignalTable thermometer_signals[] = {
 	{ }
 };
 
+static void update_watcher(gpointer data, gpointer user_data)
+{
+	struct watcher *w = data;
+	struct measurement *m = user_data;
+	DBusConnection *conn = w->t->conn;
+	DBusMessageIter iter;
+	DBusMessageIter dict;
+	DBusMessage *msg;
+
+	msg = dbus_message_new_method_call(w->srv, w->path,
+				"org.bluez.ThermometerWatcher",
+				"MeasurementReceived");
+	if (msg == NULL)
+		return;
+
+	dbus_message_iter_init_append(msg, &iter);
+
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
+			DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING
+			DBUS_TYPE_STRING_AS_STRING DBUS_TYPE_VARIANT_AS_STRING
+			DBUS_DICT_ENTRY_END_CHAR_AS_STRING, &dict);
+
+	dict_append_entry(&dict, "Exponent", DBUS_TYPE_INT16, &m->exp);
+	dict_append_entry(&dict, "Mantissa", DBUS_TYPE_INT32, &m->mant);
+	dict_append_entry(&dict, "Unit", DBUS_TYPE_STRING, &m->unit);
+
+	if (m->suptime)
+		dict_append_entry(&dict, "Time", DBUS_TYPE_UINT64, &m->time);
+
+	dict_append_entry(&dict, "Type", DBUS_TYPE_STRING, &m->type);
+	dict_append_entry(&dict, "Measurement", DBUS_TYPE_STRING, &m->value);
+
+	dbus_message_iter_close_container(&iter, &dict);
+
+	dbus_message_set_no_reply(msg, TRUE);
+	g_dbus_send_message(conn, msg);
+}
+
+static void recv_measurement(struct thermometer *t, struct measurement *m)
+{
+	if (g_strcmp0(m->value, "Intermediate") == 0) {
+		DBG("Notification of intermediate measurement not implemented");
+		return;
+	}
+
+	g_slist_foreach(t->fwatchers, update_watcher, m);
+}
+
 static void proc_measurement(struct thermometer *t, const uint8_t *pdu,
 						uint16_t len, gboolean final)
 {
-	DBG("TODO: Process measurement indication");
+	struct measurement m;
+	const gchar *type;
+	uint8_t flags;
+	uint32_t raw;
+
+	if (len < 4) {
+		DBG("Mandatory flags are not provided");
+		return;
+	}
+
+	flags = pdu[3];
+	if (flags & TEMP_UNITS)
+		m.unit = "Fahrenheit";
+	else
+		m.unit = "Celsius";
+
+	if (len < 8) {
+		DBG("Temperature measurement value is not provided");
+		return;
+	}
+
+	raw = att_get_u32(&pdu[4]);
+	m.mant = raw & 0x00FFFFFF;
+	m.exp = ((gint32) raw) >> 24;
+
+	if (m.mant & 0x00800000) {
+		/* convert to C2 negative value */
+		m.mant = m.mant - FLOAT_MAX_MANTISSA;
+	}
+
+	if (flags & TEMP_TIME_STAMP) {
+		struct tm ts;
+		time_t time;
+
+		if (len < 15) {
+			DBG("Can't get time stamp value");
+			return;
+		}
+
+		ts.tm_year = att_get_u16(&pdu[8]) - 1900;
+		ts.tm_mon = pdu[10];
+		ts.tm_mday = pdu[11];
+		ts.tm_hour = pdu[12];
+		ts.tm_min = pdu[13];
+		ts.tm_sec = pdu[14];
+		ts.tm_isdst = -1;
+
+		time = mktime(&ts);
+		m.time = (guint64) time;
+		m.suptime = TRUE;
+	} else
+		m.suptime = FALSE;
+
+	if (flags & TEMP_TYPE) {
+		if (len < 16) {
+			DBG("Can't get temperature type");
+			return;
+		}
+
+		type = temptype2str(pdu[15]);
+	} else if (t->has_type)
+		type = temptype2str(t->type);
+	else {
+		DBG("Can't get temperature type");
+		return;
+	}
+
+	if (type == NULL)
+		return;
+
+	m.type = g_strdup(type);
+	m.value = final ? "Final" : "Intermediate";
+
+	recv_measurement(t, &m);
+	g_free(m.type);
 }
 
 static void proc_measurement_interval(struct thermometer *t, const uint8_t *pdu,
-- 
1.7.7.3

[PATCH 3/6] Add org.bluez.ThermometerWatcher interface to default policy

[Santiago Carot-Nemesio]

---
 src/bluetooth.conf |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/src/bluetooth.conf b/src/bluetooth.conf
index b5a6af3..664dbd9 100644
--- a/src/bluetooth.conf
+++ b/src/bluetooth.conf
@@ -15,6 +15,7 @@
     <allow send_interface="org.bluez.MediaEndpoint"/>
     <allow send_interface="org.bluez.MediaPlayer"/>
     <allow send_interface="org.bluez.Watcher"/>
+    <allow send_interface="org.bluez.ThermometerWatcher"/>
   </policy>
 
   <policy at_console="true">
-- 
1.7.7.3

[PATCH 4/6] Implement EnableIntermediateMeasurement D-Bus method

[Santiago Carot-Nemesio]

---
 thermometer/thermometer.c |   74 +++++++++++++++++++++++++++++++++++++-------
 1 files changed, 62 insertions(+), 12 deletions(-)

diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index b35c3aa..64b6aa6 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -63,6 +63,7 @@ struct thermometer {
 	guint			attindid;	/* Att incications id */
 	GSList			*chars;		/* Characteristics */
 	GSList			*fwatchers;     /* Final measurements */
+	GSList			*iwatchers;     /* Intermediate measurements */
 	gboolean		intermediate;
 	guint8			type;
 	guint16			interval;
@@ -558,7 +559,7 @@ static struct descriptor *get_descriptor(struct characteristic *ch,
 	return l->data;
 }
 
-static void final_measurement_cb(guint8 status, const guint8 *pdu,
+static void measurement_cb(guint8 status, const guint8 *pdu,
 						guint16 len, gpointer user_data)
 {
 	gchar *msg = user_data;
@@ -593,8 +594,34 @@ static void enable_final_measurement(struct thermometer *t)
 	atval[0] = 0x02;
 	atval[1] = 0x00;
 	msg = g_strdup("Enable final measurement");
-	gatt_write_char(t->attrib, desc->handle, atval, 2,
-						final_measurement_cb, msg);
+	gatt_write_char(t->attrib, desc->handle, atval, 2, measurement_cb, msg);
+}
+
+static void enable_intermediate_measurement(struct thermometer *t)
+{
+	struct characteristic *ch;
+	struct descriptor *desc;
+	bt_uuid_t btuuid;
+	uint8_t atval[2];
+	gchar *msg;
+
+	ch = get_characteristic(t, INTERMEDIATE_TEMPERATURE_UUID);
+	if (ch == NULL) {
+		DBG("Intermediate measurement characteristic not found");
+		return;
+	}
+
+	bt_uuid16_create(&btuuid, GATT_CLIENT_CHARAC_CFG_UUID);
+	desc = get_descriptor(ch, &btuuid);
+	if (desc == NULL) {
+		DBG("Client characteristic configuration descriptor not found");
+		return;
+	}
+
+	atval[0] = 0x01;
+	atval[1] = 0x00;
+	msg = g_strdup("Enable intermediate measurement");
+	gatt_write_char(t->attrib, desc->handle, atval, 2, measurement_cb, msg);
 }
 
 static void disable_final_measurement(struct thermometer *t)
@@ -621,8 +648,7 @@ static void disable_final_measurement(struct thermometer *t)
 	atval[0] = 0x00;
 	atval[1] = 0x00;
 	msg = g_strdup("Disable final measurement");
-	gatt_write_char(t->attrib, desc->handle, atval, 2,
-						final_measurement_cb, msg);
+	gatt_write_char(t->attrib, desc->handle, atval, 2, measurement_cb, msg);
 }
 
 static void watcher_exit(DBusConnection *conn, void *user_data)
@@ -639,7 +665,7 @@ static void watcher_exit(DBusConnection *conn, void *user_data)
 		disable_final_measurement(t);
 }
 
-static struct watcher *find_watcher(struct thermometer *t, const gchar *sender,
+static struct watcher *find_watcher(GSList *list, const gchar *sender,
 							const gchar *path)
 {
 	struct watcher *match;
@@ -649,7 +675,7 @@ static struct watcher *find_watcher(struct thermometer *t, const gchar *sender,
 	match->srv = g_strdup(sender);
 	match->path = g_strdup(path);
 
-	l = g_slist_find_custom(t->fwatchers, match, cmp_watcher);
+	l = g_slist_find_custom(list, match, cmp_watcher);
 	destroy_watcher(match);
 
 	if (l != NULL)
@@ -670,7 +696,7 @@ static DBusMessage *register_watcher(DBusConnection *conn, DBusMessage *msg,
 							DBUS_TYPE_INVALID))
 		return btd_error_invalid_args(msg);
 
-	watcher = find_watcher(t, sender, path);
+	watcher = find_watcher(t->fwatchers, sender, path);
 	if (watcher != NULL)
 		return btd_error_already_exists(msg);
 
@@ -703,7 +729,7 @@ static DBusMessage *unregister_watcher(DBusConnection *conn, DBusMessage *msg,
 							DBUS_TYPE_INVALID))
 		return btd_error_invalid_args(msg);
 
-	watcher = find_watcher(t, sender, path);
+	watcher = find_watcher(t->fwatchers, sender, path);
 	if (watcher == NULL)
 		return btd_error_does_not_exist(msg);
 
@@ -721,9 +747,33 @@ static DBusMessage *unregister_watcher(DBusConnection *conn, DBusMessage *msg,
 static DBusMessage *enable_intermediate(DBusConnection *conn, DBusMessage *msg,
 								void *data)
 {
-	/* TODO: */
-	return g_dbus_create_error(msg, ERROR_INTERFACE ".ThermometerError",
-						"Function not implemented.");
+	const gchar *sender = dbus_message_get_sender(msg);
+	struct thermometer *t = data;
+	struct watcher *watcher;
+	gchar *path;
+
+	if (!t->intermediate)
+		return btd_error_not_supported(msg);
+
+	if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_OBJECT_PATH, &path,
+							DBUS_TYPE_INVALID))
+		return btd_error_invalid_args(msg);
+
+	watcher = find_watcher(t->fwatchers, sender, path);
+	if (watcher == NULL)
+		return btd_error_does_not_exist(msg);
+
+	if (find_watcher(t->iwatchers, sender, path))
+		return btd_error_already_exists(msg);
+
+	DBG("Intermediate measurement watcher %s registered", path);
+
+	if (g_slist_length(t->iwatchers) == 0)
+		enable_intermediate_measurement(t);
+
+	t->iwatchers = g_slist_prepend(t->iwatchers, watcher);
+
+	return dbus_message_new_method_return(msg);
 }
 
 static DBusMessage *disable_intermediate(DBusConnection *conn, DBusMessage *msg,
-- 
1.7.7.3

[PATCH 5/6] Implement DisableIntermediateMeasurement D-Bus method

[Santiago Carot-Nemesio]

---
 thermometer/thermometer.c |   63 ++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index 64b6aa6..0e38b19 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -651,6 +651,44 @@ static void disable_final_measurement(struct thermometer *t)
 	gatt_write_char(t->attrib, desc->handle, atval, 2, measurement_cb, msg);
 }
 
+static void disable_intermediate_measurement(struct thermometer *t)
+{
+	struct characteristic *ch;
+	struct descriptor *desc;
+	bt_uuid_t btuuid;
+	uint8_t atval[2];
+	gchar *msg;
+
+	ch = get_characteristic(t, INTERMEDIATE_TEMPERATURE_UUID);
+	if (ch == NULL) {
+		DBG("Intermediate measurement characteristic not found");
+		return;
+	}
+
+	bt_uuid16_create(&btuuid, GATT_CLIENT_CHARAC_CFG_UUID);
+	desc = get_descriptor(ch, &btuuid);
+	if (desc == NULL) {
+		DBG("Client characteristic configuration descriptor not found");
+		return;
+	}
+
+	atval[0] = 0x00;
+	atval[1] = 0x00;
+	msg = g_strdup("Disable intermediate measurement");
+	gatt_write_char(t->attrib, desc->handle, atval, 2, measurement_cb, msg);
+}
+
+static void remove_int_watcher(struct thermometer *t, struct watcher *w)
+{
+	if (!g_slist_find(t->iwatchers, w))
+		return;
+
+	t->iwatchers = g_slist_remove(t->iwatchers, w);
+
+	if (g_slist_length(t->iwatchers) == 0)
+		disable_intermediate_measurement(t);
+}
+
 static void watcher_exit(DBusConnection *conn, void *user_data)
 {
 	struct watcher *watcher = user_data;
@@ -658,6 +696,8 @@ static void watcher_exit(DBusConnection *conn, void *user_data)
 
 	DBG("Thermometer watcher %s disconnected", watcher->path);
 
+	remove_int_watcher(t, watcher);
+
 	t->fwatchers = g_slist_remove(t->fwatchers, watcher);
 	watcher->id = 0;
 
@@ -735,6 +775,8 @@ static DBusMessage *unregister_watcher(DBusConnection *conn, DBusMessage *msg,
 
 	DBG("Thermometer watcher %s unregistered", path);
 
+	remove_int_watcher(t, watcher);
+
 	t->fwatchers = g_slist_remove(t->fwatchers, watcher);
 	destroy_watcher(watcher);
 
@@ -779,9 +821,24 @@ static DBusMessage *enable_intermediate(DBusConnection *conn, DBusMessage *msg,
 static DBusMessage *disable_intermediate(DBusConnection *conn, DBusMessage *msg,
 								void *data)
 {
-	/* TODO: */
-	return g_dbus_create_error(msg, ERROR_INTERFACE ".ThermometerError",
-						"Function not implemented.");
+	const gchar *sender = dbus_message_get_sender(msg);
+	struct thermometer *t = data;
+	struct watcher *watcher;
+	gchar *path;
+
+	if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_OBJECT_PATH, &path,
+							DBUS_TYPE_INVALID))
+		return btd_error_invalid_args(msg);
+
+	watcher = find_watcher(t->iwatchers, sender, path);
+	if (watcher == NULL)
+		return btd_error_does_not_exist(msg);
+
+	DBG("Intermediate measurement %s unregistered", path);
+
+	remove_int_watcher(t, watcher);
+
+	return dbus_message_new_method_return(msg);
 }
 
 static GDBusMethodTable thermometer_methods[] = {
-- 
1.7.7.3

[PATCH 6/6] Notify intermediate measurements

[Santiago Carot-Nemesio]

---
 thermometer/thermometer.c |   47 ++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index 0e38b19..db33cdb 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -61,6 +61,7 @@ struct thermometer {
 	struct att_range	*svc_range;	/* Thermometer range */
 	guint			attioid;	/* Att watcher id */
 	guint			attindid;	/* Att incications id */
+	guint			attnotid;	/* Att notifications id */
 	GSList			*chars;		/* Characteristics */
 	GSList			*fwatchers;     /* Final measurements */
 	GSList			*iwatchers;     /* Intermediate measurements */
@@ -156,6 +157,9 @@ static void destroy_thermometer(gpointer user_data)
 	if (t->attindid > 0)
 		g_attrib_unregister(t->attrib, t->attindid);
 
+	if (t->attnotid > 0)
+		g_attrib_unregister(t->attrib, t->attnotid);
+
 	if (t->attrib != NULL)
 		g_attrib_unref(t->attrib);
 
@@ -897,12 +901,14 @@ static void update_watcher(gpointer data, gpointer user_data)
 
 static void recv_measurement(struct thermometer *t, struct measurement *m)
 {
-	if (g_strcmp0(m->value, "Intermediate") == 0) {
-		DBG("Notification of intermediate measurement not implemented");
-		return;
-	}
+	GSList *wlist;
 
-	g_slist_foreach(t->fwatchers, update_watcher, m);
+	if (g_strcmp0(m->value, "Intermediate") == 0)
+		wlist = t->iwatchers;
+	else
+		wlist = t->fwatchers;
+
+	g_slist_foreach(wlist, update_watcher, m);
 }
 
 static void proc_measurement(struct thermometer *t, const uint8_t *pdu,
@@ -1025,6 +1031,30 @@ static void ind_handler(const uint8_t *pdu, uint16_t len, gpointer user_data)
 									NULL);
 }
 
+static void notif_handler(const uint8_t *pdu, uint16_t len, gpointer user_data)
+{
+	struct thermometer *t = user_data;
+	const struct characteristic *ch;
+	uint16_t handle;
+	GSList *l;
+
+	if (len < 3) {
+		DBG("Bad pdu received");
+		return;
+	}
+
+	handle = att_get_u16(&pdu[1]);
+	l = g_slist_find_custom(t->chars, &handle, cmp_char_val_handle);
+	if (l == NULL) {
+		DBG("Unexpected handle: 0x%04x", handle);
+		return;
+	}
+
+	ch = l->data;
+	if (g_strcmp0(ch->attr.uuid, INTERMEDIATE_TEMPERATURE_UUID) == 0)
+		proc_measurement(t, pdu, len, FALSE);
+}
+
 static void attio_connected_cb(GAttrib *attrib, gpointer user_data)
 {
 	struct thermometer *t = user_data;
@@ -1033,6 +1063,8 @@ static void attio_connected_cb(GAttrib *attrib, gpointer user_data)
 
 	t->attindid = g_attrib_register(t->attrib, ATT_OP_HANDLE_IND,
 							ind_handler, t, NULL);
+	t->attnotid = g_attrib_register(t->attrib, ATT_OP_HANDLE_NOTIFY,
+							notif_handler, t, NULL);
 	gatt_discover_char(t->attrib, t->svc_range->start, t->svc_range->end,
 					NULL, configure_thermometer_cb, t);
 }
@@ -1048,6 +1080,11 @@ static void attio_disconnected_cb(gpointer user_data)
 		t->attindid = 0;
 	}
 
+	if (t->attnotid > 0) {
+		g_attrib_unregister(t->attrib, t->attnotid);
+		t->attnotid = 0;
+	}
+
 	g_attrib_unref(t->attrib);
 	t->attrib = NULL;
 }
-- 
1.7.7.3

Re: [PATCH 2/6] Parse final measurement indication

[Johan Hedberg]

Hi Santiago,

On Mon, Nov 14, 2011, Santiago Carot-Nemesio wrote:
> +	dict_append_entry(&dict, "Exponent", DBUS_TYPE_INT16, &m->exp);
> +	dict_append_entry(&dict, "Mantissa", DBUS_TYPE_INT32, &m->mant);

Could you remind me why we have this kind of encoding in the D-Bus
interface instead of using the native DBUS_TYPE_DOUBLE? Using
DBUS_TYPE_DOUBLE seems like the obvious & clean approach from an API
standpoint so there should be good reasons for not using it.

Johan

Re: [PATCH 2/6] Parse final measurement indication

[Santiago Carot]

Hi Johan,

2011/11/14 Johan Hedberg <johan.hedberg@gmail.com>:
> Hi Santiago,
>
> On Mon, Nov 14, 2011, Santiago Carot-Nemesio wrote:
>> +     dict_append_entry(&dict, "Exponent", DBUS_TYPE_INT16, &m->exp);
>> +     dict_append_entry(&dict, "Mantissa", DBUS_TYPE_INT32, &m->mant);
>
> Could you remind me why we have this kind of encoding in the D-Bus
> interface instead of using the native DBUS_TYPE_DOUBLE? Using
> DBUS_TYPE_DOUBLE seems like the obvious & clean approach from an API
> standpoint so there should be good reasons for not using it.
>

As far as I remember, the reason why we did that in this way is
because health devices through GATT use IEEE-11073 32-bit FLOAT types,
so in order to keep the thermometer transcodable[1] through the API we
thought the esier way could be by providing its value representation
in their exponent/magnitude components.  IEEE-11073 32-bit FLOAT type
has special values to indicate special circumstances so we thought
that it could be better for applications to provide the exp/mantissa
values to make easier for them to detect this kind of issues instead
of comparing floating point numbers directly, wich is more tricky.
Furthermomre, most of the transcoder applications will need to know
expnonet and mantissa values to generate APDUs in an
ISO/IEEE-11073-20601 eco-system, so we thought that it would be easier
for both non-IEEE11073 and transcoder applications to get the FLOAT
value through their exp/mantissa represenation rather than to get
exp/mantissa values from the FLOAT value directly.

Those are some of the reason we were talking about... of course,
suggestion are always welcome.

[1] Personal Health Devices Transcoding Whitepaper
https://www.bluetooth.org/docman/handlers/downloaddoc.ashx?doc_id=238300

Regards.
Santiago

Re: Health Thermometer Profile (HTP)

[Johan Hedberg]

Hi Santiago,

On Mon, Nov 14, 2011, Santiago Carot-Nemesio wrote:
> This is a new set of patches to enhance thermometer plugin fuctionality.
> These patches enable watchers to receiave final/intermediate measurements
> from thermometers devices. They also includes latest changes suggested
> in the list.
> 
> [PATCH 1/6] Manage GATT attribute indications in handle callback.
> [PATCH 2/6] Parse final measurement indication
> [PATCH 3/6] Add org.bluez.ThermometerWatcher interface to default
> [PATCH 4/6] Implement EnableIntermediateMeasurement D-Bus method
> [PATCH 5/6] Implement DisableIntermediateMeasurement D-Bus method
> [PATCH 6/6] Notify intermediate measurements

All of these patches have been pushed upstream. Thanks.

Johan

Health Thermometer Profile (HTP)

[Santiago Carot-Nemesio]

This is a new set of patches to enhance thermometer plugin fuctionality.
These patches enable watchers to receiave final/intermediate measurements
from thermometers devices. They also includes latest changes suggested
in the list.

[PATCH 1/6] Manage GATT attribute indications in handle callback.
[PATCH 2/6] Parse final measurement indication
[PATCH 3/6] Add org.bluez.ThermometerWatcher interface to default
[PATCH 4/6] Implement EnableIntermediateMeasurement D-Bus method
[PATCH 5/6] Implement DisableIntermediateMeasurement D-Bus method
[PATCH 6/6] Notify intermediate measurements

[PATCH 1/6] Manage GATT attribute indications in handle callback.

[Santiago Carot-Nemesio]

---
 thermometer/thermometer.c |   51 ++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 50 insertions(+), 1 deletions(-)

diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index 5b0e30a..69ae708 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -163,6 +163,14 @@ static gint cmp_char_uuid(gconstpointer a, gconstpointer b)
 	return g_strcmp0(ch->attr.uuid, uuid);
 }
 
+static gint cmp_char_val_handle(gconstpointer a, gconstpointer b)
+{
+	const struct characteristic *ch = a;
+	const uint16_t *handle = b;
+
+	return ch->attr.value_handle - *handle;
+}
+
 static gint cmp_descriptor(gconstpointer a, gconstpointer b)
 {
 	const struct descriptor *desc = a;
@@ -703,9 +711,50 @@ static GDBusSignalTable thermometer_signals[] = {
 	{ }
 };
 
+static void proc_measurement(struct thermometer *t, const uint8_t *pdu,
+						uint16_t len, gboolean final)
+{
+	DBG("TODO: Process measurement indication");
+}
+
+static void proc_measurement_interval(struct thermometer *t, const uint8_t *pdu,
+								uint16_t len)
+{
+	DBG("TODO: Process measurements interval indication");
+}
+
 static void ind_handler(const uint8_t *pdu, uint16_t len, gpointer user_data)
 {
-	/* TODO: Process indication */
+	struct thermometer *t = user_data;
+	const struct characteristic *ch;
+	uint8_t opdu[ATT_MAX_MTU];
+	uint16_t handle, olen;
+	GSList *l;
+
+	if (len < 3) {
+		DBG("Bad pdu received");
+		return;
+	}
+
+	handle = att_get_u16(&pdu[1]);
+	l = g_slist_find_custom(t->chars, &handle, cmp_char_val_handle);
+	if (l == NULL) {
+		DBG("Unexpected handle: 0x%04x", handle);
+		return;
+	}
+
+	ch = l->data;
+
+	if (g_strcmp0(ch->attr.uuid, TEMPERATURE_MEASUREMENT_UUID) == 0)
+		proc_measurement(t, pdu, len, TRUE);
+	else if (g_strcmp0(ch->attr.uuid, MEASUREMENT_INTERVAL_UUID) == 0)
+		proc_measurement_interval(t, pdu, len);
+
+	olen = enc_confirmation(opdu, sizeof(opdu));
+
+	if (olen > 0)
+		g_attrib_send(t->attrib, 0, opdu[0], opdu, olen, NULL, NULL,
+									NULL);
 }
 
 static void attio_connected_cb(GAttrib *attrib, gpointer user_data)
-- 
1.7.7.3

Health Thermometer Profile (HTP)

[Santiago Carot-Nemesio]

This is a new set of patches to enhance thermometer plugin fuctionality.
These patches enable watchers to receiave final/intermediate measurements
from thermometers devices.

[PATCH 1/6] Manage GATT attribute indications in handle callback.
[PATCH 2/6] Parse final measurement indication
[PATCH 3/6] Add org.bluez.ThermometerWatcher interface to default
[PATCH 4/6] Implement EnableIntermediateMeasurement D-Bus method
[PATCH 5/6] Implement DisableIntermediateMeasurement D-Bus method
[PATCH 6/6] Notify intermediate measurements

[PATCH 1/6] Manage GATT attribute indications in handle callback.

[Santiago Carot-Nemesio]

---
 thermometer/thermometer.c |   57 ++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 56 insertions(+), 1 deletions(-)

diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index 5b0e30a..0928f15 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -163,6 +163,17 @@ static gint cmp_char_uuid(gconstpointer a, gconstpointer b)
 	return g_strcmp0(ch->attr.uuid, uuid);
 }
 
+static gint cmp_char_val_handle(gconstpointer a, gconstpointer b)
+{
+	const struct characteristic *ch = a;
+	const uint16_t *handle = b;
+
+	if (ch->attr.value_handle == *handle)
+		return 0;
+
+	return -1;
+}
+
 static gint cmp_descriptor(gconstpointer a, gconstpointer b)
 {
 	const struct descriptor *desc = a;
@@ -703,9 +714,53 @@ static GDBusSignalTable thermometer_signals[] = {
 	{ }
 };
 
+static void proc_measurement(struct thermometer *t, const uint8_t *pdu,
+						uint16_t len, gboolean final)
+{
+	DBG("TODO: Process measurement indication");
+}
+
+static void proc_measurement_interval(struct thermometer *t, const uint8_t *pdu,
+								uint16_t len)
+{
+	DBG("TODO: Process measurements interval indication");
+}
+
 static void ind_handler(const uint8_t *pdu, uint16_t len, gpointer user_data)
 {
-	/* TODO: Process indication */
+	struct thermometer *t = user_data;
+	const struct characteristic *ch;
+	uint8_t opdu[ATT_MAX_MTU];
+	uint16_t handle, olen;
+	GSList *l;
+
+	if (len < 3) {
+		DBG("Bad pdu received");
+		goto done;
+	}
+
+	if (pdu[0] != ATT_OP_HANDLE_IND) {
+		DBG("Not indication received");
+		return;
+	}
+
+	handle = att_get_u16(&pdu[1]);
+	l = g_slist_find_custom(t->chars, &handle, cmp_char_val_handle);
+	if (l == NULL)
+		goto done;
+
+	ch = l->data;
+	if (g_strcmp0(ch->attr.uuid, TEMPERATURE_MEASUREMENT_UUID) == 0)
+		proc_measurement(t, pdu, len, TRUE);
+	else if (g_strcmp0(ch->attr.uuid, MEASUREMENT_INTERVAL_UUID) == 0)
+		proc_measurement_interval(t, pdu, len);
+
+done:
+	olen = enc_confirmation(opdu, sizeof(opdu));
+
+	if (olen > 0)
+		g_attrib_send(t->attrib, 0, opdu[0], opdu, olen, NULL, NULL,
+									NULL);
 }
 
 static void attio_connected_cb(GAttrib *attrib, gpointer user_data)
-- 
1.7.7.2

Re: [PATCH 1/6] Manage GATT attribute indications in handle callback.

[Anderson Lizardo]

Hi Santiago,

On Wed, Nov 9, 2011 at 6:51 AM, Santiago Carot-Nemesio
<sancane@gmail.com> wrote:
> +static gint cmp_char_val_handle(gconstpointer a, gconstpointer b)
> +{
> +       const struct characteristic *ch = a;
> +       const uint16_t *handle = b;
> +
> +       if (ch->attr.value_handle == *handle)
> +               return 0;
> +
> +       return -1;
> +}

Usually we implement the function above as:

return ch->attr.value_handle - *handle;

It will work exactly as your code.

>  static void ind_handler(const uint8_t *pdu, uint16_t len, gpointer user_data)
>  {
> -       /* TODO: Process indication */
> +       struct thermometer *t = user_data;
> +       const struct characteristic *ch;
> +       uint8_t opdu[ATT_MAX_MTU];
> +       uint16_t handle, olen;
> +       GSList *l;
> +
> +       if (len < 3) {
> +               DBG("Bad pdu received");
> +               goto done;
> +       }

Are you sure we should send a confirmation even if the PDU received is
invalid? What if it is not even an indication (you only check below)?

> +
> +       if (pdu[0] != ATT_OP_HANDLE_IND) {
> +               DBG("Not indication received");
> +               return;
> +       }

I suggest a more descriptive debug message here, something like:

DBG("Unexpected ATT opcode: 0x%02x", pdu[0]);

> +
> +       handle = att_get_u16(&pdu[1]);
> +       l = g_slist_find_custom(t->chars, &handle, cmp_char_val_handle);
> +       if (l == NULL)
> +               goto done;

Again, should we send a confirmation to the thermometer even if the
indication's handle does not match any characteristic value?

(note I didn't read the thermometer profile spec carefully yet. Not
sure if it covers invalid PDUs.)

> +
> +       ch = l->data;
> +       if (g_strcmp0(ch->attr.uuid, TEMPERATURE_MEASUREMENT_UUID) == 0)
> +               proc_measurement(t, pdu, len, TRUE);
> +       else if (g_strcmp0(ch->attr.uuid, MEASUREMENT_INTERVAL_UUID) == 0)
> +               proc_measurement_interval(t, pdu, len);
> +
> +done:
> +       olen = enc_confirmation(opdu, sizeof(opdu));
> +
> +       if (olen > 0)
> +               g_attrib_send(t->attrib, 0, opdu[0], opdu, olen, NULL, NULL,
> +                                                                       NULL);
>  }

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

Re: [PATCH 1/6] Manage GATT attribute indications in handle callback.

[Hendrik Sattler]

Am 09.11.2011 13:00, schrieb Anderson Lizardo:
> Hi Santiago,
>
> On Wed, Nov 9, 2011 at 6:51 AM, Santiago Carot-Nemesio
> <sancane@gmail.com> wrote:
>> +static gint cmp_char_val_handle(gconstpointer a, gconstpointer b)
>> +{
>> +       const struct characteristic *ch = a;
>> +       const uint16_t *handle = b;
>> +
>> +       if (ch->attr.value_handle == *handle)
>> +               return 0;
>> +
>> +       return -1;
>> +}
>
> Usually we implement the function above as:
>
> return ch->attr.value_handle - *handle;
>
> It will work exactly as your code.

You can do it this way with signed integers but not with unsigned 
integers, unless you cast both to signed first.
Still, the above code completely misses the +1 case.

HS

Re: [PATCH 1/6] Manage GATT attribute indications in handle callback.

[Anderson Lizardo]

Hi Hendrik,

On Wed, Nov 9, 2011 at 8:54 AM, Hendrik Sattler <post@hendrik-sattler.de> wrote:
> Am 09.11.2011 13:00, schrieb Anderson Lizardo:
>>
>> Hi Santiago,
>>
>> On Wed, Nov 9, 2011 at 6:51 AM, Santiago Carot-Nemesio
>> <sancane@gmail.com> wrote:
>>>
>>> +static gint cmp_char_val_handle(gconstpointer a, gconstpointer b)
>>> +{
>>> +       const struct characteristic *ch = a;
>>> +       const uint16_t *handle = b;
>>> +
>>> +       if (ch->attr.value_handle == *handle)
>>> +               return 0;
>>> +
>>> +       return -1;
>>> +}
>>
>> Usually we implement the function above as:
>>
>> return ch->attr.value_handle - *handle;
>>
>> It will work exactly as your code.
>
> You can do it this way with signed integers but not with unsigned integers,
> unless you cast both to signed first.
> Still, the above code completely misses the +1 case.

The idea here is to return 0 if they are same or non zero if they are
not same. The actual sign is not relevant because the comparison is
for finding items, not sorting them (in this case).

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

Re: [PATCH 1/6] Manage GATT attribute indications in handle callback.

[Hendrik Sattler]

Am 09.11.2011 14:30, schrieb Anderson Lizardo:
> Hi Hendrik,
>
> On Wed, Nov 9, 2011 at 8:54 AM, Hendrik Sattler
> <post@hendrik-sattler.de> wrote:
>> Am 09.11.2011 13:00, schrieb Anderson Lizardo:
>>>
>>> Hi Santiago,
>>>
>>> On Wed, Nov 9, 2011 at 6:51 AM, Santiago Carot-Nemesio
>>> <sancane@gmail.com> wrote:
>>>>
>>>> +static gint cmp_char_val_handle(gconstpointer a, gconstpointer b)
>>>> +{
>>>> +       const struct characteristic *ch = a;
>>>> +       const uint16_t *handle = b;
>>>> +
>>>> +       if (ch->attr.value_handle == *handle)
>>>> +               return 0;
>>>> +
>>>> +       return -1;
>>>> +}
>>>
>>> Usually we implement the function above as:
>>>
>>> return ch->attr.value_handle - *handle;
>>>
>>> It will work exactly as your code.
>>
>> You can do it this way with signed integers but not with unsigned 
>> integers,
>> unless you cast both to signed first.
>> Still, the above code completely misses the +1 case.
>
> The idea here is to return 0 if they are same or non zero if they are
> not same. The actual sign is not relevant because the comparison is
> for finding items, not sorting them (in this case).

Kind of a lazy approach, no?
Ah well...

HS

Re: [PATCH 1/6] Manage GATT attribute indications in handle callback.

[Anderson Lizardo]

Hi Hendrik,

On Wed, Nov 9, 2011 at 9:53 AM, Hendrik Sattler <post@hendrik-sattler.de> wrote:
>> The idea here is to return 0 if they are same or non zero if they are
>> not same. The actual sign is not relevant because the comparison is
>> for finding items, not sorting them (in this case).
>
> Kind of a lazy approach, no?
> Ah well...

I agree with you, and maybe it is even risky if soneone reuses these
functions for list sorting as well without extending them, but this is
the current procedure on BlueZ as a whole:

$ grep -R 'return.*handle - ' src/ attrib/
src/sdpd-database.c:	return rec1->handle - rec2->handle;
src/sdpd-database.c:	return rec1->handle - rec2->handle;
src/device.c:	return r1->handle - r2->handle;
src/attrib-server.c:	return attrib->handle - handle;
src/attrib-server.c:	return attrib1->handle - attrib2->handle;
attrib/client.c:	return chr->handle - handle;

(all these handles are unsigned.)

Note that I'm not defending the current usage, but then it would need
be "fixed" in a separate commit to fix all cases and maintain
consistency.

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

Re: [PATCH 1/6] Manage GATT attribute indications in handle callback.

[Santiago Carot]

Hi Anderson,

Please, see coments below,

2011/11/9 Anderson Lizardo <anderson.lizardo@openbossa.org>:
> Hi Santiago,
>
> On Wed, Nov 9, 2011 at 6:51 AM, Santiago Carot-Nemesio
> <sancane@gmail.com> wrote:
>> +static gint cmp_char_val_handle(gconstpointer a, gconstpointer b)
>> +{
>> +       const struct characteristic *ch = a;
>> +       const uint16_t *handle = b;
>> +
>> +       if (ch->attr.value_handle == *handle)
>> +               return 0;
>> +
>> +       return -1;
>> +}
>
> Usually we implement the function above as:
>
> return ch->attr.value_handle - *handle;
>
> It will work exactly as your code.
>

ok, that's only a nitpick.


>>  static void ind_handler(const uint8_t *pdu, uint16_t len, gpointer user_data)
>>  {
>> -       /* TODO: Process indication */
>> +       struct thermometer *t = user_data;
>> +       const struct characteristic *ch;
>> +       uint8_t opdu[ATT_MAX_MTU];
>> +       uint16_t handle, olen;
>> +       GSList *l;
>> +
>> +       if (len < 3) {
>> +               DBG("Bad pdu received");
>> +               goto done;
>> +       }
>
> Are you sure we should send a confirmation even if the PDU received is
> invalid? What if it is not even an indication (you only check below)?

This callback is supposed only to manage indications not notifications:
in the code:
...
g_attrib_register(t->attrib, ATT_OP_HANDLE_IND,
							ind_handler, t, NULL);

Shall I expect another kind of notifications even if I have not
indicated it when I registered the callback?

>
>> +
>> +       if (pdu[0] != ATT_OP_HANDLE_IND) {
>> +               DBG("Not indication received");
>> +               return;
>> +       }
>
> I suggest a more descriptive debug message here, something like:
>
> DBG("Unexpected ATT opcode: 0x%02x", pdu[0]);
>

OK, no problem.

>> +
>> +       handle = att_get_u16(&pdu[1]);
>> +       l = g_slist_find_custom(t->chars, &handle, cmp_char_val_handle);
>> +       if (l == NULL)
>> +               goto done;
>
> Again, should we send a confirmation to the thermometer even if the
> indication's handle does not match any characteristic value?
>
> (note I didn't read the thermometer profile spec carefully yet. Not
> sure if it covers invalid PDUs.)
>

As far as I know, the thermometer spec is not focused in such kind of
GATT details, so I guess it will depend of the implementation.  If we
don't send a reply to that indication the remote side will close the
connection when the timeout expired. So here is the question: what
would we rather do, to keep the connection alive or to let the remote
side to close it?

Waiting comments.

Regards
Santiago

Re: [PATCH 1/6] Manage GATT attribute indications in handle callback.

[Anderson Lizardo]

Hi Santiago,

On Wed, Nov 9, 2011 at 10:44 AM, Santiago Carot <sancane@gmail.com> wrote:
>>>  static void ind_handler(const uint8_t *pdu, uint16_t len, gpointer user_data)
>>>  {
>>> -       /* TODO: Process indication */
>>> +       struct thermometer *t = user_data;
>>> +       const struct characteristic *ch;
>>> +       uint8_t opdu[ATT_MAX_MTU];
>>> +       uint16_t handle, olen;
>>> +       GSList *l;
>>> +
>>> +       if (len < 3) {
>>> +               DBG("Bad pdu received");
>>> +               goto done;
>>> +       }
>>
>> Are you sure we should send a confirmation even if the PDU received is
>> invalid? What if it is not even an indication (you only check below)?
>
> This callback is supposed only to manage indications not notifications:
> in the code:
> ...
> g_attrib_register(t->attrib, ATT_OP_HANDLE_IND,
>                                                        ind_handler, t, NULL);
>
> Shall I expect another kind of notifications even if I have not
> indicated it when I registered the callback?

I only made this comment because you do this check on the function:

+       if (pdu[0] != ATT_OP_HANDLE_IND) {
+               DBG("Not indication received");
+               return;
+       }

I think you can simply drop this check, as you mentioned it will only
be called for indications.

About the PDU size check, I think it is important to have it (to avoid
memory corruption on invalid PDU), but then the peer device is bogus,
so I think you should not send a confirmation for it, because you did
not receive a valid indication in the first place.

>>> +
>>> +       handle = att_get_u16(&pdu[1]);
>>> +       l = g_slist_find_custom(t->chars, &handle, cmp_char_val_handle);
>>> +       if (l == NULL)
>>> +               goto done;
>>
>> Again, should we send a confirmation to the thermometer even if the
>> indication's handle does not match any characteristic value?
>>
>> (note I didn't read the thermometer profile spec carefully yet. Not
>> sure if it covers invalid PDUs.)
>>
>
> As far as I know, the thermometer spec is not focused in such kind of
> GATT details, so I guess it will depend of the implementation.  If we
> don't send a reply to that indication the remote side will close the
> connection when the timeout expired. So here is the question: what
> would we rather do, to keep the connection alive or to let the remote
> side to close it?

I believe that in general, if the peer device sends a bogus indication
(or if the communication channel was corrupted somehow), we should
*not* confirm it. I see confirmations as an "ack" that the indication
was received, processed and validated by BlueZ. If it is somehow
invalid (PDU size incorrect, invalid handle), we should not confirm
it.

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

Re: [PATCH 1/6] Manage GATT attribute indications in handle callback.

[Santiago Carot]

Hi Anderson,

2011/11/9 Anderson Lizardo <anderson.lizardo@openbossa.org>:
> Hi Santiago,
>
> On Wed, Nov 9, 2011 at 10:44 AM, Santiago Carot <sancane@gmail.com> wrote:
>>>>  static void ind_handler(const uint8_t *pdu, uint16_t len, gpointer user_data)
>>>>  {
>>>> -       /* TODO: Process indication */
>>>> +       struct thermometer *t = user_data;
>>>> +       const struct characteristic *ch;
>>>> +       uint8_t opdu[ATT_MAX_MTU];
>>>> +       uint16_t handle, olen;
>>>> +       GSList *l;
>>>> +
>>>> +       if (len < 3) {
>>>> +               DBG("Bad pdu received");
>>>> +               goto done;
>>>> +       }
>>>
>>> Are you sure we should send a confirmation even if the PDU received is
>>> invalid? What if it is not even an indication (you only check below)?
>>
>> This callback is supposed only to manage indications not notifications:
>> in the code:
>> ...
>> g_attrib_register(t->attrib, ATT_OP_HANDLE_IND,
>>                                                        ind_handler, t, NULL);
>>
>> Shall I expect another kind of notifications even if I have not
>> indicated it when I registered the callback?
>
> I only made this comment because you do this check on the function:
>
> +       if (pdu[0] != ATT_OP_HANDLE_IND) {
> +               DBG("Not indication received");
> +               return;
> +       }
>
> I think you can simply drop this check, as you mentioned it will only
> be called for indications.
>
> About the PDU size check, I think it is important to have it (to avoid
> memory corruption on invalid PDU), but then the peer device is bogus,
> so I think you should not send a confirmation for it, because you did
> not receive a valid indication in the first place.
>
>>>> +
>>>> +       handle = att_get_u16(&pdu[1]);
>>>> +       l = g_slist_find_custom(t->chars, &handle, cmp_char_val_handle);
>>>> +       if (l == NULL)
>>>> +               goto done;
>>>
>>> Again, should we send a confirmation to the thermometer even if the
>>> indication's handle does not match any characteristic value?
>>>
>>> (note I didn't read the thermometer profile spec carefully yet. Not
>>> sure if it covers invalid PDUs.)
>>>
>>
>> As far as I know, the thermometer spec is not focused in such kind of
>> GATT details, so I guess it will depend of the implementation.  If we
>> don't send a reply to that indication the remote side will close the
>> connection when the timeout expired. So here is the question: what
>> would we rather do, to keep the connection alive or to let the remote
>> side to close it?
>
> I believe that in general, if the peer device sends a bogus indication
> (or if the communication channel was corrupted somehow), we should
> *not* confirm it. I see confirmations as an "ack" that the indication
> was received, processed and validated by BlueZ. If it is somehow
> invalid (PDU size incorrect, invalid handle), we should not confirm
> it.

It makes sense for me, I'll send a new set of patches with the changes
you have suggested.
Thanks.