This is a new set of patches for health thermometer profile.
They include all fixes suggested previously.
[PATCH 1/9] Get thermometer service range to load the driver.
[PATCH 2/9] Register Health Thermometer Interface
[PATCH 3/9] Unregister Health Thermometer Interface
[PATCH 4/9] Add functions to manage attio callbacks
[PATCH 5/9] Add handler function to manage GATT indications
[PATCH 6/9] Get all characteristics in thermometer service
[PATCH 7/9] Read temperature type characteristic
[PATCH 8/9] Read measurement interval characteristic
[PATCH 9/9] Set Intermediate property if intermediate temp.
---
thermometer/thermometer.c | 31 ++++++++++++++++++++++++++++++-
1 files changed, 30 insertions(+), 1 deletions(-)
diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index f0b977e..b10d09e 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -47,6 +47,13 @@ struct thermometer {
gint attioid; /* Att watcher id */
gint attindid; /* Att incications id */
GSList *chars; /* Characteristics */
+ gboolean intermediate;
+ guint8 type;
+ guint16 interval;
+ guint16 max;
+ guint16 min;
+ gboolean has_type;
+ gboolean has_interval;
};
struct characteristic {
@@ -113,7 +120,29 @@ static void discover_desc_cb(guint8 status, const guint8 *pdu, guint16 len,
static void read_temp_type_cb(guint8 status, const guint8 *pdu, guint16 len,
gpointer user_data)
{
- /* TODO */
+ struct characteristic *ch = user_data;
+ struct thermometer *t = ch->t;
+ uint8_t value[ATT_MAX_MTU];
+ int vlen;
+
+ if (status != 0) {
+ DBG("Temperature Type value read failed: %s",
+ att_ecode2str(status));
+ return;
+ }
+
+ if (!dec_read_resp(pdu, len, value, &vlen)) {
+ DBG("Protocol error.");
+ return;
+ }
+
+ if (vlen != 1) {
+ DBG("Invalid length for Temperature type");
+ return;
+ }
+
+ t->has_interval = TRUE;
+ t->type = (uint8_t) value[0];
}
static void read_interval_cb(guint8 status, const guint8 *pdu, guint16 len,
--
1.7.6.1
---
thermometer/thermometer.c | 70 ++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 69 insertions(+), 1 deletions(-)
diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index b10d09e..8cfc0e4 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -23,6 +23,7 @@
#include <gdbus.h>
#include <bluetooth/uuid.h>
+#include "dbus-common.h"
#include "adapter.h"
#include "device.h"
#include "error.h"
@@ -111,6 +112,49 @@ static gint cmp_device(gconstpointer a, gconstpointer b)
return -1;
}
+static void change_property(struct thermometer *t, const gchar *name,
+ gpointer value) {
+ if (g_strcmp0(name, "Intermediate") == 0) {
+ gboolean *intermediate = value;
+ if (t->intermediate == *intermediate)
+ return;
+
+ t->intermediate = *intermediate;
+ emit_property_changed(t->conn, device_get_path(t->dev),
+ THERMOMETER_INTERFACE, name,
+ DBUS_TYPE_BOOLEAN, &t->intermediate);
+ } else if (g_strcmp0(name, "Interval") == 0) {
+ uint16_t *interval = value;
+ if (t->has_interval && t->interval == *interval)
+ return;
+
+ t->has_interval = TRUE;
+ t->interval = *interval;
+ emit_property_changed(t->conn, device_get_path(t->dev),
+ THERMOMETER_INTERFACE, name,
+ DBUS_TYPE_UINT16, &t->interval);
+ } else if (g_strcmp0(name, "Maximum") == 0) {
+ uint16_t *max = value;
+ if (t->max == *max)
+ return;
+
+ t->max = *max;
+ emit_property_changed(t->conn, device_get_path(t->dev),
+ THERMOMETER_INTERFACE, name,
+ DBUS_TYPE_UINT16, &t->max);
+ } else if (g_strcmp0(name, "Minimum") == 0) {
+ uint16_t *min = value;
+ if (t->min == *min)
+ return;
+
+ t->min = *min;
+ emit_property_changed(t->conn, device_get_path(t->dev),
+ THERMOMETER_INTERFACE, name,
+ DBUS_TYPE_UINT16, &t->min);
+ } else
+ DBG("%s is not a thermometer property", name);
+}
+
static void discover_desc_cb(guint8 status, const guint8 *pdu, guint16 len,
gpointer user_data)
{
@@ -148,7 +192,31 @@ static void read_temp_type_cb(guint8 status, const guint8 *pdu, guint16 len,
static void read_interval_cb(guint8 status, const guint8 *pdu, guint16 len,
gpointer user_data)
{
- /* TODO */
+ struct characteristic *ch = user_data;
+ uint8_t value[ATT_MAX_MTU];
+ uint16_t *p, interval;
+ int vlen;
+
+ if (status != 0) {
+ DBG("Measurement Interval value read failed: %s",
+ att_ecode2str(status));
+ return;
+ }
+
+ if (!dec_read_resp(pdu, len, value, &vlen)) {
+ DBG("Protocol error\n");
+ return;
+ }
+
+ if (vlen < 2) {
+ DBG("Invalid Interval received");
+ return;
+ }
+
+ p = (uint16_t *) value;
+ interval = btohs(*p);
+
+ change_property(ch->t, "Interval", &interval);
}
static void process_thermometer_char(struct characteristic *ch)
--
1.7.6.1
---
thermometer/thermometer.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index 8cfc0e4..e9e93cf 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -224,7 +224,8 @@ static void process_thermometer_char(struct characteristic *ch)
GAttribResultFunc func;
if (g_strcmp0(ch->attr.uuid, INTERMEDIATE_TEMPERATURE_UUID) == 0) {
- /* TODO: Change intermediate property and emit signal */
+ gboolean intermediate = TRUE;
+ change_property(ch->t, "Intermediate", &intermediate);
return;
} else if (g_strcmp0(ch->attr.uuid, TEMPERATURE_TYPE_UUID) == 0)
func = read_temp_type_cb;
--
1.7.6.1
---
thermometer/thermometer.c | 110 +++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 110 insertions(+), 0 deletions(-)
diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index f8c17c6..f0b977e 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -30,10 +30,15 @@
#include "gattrib.h"
#include "attio.h"
#include "att.h"
+#include "gatt.h"
#include "thermometer.h"
#define THERMOMETER_INTERFACE "org.bluez.Thermometer"
+#define TEMPERATURE_TYPE_UUID "00002a1d-0000-1000-8000-00805f9b34fb"
+#define INTERMEDIATE_TEMPERATURE_UUID "00002a1e-0000-1000-8000-00805f9b34fb"
+#define MEASUREMENT_INTERVAL_UUID "00002a21-0000-1000-8000-00805f9b34fb"
+
struct thermometer {
DBusConnection *conn; /* The connection to the bus */
struct btd_device *dev; /* Device reference */
@@ -41,10 +46,31 @@ struct thermometer {
struct att_range *svc_range; /* Thermometer range */
gint attioid; /* Att watcher id */
gint attindid; /* Att incications id */
+ GSList *chars; /* Characteristics */
+};
+
+struct characteristic {
+ struct att_char attr; /* Characteristic */
+ GSList *desc; /* Descriptors */
+ struct thermometer *t; /* Thermometer where the char belongs */
+};
+
+struct descriptor {
+ struct characteristic *ch;
+ uint16_t handle;
+ bt_uuid_t uuid;
};
static GSList *thermometers = NULL;
+static void destroy_char(gpointer user_data)
+{
+ struct characteristic *c = user_data;
+
+ g_slist_free_full(c->desc, g_free);
+ g_free(c);
+}
+
static void destroy_thermometer(gpointer user_data)
{
struct thermometer *t = user_data;
@@ -58,6 +84,9 @@ static void destroy_thermometer(gpointer user_data)
if (t->attrib)
g_attrib_unref(t->attrib);
+ if (t->chars)
+ g_slist_free_full(t->chars, destroy_char);
+
dbus_connection_unref(t->conn);
btd_device_unref(t->dev);
g_free(t->svc_range);
@@ -75,6 +104,85 @@ static gint cmp_device(gconstpointer a, gconstpointer b)
return -1;
}
+static void discover_desc_cb(guint8 status, const guint8 *pdu, guint16 len,
+ gpointer user_data)
+{
+ /* TODO */
+}
+
+static void read_temp_type_cb(guint8 status, const guint8 *pdu, guint16 len,
+ gpointer user_data)
+{
+ /* TODO */
+}
+
+static void read_interval_cb(guint8 status, const guint8 *pdu, guint16 len,
+ gpointer user_data)
+{
+ /* TODO */
+}
+
+static void process_thermometer_char(struct characteristic *ch)
+{
+ GAttribResultFunc func;
+
+ if (g_strcmp0(ch->attr.uuid, INTERMEDIATE_TEMPERATURE_UUID) == 0) {
+ /* TODO: Change intermediate property and emit signal */
+ return;
+ } else if (g_strcmp0(ch->attr.uuid, TEMPERATURE_TYPE_UUID) == 0)
+ func = read_temp_type_cb;
+ else if (g_strcmp0(ch->attr.uuid, MEASUREMENT_INTERVAL_UUID) == 0)
+ func = read_interval_cb;
+ else
+ return;
+
+ gatt_read_char(ch->t->attrib, ch->attr.value_handle, 0, func, ch);
+}
+
+static void configure_thermometer_cb(GSList *characteristics, guint8 status,
+ gpointer user_data)
+{
+ struct thermometer *t = user_data;
+ GSList *l;
+
+ if (status) {
+ error("Discover thermometer characteristics: %s",
+ att_ecode2str(status));
+ return;
+ }
+
+ for (l = characteristics; l; l = l->next) {
+ struct att_char *c = l->data;
+ struct characteristic *ch;
+ uint16_t start, end;
+
+ ch = g_new0(struct characteristic, 1);
+ ch->attr.handle = c->handle;
+ ch->attr.properties = c->properties;
+ ch->attr.value_handle = c->value_handle;
+ memcpy(ch->attr.uuid, c->uuid, MAX_LEN_UUID_STR + 1);
+ ch->t = t;
+
+ t->chars = g_slist_append(t->chars, ch);
+
+ process_thermometer_char(ch);
+
+ start = c->value_handle + 1;
+
+ if (l->next) {
+ struct att_char *c = l->next->data;
+ if (start == c->handle)
+ continue;
+ end = c->handle - 1;
+ } else if (c->value_handle != t->svc_range->end)
+ end = t->svc_range->end;
+ else
+ continue;
+
+ gatt_find_info(t->attrib, start, end, discover_desc_cb, ch);
+ }
+}
+
static DBusMessage *get_properties(DBusConnection *conn, DBusMessage *msg,
void *data)
{
@@ -152,6 +260,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);
+ gatt_discover_char(t->attrib, t->svc_range->start, t->svc_range->end,
+ NULL, configure_thermometer_cb, t);
}
static void attio_disconnected_cb(gpointer user_data)
--
1.7.6.1
---
thermometer/thermometer.c | 17 +++++++++++++++++
1 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index e06c6da..f8c17c6 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -40,6 +40,7 @@ struct thermometer {
GAttrib *attrib; /* GATT connection */
struct att_range *svc_range; /* Thermometer range */
gint attioid; /* Att watcher id */
+ gint attindid; /* Att incications id */
};
static GSList *thermometers = NULL;
@@ -51,6 +52,9 @@ static void destroy_thermometer(gpointer user_data)
if (t->attioid)
btd_device_remove_attio_callback(t->dev, t->attioid);
+ if (t->attindid)
+ g_attrib_unregister(t->attrib, t->attindid);
+
if (t->attrib)
g_attrib_unref(t->attrib);
@@ -135,11 +139,19 @@ static GDBusSignalTable thermometer_signals[] = {
{ }
};
+static void ind_handler(const uint8_t *pdu, uint16_t len, gpointer user_data)
+{
+ /* TODO: Process indication */
+}
+
static void attio_connected_cb(GAttrib *attrib, gpointer user_data)
{
struct thermometer *t = user_data;
t->attrib = g_attrib_ref(attrib);
+
+ t->attindid = g_attrib_register(t->attrib, ATT_OP_HANDLE_IND,
+ ind_handler, t, NULL);
}
static void attio_disconnected_cb(gpointer user_data)
@@ -148,6 +160,11 @@ static void attio_disconnected_cb(gpointer user_data)
DBG("GATT Disconnected");
+ if (t->attindid) {
+ g_attrib_unregister(t->attrib, t->attindid);
+ t->attindid = 0;
+ }
+
g_attrib_unref(t->attrib);
t->attrib = NULL;
}
--
1.7.6.1
---
thermometer/thermometer.c | 29 +++++++++++++++++++++++++++++
1 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index cdfe455..e06c6da 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -28,6 +28,7 @@
#include "error.h"
#include "log.h"
#include "gattrib.h"
+#include "attio.h"
#include "att.h"
#include "thermometer.h"
@@ -36,7 +37,9 @@
struct thermometer {
DBusConnection *conn; /* The connection to the bus */
struct btd_device *dev; /* Device reference */
+ GAttrib *attrib; /* GATT connection */
struct att_range *svc_range; /* Thermometer range */
+ gint attioid; /* Att watcher id */
};
static GSList *thermometers = NULL;
@@ -45,6 +48,12 @@ static void destroy_thermometer(gpointer user_data)
{
struct thermometer *t = user_data;
+ if (t->attioid)
+ btd_device_remove_attio_callback(t->dev, t->attioid);
+
+ if (t->attrib)
+ g_attrib_unref(t->attrib);
+
dbus_connection_unref(t->conn);
btd_device_unref(t->dev);
g_free(t->svc_range);
@@ -126,6 +135,23 @@ static GDBusSignalTable thermometer_signals[] = {
{ }
};
+static void attio_connected_cb(GAttrib *attrib, gpointer user_data)
+{
+ struct thermometer *t = user_data;
+
+ t->attrib = g_attrib_ref(attrib);
+}
+
+static void attio_disconnected_cb(gpointer user_data)
+{
+ struct thermometer *t = user_data;
+
+ DBG("GATT Disconnected");
+
+ g_attrib_unref(t->attrib);
+ t->attrib = NULL;
+}
+
int thermometer_register(DBusConnection *connection, struct btd_device *device,
struct att_primary *tattr)
{
@@ -149,6 +175,9 @@ int thermometer_register(DBusConnection *connection, struct btd_device *device,
}
thermometers = g_slist_prepend(thermometers, t);
+
+ t->attioid = btd_device_add_attio_callback(device, attio_connected_cb,
+ attio_disconnected_cb, t);
return 0;
}
--
1.7.6.1
---
thermometer/manager.c | 13 ++++++++++++-
thermometer/thermometer.c | 5 ++++-
thermometer/thermometer.h | 3 ++-
3 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/thermometer/manager.c b/thermometer/manager.c
index 08f0e0a..b39094f 100644
--- a/thermometer/manager.c
+++ b/thermometer/manager.c
@@ -21,9 +21,11 @@
*/
#include <gdbus.h>
+#include <bluetooth/uuid.h>
#include "adapter.h"
#include "device.h"
+#include "att.h"
#include "thermometer.h"
#include "manager.h"
@@ -33,7 +35,16 @@ static DBusConnection *connection = NULL;
static int thermometer_driver_probe(struct btd_device *device, GSList *uuids)
{
- return thermometer_register(connection, device);
+ struct att_primary *tattr;
+ GSList *list;
+
+ list = device_services_from_record(device, uuids);
+ if (!list)
+ return -1;
+
+ tattr = list->data;
+
+ return thermometer_register(connection, device, tattr);
}
static void thermometer_driver_remove(struct btd_device *device)
diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index 3cd821a..027ae02 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -21,12 +21,15 @@
*/
#include <gdbus.h>
+#include <bluetooth/uuid.h>
#include "adapter.h"
#include "device.h"
+#include "att.h"
#include "thermometer.h"
-int thermometer_register(DBusConnection *connection, struct btd_device *device)
+int thermometer_register(DBusConnection *connection, struct btd_device *device,
+ struct att_primary *tattr)
{
/* TODO: Register Health Thermometer Interface */
return 0;
diff --git a/thermometer/thermometer.h b/thermometer/thermometer.h
index 0937444..298c9ad 100644
--- a/thermometer/thermometer.h
+++ b/thermometer/thermometer.h
@@ -20,5 +20,6 @@
*
*/
-int thermometer_register(DBusConnection *connection, struct btd_device *device);
+int thermometer_register(DBusConnection *connection, struct btd_device *device,
+ struct att_primary *tattr);
void thermometer_unregister(struct btd_device *device);
--
1.7.6.1
---
thermometer/thermometer.c | 108 ++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 107 insertions(+), 1 deletions(-)
diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index 027ae02..a89d8a9 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -25,13 +25,119 @@
#include "adapter.h"
#include "device.h"
+#include "error.h"
+#include "log.h"
+#include "gattrib.h"
#include "att.h"
#include "thermometer.h"
+#define THERMOMETER_INTERFACE "org.bluez.Thermometer"
+
+struct thermometer {
+ DBusConnection *conn; /* The connection to the bus */
+ struct btd_device *dev; /* Device reference */
+ struct att_range *svc_range; /* Thermometer range */
+};
+
+static GSList *thermometers = NULL;
+
+static void destroy_thermometer(gpointer user_data)
+{
+ struct thermometer *t = user_data;
+
+ dbus_connection_unref(t->conn);
+ btd_device_unref(t->dev);
+ g_free(t->svc_range);
+ g_free(t);
+}
+
+static DBusMessage *get_properties(DBusConnection *conn, DBusMessage *msg,
+ void *data)
+{
+ /* TODO: */
+ return g_dbus_create_error(msg, ERROR_INTERFACE ".ThermometerError",
+ "Function not implemented.");
+}
+
+static DBusMessage *set_property(DBusConnection *conn, DBusMessage *msg,
+ void *data)
+{
+ /* TODO: */
+ return g_dbus_create_error(msg, ERROR_INTERFACE ".ThermometerError",
+ "Function not implemented.");
+}
+
+static DBusMessage *register_watcher(DBusConnection *conn, DBusMessage *msg,
+ void *data)
+{
+ /* TODO: */
+ return g_dbus_create_error(msg, ERROR_INTERFACE ".ThermometerError",
+ "Function not implemented.");
+}
+
+static DBusMessage *unregister_watcher(DBusConnection *conn, DBusMessage *msg,
+ void *data)
+{
+ /* TODO: */
+ return g_dbus_create_error(msg, ERROR_INTERFACE ".ThermometerError",
+ "Function not implemented.");
+}
+
+static DBusMessage *enable_intermediate(DBusConnection *conn, DBusMessage *msg,
+ void *data)
+{
+ /* TODO: */
+ return g_dbus_create_error(msg, ERROR_INTERFACE ".ThermometerError",
+ "Function not implemented.");
+}
+
+static DBusMessage *disable_intermediate(DBusConnection *conn, DBusMessage *msg,
+ void *data)
+{
+ /* TODO: */
+ return g_dbus_create_error(msg, ERROR_INTERFACE ".ThermometerError",
+ "Function not implemented.");
+}
+
+static GDBusMethodTable thermometer_methods[] = {
+ { "GetProperties", "", "a{sv}", get_properties },
+ { "SetProperty", "sv", "", set_property,
+ G_DBUS_METHOD_FLAG_ASYNC },
+ { "RegisterWatcher", "o", "", register_watcher },
+ { "UnregisterWatcher", "o", "", unregister_watcher },
+ { "EnableIntermediateMeasurement", "o", "", enable_intermediate },
+ { "DisableIntermediateMeasurement","o", "", disable_intermediate },
+ { }
+};
+
+static GDBusSignalTable thermometer_signals[] = {
+ { "PropertyChanged", "sv" },
+ { }
+};
+
int thermometer_register(DBusConnection *connection, struct btd_device *device,
struct att_primary *tattr)
{
- /* TODO: Register Health Thermometer Interface */
+ const gchar *path = device_get_path(device);
+ struct thermometer *t;
+
+ t = g_new0(struct thermometer, 1);
+ t->conn = dbus_connection_ref(connection);
+ t->dev = btd_device_ref(device);
+ t->svc_range = g_new0(struct att_range, 1);
+ t->svc_range->start = tattr->start;
+ t->svc_range->end = tattr->end;
+
+ if (!g_dbus_register_interface(t->conn, path, THERMOMETER_INTERFACE,
+ thermometer_methods, thermometer_signals,
+ NULL, t, destroy_thermometer)) {
+ error("D-Bus failed to register %s interface",
+ THERMOMETER_INTERFACE);
+ destroy_thermometer(t);
+ return -1;
+ }
+
+ thermometers = g_slist_prepend(thermometers, t);
return 0;
}
--
1.7.6.1
---
thermometer/thermometer.c | 23 ++++++++++++++++++++++-
1 files changed, 22 insertions(+), 1 deletions(-)
diff --git a/thermometer/thermometer.c b/thermometer/thermometer.c
index a89d8a9..cdfe455 100644
--- a/thermometer/thermometer.c
+++ b/thermometer/thermometer.c
@@ -51,6 +51,17 @@ static void destroy_thermometer(gpointer user_data)
g_free(t);
}
+static gint cmp_device(gconstpointer a, gconstpointer b)
+{
+ const struct thermometer *t = a;
+ const struct btd_device *dev = b;
+
+ if (dev == t->dev)
+ return 0;
+
+ return -1;
+}
+
static DBusMessage *get_properties(DBusConnection *conn, DBusMessage *msg,
void *data)
{
@@ -143,5 +154,15 @@ int thermometer_register(DBusConnection *connection, struct btd_device *device,
void thermometer_unregister(struct btd_device *device)
{
- /* TODO: Unregister Health Thermometer Interface */
+ struct thermometer *t;
+ GSList *l;
+
+ l = g_slist_find_custom(thermometers, device, cmp_device);
+ if (!l)
+ return;
+
+ t = l->data;
+ thermometers = g_slist_remove(thermometers, t);
+ g_dbus_unregister_interface(t->conn, device_get_path(t->dev),
+ THERMOMETER_INTERFACE);
}
--
1.7.6.1
Hi Anderson
2011/9/28 Anderson Lizardo <[email protected]>:
> Hi Santiago,
>
> On Wed, Sep 28, 2011 at 1:48 PM, Santiago Carot-Nemesio
> <[email protected]> wrote:
>> +static gchar* temptype2str(uint8_t value)
>
> Small conding style issue here: gchar* ?-> gchar *
>
>> +{
>> + ? ? ? 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;
>> + ? ? ? };
>> +}
>> +
>> ?static void destroy_char(gpointer user_data)
>> ?{
>> ? ? ? ?struct characteristic *c = user_data;
>> @@ -113,7 +138,27 @@ static void discover_desc_cb(guint8 status, const guint8 *pdu, guint16 len,
>> ?static void read_temp_type_cb(guint8 status, const guint8 *pdu, guint16 len,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?gpointer user_data)
>> ?{
>> - ? ? ? /* TODO */
>> + ? ? ? struct characteristic *ch = user_data;
>> + ? ? ? struct thermometer *t = ch->t;
>> + ? ? ? uint8_t value;
>> + ? ? ? int vlen;
>> +
>> + ? ? ? if (status != 0) {
>> + ? ? ? ? ? ? ? DBG("Temperature Type value read failed: %s",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? att_ecode2str(status));
>> + ? ? ? ? ? ? ? return;
>> + ? ? ? }
>> +
>> + ? ? ? if (!dec_read_resp(pdu, len, &value, &vlen)) {
>
> Your code is assuming that the read response PDU will always have a
> one-byte value. This will cause memory corruption if the peer sends an
> error response with size greater than 2 (first byte is the opcode).
>
> Either put a check like "if (len == sizeof(uint8_t) + 1 &&
> read_read_resp(...))", or use:
>
> uint8_t value[ATT_MAX_MTU];
>
> and cast the first byte.
>
You're right. I'm fixing it. Gatt API is quite confusing.
> (just noticed that Proximity monitor code has the same bug...)
>
>> + ? ? ? ? ? ? ? DBG("Protocol error");
>> + ? ? ? ? ? ? ? return;
>> + ? ? ? }
>> +
>> + ? ? ? DBG("TEMPERATURE TYPE: %s", temptype2str(value));
>
> Can you avoid using all-caps on the DBG() message above?
>
>> + ? ? ? if (!t->has_interval)
>> + ? ? ? ? ? ? ? t->has_interval = TRUE;
>
> Why the if() check? You could just do "t->has_interval = TRUE;" always.
>
Ok.
I'm going to send a new set of patches fixing this issues.
Regards.
Hi Santiago,
On Wed, Sep 28, 2011 at 1:48 PM, Santiago Carot-Nemesio
<[email protected]> wrote:
> +static gchar* temptype2str(uint8_t value)
Small conding style issue here: gchar* -> gchar *
> +{
> + ? ? ? 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;
> + ? ? ? };
> +}
> +
> ?static void destroy_char(gpointer user_data)
> ?{
> ? ? ? ?struct characteristic *c = user_data;
> @@ -113,7 +138,27 @@ static void discover_desc_cb(guint8 status, const guint8 *pdu, guint16 len,
> ?static void read_temp_type_cb(guint8 status, const guint8 *pdu, guint16 len,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?gpointer user_data)
> ?{
> - ? ? ? /* TODO */
> + ? ? ? struct characteristic *ch = user_data;
> + ? ? ? struct thermometer *t = ch->t;
> + ? ? ? uint8_t value;
> + ? ? ? int vlen;
> +
> + ? ? ? if (status != 0) {
> + ? ? ? ? ? ? ? DBG("Temperature Type value read failed: %s",
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? att_ecode2str(status));
> + ? ? ? ? ? ? ? return;
> + ? ? ? }
> +
> + ? ? ? if (!dec_read_resp(pdu, len, &value, &vlen)) {
Your code is assuming that the read response PDU will always have a
one-byte value. This will cause memory corruption if the peer sends an
error response with size greater than 2 (first byte is the opcode).
Either put a check like "if (len == sizeof(uint8_t) + 1 &&
read_read_resp(...))", or use:
uint8_t value[ATT_MAX_MTU];
and cast the first byte.
(just noticed that Proximity monitor code has the same bug...)
> + ? ? ? ? ? ? ? DBG("Protocol error");
> + ? ? ? ? ? ? ? return;
> + ? ? ? }
> +
> + ? ? ? DBG("TEMPERATURE TYPE: %s", temptype2str(value));
Can you avoid using all-caps on the DBG() message above?
> + ? ? ? if (!t->has_interval)
> + ? ? ? ? ? ? ? t->has_interval = TRUE;
Why the if() check? You could just do "t->has_interval = TRUE;" always.
> +
> + ? ? ? t->type = value;
> ?}
Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
Hi Santiago,
On Thu, Sep 29, 2011, Santiago Carot-Nemesio wrote:
> @@ -148,7 +192,31 @@ static void read_temp_type_cb(guint8 status, const guint8 *pdu, guint16 len,
> static void read_interval_cb(guint8 status, const guint8 *pdu, guint16 len,
> gpointer user_data)
> {
> - /* TODO */
> + struct characteristic *ch = user_data;
> + uint8_t value[ATT_MAX_MTU];
> + uint16_t *p, interval;
> + int vlen;
> +
> + if (status != 0) {
> + DBG("Measurement Interval value read failed: %s",
> + att_ecode2str(status));
> + return;
> + }
> +
> + if (!dec_read_resp(pdu, len, value, &vlen)) {
> + DBG("Protocol error\n");
> + return;
> + }
> +
> + if (vlen < 2) {
> + DBG("Invalid Interval received");
> + return;
> + }
> +
> + p = (uint16_t *) value;
> + interval = btohs(*p);
This looks like a potential unaligned access issue. Probably safer to
use bt_get_unaligned here.
Johan
On Thu, Sep 29, 2011, Santiago Carot-Nemesio wrote:
> @@ -113,7 +120,29 @@ static void discover_desc_cb(guint8 status, const guint8 *pdu, guint16 len,
> static void read_temp_type_cb(guint8 status, const guint8 *pdu, guint16 len,
> gpointer user_data)
> {
> - /* TODO */
> + struct characteristic *ch = user_data;
> + struct thermometer *t = ch->t;
> + uint8_t value[ATT_MAX_MTU];
> + int vlen;
> +
> + if (status != 0) {
This is correct. So why did you have "if (status)" in the previous
patch?
> + t->type = (uint8_t) value[0];
This looks like an unnecessary explicit type-cast. The array is already
of type uint8_t.
Johan
Hi Santiago,
On Thu, Sep 29, 2011, Santiago Carot-Nemesio wrote:
> +static void configure_thermometer_cb(GSList *characteristics, guint8 status,
> + gpointer user_data)
> +{
> + struct thermometer *t = user_data;
> + GSList *l;
> +
> + if (status) {
if (status != 0)
Johan
Hi Santiago,
On Thu, Sep 29, 2011, Santiago Carot-Nemesio wrote:
> @@ -40,6 +40,7 @@ struct thermometer {
> GAttrib *attrib; /* GATT connection */
> struct att_range *svc_range; /* Thermometer range */
> gint attioid; /* Att watcher id */
> + gint attindid; /* Att incications id */
I suspect this should also be a guint?
> + if (t->attindid)
attindid > 0
> + if (t->attindid) {
> + g_attrib_unregister(t->attrib, t->attindid);
> + t->attindid = 0;
> + }
attindid > 0
Johan
Hi Santiago,
On Thu, Sep 29, 2011, Santiago Carot-Nemesio wrote:
> @@ -36,7 +37,9 @@
> struct thermometer {
> DBusConnection *conn; /* The connection to the bus */
> struct btd_device *dev; /* Device reference */
> + GAttrib *attrib; /* GATT connection */
> struct att_range *svc_range; /* Thermometer range */
> + gint attioid; /* Att watcher id */
> };
Looking at the attio.h API, the attioid should be guint and not gint,
right? Also, this struct has the same inconsistency in indentation. Some
lines use spaces whereas other use tabs. Please fix this (though
potentially in a separate coding-style cleanup patch since the
inconsistency exists there already before this patch).
> + if (t->attioid)
> + btd_device_remove_attio_callback(t->dev, t->attioid);
Please use if (t->attioid > 0)
Johan
Hi Santiago,
On Thu, Sep 29, 2011, Santiago Carot-Nemesio wrote:
> + l = g_slist_find_custom(thermometers, device, cmp_device);
> + if (!l)
Same thing as I mentioned previously about comparing pointers.
Johan
Hi Santiago,
On Thu, Sep 29, 2011, Santiago Carot-Nemesio wrote:
> +struct thermometer {
> + DBusConnection *conn; /* The connection to the bus */
> + struct btd_device *dev; /* Device reference */
> + struct att_range *svc_range; /* Thermometer range */
> +};
This struct has inconsistent indentation between the variable types and
their names. The first two ones use spaces whereas the third one uses a
tab.
> +static void destroy_thermometer(gpointer user_data)
> +{
> + struct thermometer *t = user_data;
> +
> + dbus_connection_unref(t->conn);
> + btd_device_unref(t->dev);
> + g_free(t->svc_range);
> + g_free(t);
Incorrect indentation for the last line (just spaces).
> + }
> +
> + thermometers = g_slist_prepend(thermometers, t);
> return 0;
Minor nitpick: add an empty line before the return statement.
Johan
Hi Santiago,
On Thu, Sep 29, 2011, Santiago Carot-Nemesio wrote:
> static int thermometer_driver_probe(struct btd_device *device, GSList *uuids)
> {
> - return thermometer_register(connection, device);
> + struct att_primary *tattr;
> + GSList *list;
> +
> + list = device_services_from_record(device, uuids);
> + if (!list)
> + return -1;
Could you try to avoid -1 error values. Please choose some appropriate
POSIX error code instead (e.g. EINVAL in this case). Also (and I know
we're note very consistent with this) when you're testing whether a
pointer is NULL use "pointer == NULL" instead of "!pointer". This makes
is directly clear that this is not a variable of some boolean type.
Johan