From: Chen Ganir <chen.ganir@ti.com>
To: Joao Paulo Rechi Vita <jprvita@openbossa.org>
Cc: <linux-bluetooth@vger.kernel.org>
Subject: Re: [PATCH 09/10] battery: Add support for notifications
Date: Thu, 13 Sep 2012 14:27:09 +0300 [thread overview]
Message-ID: <5051C30D.9020608@ti.com> (raw)
In-Reply-To: <5050167A.8020400@ti.com>
Joao,
On 09/12/2012 07:58 AM, Chen Ganir wrote:
> Joao,
> On 09/12/2012 01:08 AM, Joao Paulo Rechi Vita wrote:
>> On Tue, Sep 11, 2012 at 4:38 AM, <chen.ganir@ti.com> wrote:
>>> From: Chen Ganir <chen.ganir@ti.com>
>>>
>>> Add support for emitting PropertyChanged when a battery level
>>> characteristic notification is sent from the peer device.
>>> ---
>>> profiles/battery/battery.c | 95
>>> +++++++++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 93 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/profiles/battery/battery.c b/profiles/battery/battery.c
>>> index a93b352..95f548e 100644
>>> --- a/profiles/battery/battery.c
>>> +++ b/profiles/battery/battery.c
>>> @@ -43,6 +43,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 */
>>> };
>>>
>>> @@ -56,6 +57,7 @@ struct characteristic {
>>> uint8_t ns; /* Battery Namespace */
>>> uint16_t description; /* Battery
>>> description */
>>> uint8_t level;
>>> + gboolean canNotify;
>>> };
>>>
>>> struct descriptor {
>>> @@ -86,6 +88,14 @@ static void char_free(gpointer user_data)
>>> g_free(c);
>>> }
>>>
>>> +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 battery_free(gpointer user_data)
>>> {
>>> struct battery *batt = user_data;
>>> @@ -99,6 +109,10 @@ static void battery_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);
>>> + }
>>
>> You're using the same condition twice here (batt->attrib != NULL).
>> Looks like a copy & paste error. Make sure you check for attnotid
>> before checking attrib, and that you don't unref attrib twice.
>>
> Correct. I'll fix this and check for attnotid.
>
>>> btd_device_unref(batt->dev);
>>> g_free(batt->svc_range);
>>> g_free(batt);
>>> @@ -140,6 +154,18 @@ static void process_batteryservice_char(struct
>>> characteristic *ch)
>>> }
>>> }
>>>
>>> +static void batterylevel_enable_notify_cb(guint8 status, const
>>> guint8 *pdu,
>>> + guint16 len, gpointer
>>> user_data)
>>> +{
>>> + struct characteristic *ch = (struct characteristic *)user_data;
>>> +
>>> + if (status != 0) {
>>> + error("Could not enable batt level notification.");
>>> + ch->canNotify = FALSE;
>>> + process_batteryservice_char(ch);
>>
>> I don't think is necessary to call process_batteryservice_char() here,
>> since you already called it during the characteristics discovery.
>>
> I'll remove the unnecessary call here.
>
>
After checking this again, this call should not be removed. The reason
for that is simple - if a characteristic can not be notified, and we
read its initial value from file, we need to make sure we refresh the
battery level from the peer device automatically (since we will not get
notified when level changes).
>>> + }
>>> +}
>>> +
>>> static gint device_battery_cmp(gconstpointer a, gconstpointer b)
>>> {
>>> const struct characteristic *ch = a;
>>> @@ -178,6 +204,19 @@ static void batterylevel_refresh_cb(struct
>>> device_battery *batt)
>>> process_batteryservice_char(ch);
>>> }
>>>
>>> +static void enable_battery_notification(struct characteristic *ch,
>>> +
>>> uint16_t handle)
>>> +{
>>> + uint8_t atval[2];
>>> + uint16_t val;
>>> +
>>> + val = GATT_CLIENT_CHARAC_CFG_NOTIF_BIT;
>>> +
>>> + att_put_u16(val, atval);
>>> + gatt_write_char(ch->batt->attrib, handle, atval, 2,
>>> + batterylevel_enable_notify_cb, ch);
>>> +}
>>> +
>>> static void batterylevel_presentation_format_desc_cb(guint8 status,
>>> const guint8 *pdu,
>>> guint16 len,
>>> gpointer user_data)
>>> @@ -207,13 +246,20 @@ static void
>>> batterylevel_presentation_format_desc_cb(guint8 status,
>>> desc->ch->description = att_get_u16(&value[5]);
>>> }
>>>
>>> -
>>> static void process_batterylevel_desc(struct descriptor *desc)
>>> {
>>> struct characteristic *ch = desc->ch;
>>> 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) {
>>> + enable_battery_notification(ch, desc->handle);
>>> + return;
>>> + }
>>> +
>>> bt_uuid16_create(&btuuid, GATT_CHARAC_FMT_UUID);
>>>
>>> if (bt_uuid_cmp(&desc->uuid, &btuuid) == 0) {
>>> @@ -319,12 +365,54 @@ static void configure_battery_cb(GSList
>>> *characteristics, guint8 status,
>>> }
>>> }
>>>
>>> +static void proc_batterylevel(struct characteristic *c, const
>>> uint8_t *pdu,
>>> + uint16_t len,
>>> gboolean final)
>>> +{
>>> + if (!pdu) {
>>> + error("Battery level notification: Invalid pdu length");
>>> + return;
>>> + }
>>> +
>>> + c->level = pdu[1];
>>> +
>>> + btd_device_set_battery_opt(c->devbatt, BATTERY_OPT_LEVEL,
>>> c->level,
>>> +
>>> BATTERY_OPT_INVALID);
>>> +}
>>> +
>>> +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;
>>> +
>>> + 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,
>>> @@ -333,7 +421,8 @@ static void attio_connected_cb(GAttrib *attrib,
>>> gpointer user_data)
>>> GSList *l;
>>> for (l = batt->chars; l; l = l->next) {
>>> struct characteristic *c = l->data;
>>> - process_batteryservice_char(c);
>>> + if (!c->canNotify)
>>> + process_batteryservice_char(c);
>>> }
>>> }
>>> }
>>> @@ -342,6 +431,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
>>
>>
>>
>
> Thanks,
>
--
BR,
Chen Ganir
prev parent reply other threads:[~2012-09-13 11:27 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-11 7:38 [PATCH 00/10] Implement Generic battery and LE Battery client chen.ganir
2012-09-11 7:38 ` [PATCH 01/10] battery: Add generic device battery documentation chen.ganir
2012-09-11 15:34 ` Joao Paulo Rechi Vita
2012-09-12 4:45 ` Chen Ganir
2012-09-11 7:38 ` [PATCH 02/10] battery: Implement Generic device battery chen.ganir
2012-09-11 18:27 ` Joao Paulo Rechi Vita
2012-09-12 4:48 ` Chen Ganir
2012-09-12 8:45 ` Johan Hedberg
2012-09-12 10:30 ` Chen Ganir
2012-09-12 10:57 ` Anderson Lizardo
2012-09-13 11:32 ` Chen Ganir
2012-09-11 7:38 ` [PATCH 03/10] battery: Add GATT Battery Client Service skeleton chen.ganir
2012-09-11 7:38 ` [PATCH 04/10] battery: Add client connection logic chen.ganir
2012-09-11 7:38 ` [PATCH 05/10] battery: Discover Characteristic Descriptors chen.ganir
2012-09-11 20:52 ` Joao Paulo Rechi Vita
2012-09-12 4:49 ` Chen Ganir
2012-09-11 7:38 ` [PATCH 06/10] battery: Get Battery ID chen.ganir
2012-09-11 7:38 ` [PATCH 07/10] battery: Add Battery to device chen.ganir
2012-09-11 21:40 ` Joao Paulo Rechi Vita
2012-09-12 4:54 ` Chen Ganir
2012-09-11 7:38 ` [PATCH 08/10] battery: Read Battery level characteristic chen.ganir
2012-09-11 21:50 ` Joao Paulo Rechi Vita
2012-09-12 4:55 ` Chen Ganir
2012-09-11 7:38 ` [PATCH 09/10] battery: Add support for notifications chen.ganir
2012-09-11 22:08 ` Joao Paulo Rechi Vita
2012-09-12 4:58 ` Chen Ganir
2012-09-13 11:27 ` Chen Ganir [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5051C30D.9020608@ti.com \
--to=chen.ganir@ti.com \
--cc=jprvita@openbossa.org \
--cc=linux-bluetooth@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).