Return-Path: MIME-Version: 1.0 In-Reply-To: <1343194947-13659-9-git-send-email-chen.ganir@ti.com> References: <1343194947-13659-1-git-send-email-chen.ganir@ti.com> <1343194947-13659-9-git-send-email-chen.ganir@ti.com> Date: Tue, 7 Aug 2012 13:29:39 -0300 Message-ID: Subject: Re: [PATCH v2 8/9] Battery: Add support for notifications From: Joao Paulo Rechi Vita To: Chen Ganir Cc: linux-bluetooth@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: On Wed, Jul 25, 2012 at 2:42 AM, Chen Ganir wrote: > Add support for emitting PropertyChanged when a battery level > characteristic notification is sent from the peer device. > --- > doc/battery-api.txt | 5 ++ > profiles/batterystate/batterystate.c | 107 +++++++++++++++++++++++++++++++++- > 2 files changed, 111 insertions(+), 1 deletion(-) > > diff --git a/doc/battery-api.txt b/doc/battery-api.txt > index 5d7510d..f31e7e5 100644 > --- a/doc/battery-api.txt > +++ b/doc/battery-api.txt > @@ -16,6 +16,11 @@ Methods dict GetProperties() > Returns all properties for the interface. See the > Properties section for the available properties. > > +Signals PropertyChanged(string name, variant value) > + > + This signal indicates a changed value of the given > + property. > + > Properties byte Namespace [readonly] > > Namespace value from the battery format characteristic > diff --git a/profiles/batterystate/batterystate.c b/profiles/batterystate/batterystate.c > index e5b68a9..718863b 100644 > --- a/profiles/batterystate/batterystate.c > +++ b/profiles/batterystate/batterystate.c > @@ -50,6 +50,7 @@ struct battery { > GAttrib *attrib; /* GATT connection */ > guint attioid; /* Att watcher id */ > struct att_range *svc_range; /* Battery range */ > + guint attnotid; /* Att notifications id */ > GSList *chars; /* Characteristics */ > }; > > @@ -104,6 +105,14 @@ static gint cmp_device(gconstpointer a, gconstpointer b) > return -1; > } > > +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 void batterystate_free(gpointer user_data) > { > struct battery *batt = user_data; > @@ -117,6 +126,10 @@ static void batterystate_free(gpointer user_data) > if (batt->attrib != NULL) > g_attrib_unref(batt->attrib); > > + if (batt->attrib != NULL) { > + g_attrib_unregister(batt->attrib, batt->attnotid); > + g_attrib_unref(batt->attrib); > + } > > dbus_connection_unref(batt->conn); > btd_device_unref(batt->dev); > @@ -158,6 +171,17 @@ static void process_batteryservice_char(struct characteristic *ch) > } > } > > +static void batterylevel_enable_notify_cb(guint8 status, const guint8 *pdu, > + guint16 len, gpointer user_data) > +{ > + char *msg = user_data; > + > + if (status != 0) > + error("Could not enable battery level notification: %s", msg); > + > + g_free(msg); > +} > + > static void batterylevel_presentation_format_desc_cb(guint8 status, > const guint8 *pdu, guint16 len, > gpointer user_data) > @@ -194,6 +218,23 @@ static void process_batterylevel_desc(struct descriptor *desc) > char uuidstr[MAX_LEN_UUID_STR]; > bt_uuid_t btuuid; > > + bt_uuid16_create(&btuuid, GATT_CLIENT_CHARAC_CFG_UUID); > + > + if (bt_uuid_cmp(&desc->uuid, &btuuid) == 0 && g_strcmp0(ch->attr.uuid, > + BATTERY_LEVEL_UUID) == 0) { > + uint8_t atval[2]; > + uint16_t val; > + char *msg; > + > + val = GATT_CLIENT_CHARAC_CFG_NOTIF_BIT; > + msg = g_strdup("Enable BatteryLevel notification"); > + What's the point of passing "Enable BatteryLevel notification" as user_data for the batterylevel_enable_notify_cb() ? > + att_put_u16(val, atval); > + gatt_write_char(ch->batt->attrib, desc->handle, atval, 2, > + batterylevel_enable_notify_cb, msg); > + return; > + } > + > bt_uuid16_create(&btuuid, GATT_CHARAC_FMT_UUID); > > if (bt_uuid_cmp(&desc->uuid, &btuuid) == 0) { > @@ -277,6 +318,12 @@ static DBusMessage *get_properties(DBusConnection *conn, DBusMessage *msg, > return reply; > } > > +static void emit_battery_level_changed(struct characteristic *c) > +{ > + emit_property_changed(c->batt->conn, c->path, BATTERY_INTERFACE, > + "Level", DBUS_TYPE_BYTE, &c->level); > +} > + > static GDBusMethodTable battery_methods[] = { > { GDBUS_METHOD("GetProperties", > NULL, GDBUS_ARGS({ "properties", "a{sv}" }), > @@ -284,6 +331,11 @@ static GDBusMethodTable battery_methods[] = { > { } > }; > > +static GDBusSignalTable battery_signals[] = { > + { GDBUS_SIGNAL("PropertyChanged", > + GDBUS_ARGS({ "name", "s" }, { "value", "v" })) }, > + { } > +}; > > static void configure_batterystate_cb(GSList *characteristics, guint8 status, > gpointer user_data) > @@ -318,7 +370,7 @@ static void configure_batterystate_cb(GSList *characteristics, guint8 status, > > if (!g_dbus_register_interface(batt->conn, ch->path, > BATTERY_INTERFACE, > - battery_methods, NULL, NULL, > + battery_methods, battery_signals, NULL, > ch, NULL)) { > error("D-Bus register interface %s failed", > BATTERY_INTERFACE); > @@ -346,12 +398,63 @@ static void configure_batterystate_cb(GSList *characteristics, guint8 status, > } > } > > +static void proc_batterylevel(struct characteristic *c, const uint8_t *pdu, > + uint16_t len, gboolean final) > +{ > + uint8_t new_batt_level = 0; > + gboolean changed = FALSE; > + > + if (!pdu) { > + error("Battery level notification: Invalid pdu length"); > + goto done; > + } > + If pdu can be NULL here, than bluetoothd would have broken when derreferencing it at "handle = att_get_u16(&pdu[1]);" in notif_handler(). If pdu can be NULL the check needs to be done there. > + new_batt_level = pdu[1]; > + > + if (new_batt_level != c->level) > + changed = TRUE; > + > + c->level = new_batt_level; > + > +done: There is no need for the done label, since 'changed' is always false when you 'goto' here. You could simply return instead of using goto, although the whole if statement is likely to be dropped from this function. > + if (changed) > + emit_battery_level_changed(c); > +} > + > +static void notif_handler(const uint8_t *pdu, uint16_t len, gpointer user_data) > +{ > + struct battery *batt = user_data; > + struct characteristic *ch; > + uint16_t handle; > + GSList *l; > + ch->attr.uuid == BATTERY_LEVEL_UUID should be tested before anything else, since other profiles may have enabled notifications for other characteristics (hog does that, for example). When testing with hog devices I got "notif_handler: Unexpected handle 0x0017" for every input report received. > + if (len < 3) { > + error("notif_handler: Bad pdu received"); > + return; > + } > + > + handle = att_get_u16(&pdu[1]); > + l = g_slist_find_custom(batt->chars, &handle, cmp_char_val_handle); > + if (l == NULL) { > + error("notif_handler: Unexpected handle 0x%04x", handle); > + return; > + } > + > + ch = l->data; > + if (g_strcmp0(ch->attr.uuid, BATTERY_LEVEL_UUID) == 0) { > + proc_batterylevel(ch, pdu, len, FALSE); > + } > +} > + > static void attio_connected_cb(GAttrib *attrib, gpointer user_data) > { > struct battery *batt = user_data; > > batt->attrib = g_attrib_ref(attrib); > > + batt->attnotid = g_attrib_register(batt->attrib, ATT_OP_HANDLE_NOTIFY, > + notif_handler, batt, NULL); > + > if (batt->chars == NULL) { > gatt_discover_char(batt->attrib, batt->svc_range->start, > batt->svc_range->end, NULL, > @@ -369,6 +472,8 @@ static void attio_disconnected_cb(gpointer user_data) > { > struct battery *batt = user_data; > > + g_attrib_unregister(batt->attrib, batt->attnotid); > + batt->attnotid = 0; > g_attrib_unref(batt->attrib); > batt->attrib = NULL; > } > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- João Paulo Rechi Vita Openbossa Labs - INdT