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
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
Hi Johan,
2011/11/14 Johan Hedberg <[email protected]>:
> 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
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
---
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
---
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
---
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
---
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
---
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
---
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
Hi Johan,
2011/11/12 Johan Hedberg <[email protected]>:
> Hi Santiago,
>
> On Thu, Nov 10, 2011, Santiago Carot-Nemesio wrote:
>> +struct measurement {
>> + ? ? gint16 ? ? ? ? ?exp;
>> + ? ? gint32 ? ? ? ? ?mant;
>> + ? ? guint64 ? ? ? ? time;
>> + ? ? gboolean ? ? ? ?suptime;
>> + ? ? gchar ? ? ? ? ? *unit;
>> + ? ? gchar ? ? ? ? ? *type;
>> + ? ? gchar ? ? ? ? ? *msmnt;
>
> Is msmnt supposed to be short for measurement? Could you just spell it
> out please since to me that short version is far from obvious. Or would
> "value" be a more suitable name?
>
Yes, that's supposed to be a kind of short for measurement, There's
not problem to me in changing the name. The reason why I set that name
was because I wanted to keep consistancy with the thermomether
documentation in which we called it measurement.
> Also, please avoid usage of g* types where you're not directly accessing
> some GLib API that expects them (even then it's mostly fine to go ahead
> with uint16_t, char, etc). I can see that there are other places in
> thermometer.c using the g types too which should be fixed in a separate
> patch.
>
Ok, I used only glib types because I was confused about when I should
use these or the other types, Having a look at some parts in BlueZ it
seemed a little mess, but now It's clear to me. Thank you for your
explanation,
If you dont mind I would preffer to send a separate patch fixing that
issue at the end of the new set in order to avoid too many conflicts
rebasing in my local tree.
>> +static gchar *temptype2str(uint8_t value)
>> +{
>> + ? ? switch (value) {
>> + ? ? case 1: return "Armpit";
>> + ? ? case 2: return "Body";
>> + ? ? case 3: return "Ear";
>> + ? ? case 4: return "Finger";
>> + ? ? case 5: return "Intestines";
>> + ? ? case 6: return "Mouth";
>> + ? ? case 7: return "Rectum";
>> + ? ? case 8: return "Toe";
>> + ? ? case 9: return "Tympanum";
>> + ? ? default:
>> + ? ? ? ? ? ? error("Temperature type %d reserved for future use", value);
>> + ? ? ? ? ? ? return NULL;
>> + ? ? };
>
> Please follow the coding style with switch statements. All those return
> statements should be on their own line. However, in this case I think
> it'd be just easier to make a lookup table for those values, i.e.
>
> const char *temp_type[] {
> ? ? ? ?"<unknown>",
> ? ? ? ?"Armpit",
> ? ? ? ?"Body",
> ? ? ? ?...
>
> And then in the temptype2str function:
>
> ? ? ? ?if (type > 0 && val < G_N_ELEMENTS(temp_type))
> ? ? ? ? ? ? ? ?return temp_type[type];
>
>
> Johan
>
Ok, I'll redo these patches.
Thanks a lot for your comments.
Santiago
Hi Santiago,
On Thu, Nov 10, 2011, Santiago Carot-Nemesio wrote:
> +struct measurement {
> + gint16 exp;
> + gint32 mant;
> + guint64 time;
> + gboolean suptime;
> + gchar *unit;
> + gchar *type;
> + gchar *msmnt;
Is msmnt supposed to be short for measurement? Could you just spell it
out please since to me that short version is far from obvious. Or would
"value" be a more suitable name?
Also, please avoid usage of g* types where you're not directly accessing
some GLib API that expects them (even then it's mostly fine to go ahead
with uint16_t, char, etc). I can see that there are other places in
thermometer.c using the g types too which should be fixed in a separate
patch.
> +static gchar *temptype2str(uint8_t value)
> +{
> + switch (value) {
> + case 1: return "Armpit";
> + case 2: return "Body";
> + case 3: return "Ear";
> + case 4: return "Finger";
> + case 5: return "Intestines";
> + case 6: return "Mouth";
> + case 7: return "Rectum";
> + case 8: return "Toe";
> + case 9: return "Tympanum";
> + default:
> + error("Temperature type %d reserved for future use", value);
> + return NULL;
> + };
Please follow the coding style with switch statements. All those return
statements should be on their own line. However, in this case I think
it'd be just easier to make a lookup table for those values, i.e.
const char *temp_type[] {
"<unknown>",
"Armpit",
"Body",
...
And then in the temptype2str function:
if (type > 0 && val < G_N_ELEMENTS(temp_type))
return temp_type[type];
Johan
Hi Anderson,
2011/11/9 Anderson Lizardo <[email protected]>:
> Hi Santiago,
>
> On Wed, Nov 9, 2011 at 10:44 AM, Santiago Carot <[email protected]> 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.
Hi Santiago,
On Wed, Nov 9, 2011 at 10:44 AM, Santiago Carot <[email protected]> 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
Hi Anderson,
2011/11/9 Anderson Lizardo <[email protected]>:
> Hi Santiago,
>
> On Wed, Nov 9, 2011 at 6:52 AM, Santiago Carot-Nemesio
> <[email protected]> wrote:
>> ?static void recv_measurement(struct thermometer *t, struct measurement *msmt)
>> ?{
>> - ? ? ? GSList *l;
>> + ? ? ? GSList *l, *ll;
>>
>> - ? ? ? if (g_strcmp0(msmt->msmnt, "Intermediate") == 0) {
>> - ? ? ? ? ? ? ? DBG("Notification of intermediate measurement not implemented");
>> - ? ? ? ? ? ? ? return;
>> - ? ? ? }
>> + ? ? ? if (g_strcmp0(msmt->msmnt, "Intermediate") == 0)
>> + ? ? ? ? ? ? ? ll = t->iwatchers;
>> + ? ? ? else
>> + ? ? ? ? ? ? ? ll = t->fwatchers;
>>
>> - ? ? ? for (l = t->fwatchers; l; l = l->next)
>> + ? ? ? for (l = ll; l; l = l->next)
>> ? ? ? ? ? ? ? ?update_watcher(l->data, msmt);
>> ?}
>
> What about using g_slist_foreach() above? Also "ll" seems too generic,
> what about "wlist".
Ok, I'll prepare a new set of patches, but I would rather wait to get
comments and advices to the question regarding to how we should manage
GATT attribute indications.
Thank you very much for your comments.
Santiago
>
> Regards,
> --
> Anderson Lizardo
> Instituto Nokia de Tecnologia - INdT
> Manaus - Brazil
>
Hi Anderson,
Please, see coments below,
2011/11/9 Anderson Lizardo <[email protected]>:
> Hi Santiago,
>
> On Wed, Nov 9, 2011 at 6:51 AM, Santiago Carot-Nemesio
> <[email protected]> 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
Hi Hendrik,
On Wed, Nov 9, 2011 at 9:53 AM, Hendrik Sattler <[email protected]> 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
Hi Anderson
2011/11/9 Anderson Lizardo <[email protected]>:
> Hi Santiago,
>
> On Wed, Nov 9, 2011 at 6:51 AM, Santiago Carot-Nemesio
> <[email protected]> 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.
>
> Out of curiosity, with this latest series, will we be able to use LE
> Health Thermometers? Or do you still have pending items?
>
Yes, of course, with these set of patches you should able to use a Thermometer.
> It would be nice if you share your TODO list regarding this profile,
> or current limitations (if any), on each series you send.
Ok,
At present, I'm working on this repository:
https://github.com/sancane/BlueZ
There you will able to see the current status of the plugin
implementation, there you can also find some pending patches wich I've
not yet submited to the list and so on.
On the other hand, there is nothing left more in my TODO list than
finishing the HTP implementation :-P
Regards.
>
>> [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
>
> Regards,
> --
> Anderson Lizardo
> Instituto Nokia de Tecnologia - INdT
> Manaus - Brazil
>
Am 09.11.2011 14:30, schrieb Anderson Lizardo:
> Hi Hendrik,
>
> On Wed, Nov 9, 2011 at 8:54 AM, Hendrik Sattler
> <[email protected]> wrote:
>> Am 09.11.2011 13:00, schrieb Anderson Lizardo:
>>>
>>> Hi Santiago,
>>>
>>> On Wed, Nov 9, 2011 at 6:51 AM, Santiago Carot-Nemesio
>>> <[email protected]> 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
Hi Hendrik,
On Wed, Nov 9, 2011 at 8:54 AM, Hendrik Sattler <[email protected]> wrote:
> Am 09.11.2011 13:00, schrieb Anderson Lizardo:
>>
>> Hi Santiago,
>>
>> On Wed, Nov 9, 2011 at 6:51 AM, Santiago Carot-Nemesio
>> <[email protected]> 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
Am 09.11.2011 13:00, schrieb Anderson Lizardo:
> Hi Santiago,
>
> On Wed, Nov 9, 2011 at 6:51 AM, Santiago Carot-Nemesio
> <[email protected]> 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
Hi Santiago,
On Wed, Nov 9, 2011 at 6:51 AM, Santiago Carot-Nemesio
<[email protected]> 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.
Out of curiosity, with this latest series, will we be able to use LE
Health Thermometers? Or do you still have pending items?
It would be nice if you share your TODO list regarding this profile,
or current limitations (if any), on each series you send.
> [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
Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
Hi Santiago,
On Wed, Nov 9, 2011 at 6:52 AM, Santiago Carot-Nemesio
<[email protected]> wrote:
> ?static void recv_measurement(struct thermometer *t, struct measurement *msmt)
> ?{
> - ? ? ? GSList *l;
> + ? ? ? GSList *l, *ll;
>
> - ? ? ? if (g_strcmp0(msmt->msmnt, "Intermediate") == 0) {
> - ? ? ? ? ? ? ? DBG("Notification of intermediate measurement not implemented");
> - ? ? ? ? ? ? ? return;
> - ? ? ? }
> + ? ? ? if (g_strcmp0(msmt->msmnt, "Intermediate") == 0)
> + ? ? ? ? ? ? ? ll = t->iwatchers;
> + ? ? ? else
> + ? ? ? ? ? ? ? ll = t->fwatchers;
>
> - ? ? ? for (l = t->fwatchers; l; l = l->next)
> + ? ? ? for (l = ll; l; l = l->next)
> ? ? ? ? ? ? ? ?update_watcher(l->data, msmt);
> ?}
What about using g_slist_foreach() above? Also "ll" seems too generic,
what about "wlist".
Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
Hi Santiago,
On Wed, Nov 9, 2011 at 6:51 AM, Santiago Carot-Nemesio
<[email protected]> 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