Hi Mikel:
On Fri, Feb 15, 2013 at 5:54 AM, Mikel Astiz <mikel.astiz.oss@xxxxxxxxx> wrote:
> Hi Claudio,
>
> On Thu, Feb 14, 2013 at 5:37 PM, Claudio Takahasi
> <claudio.takahasi@xxxxxxxxxxxxx> wrote:
>> Hi Mikel:
>>
>> On Wed, Feb 6, 2013 at 6:16 AM, Mikel Astiz <mikel.astiz.oss@xxxxxxxxx> wrote:
>>> From: Mikel Astiz <mikel.astiz@xxxxxxxxxxxx>
>>>
>>> Split the monitor registration API into three independent registrations
>>> each of them taking one specific GATT primary.
>>> ---
>>> profiles/proximity/manager.c | 16 +++-
>>> profiles/proximity/monitor.c | 220 ++++++++++++++++++++++++++++++++++---------
>>> profiles/proximity/monitor.h | 17 +++-
>>> 3 files changed, 204 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/profiles/proximity/manager.c b/profiles/proximity/manager.c
>>> index b405f15..1c07267 100644
>>> --- a/profiles/proximity/manager.c
>>> +++ b/profiles/proximity/manager.c
>>> @@ -52,18 +52,30 @@ static int monitor_device_probe(struct btd_profile *p,
>>> struct btd_device *device, GSList *uuids)
>>> {
>>> struct gatt_primary *linkloss, *txpower, *immediate;
>>> + int err = 0;
>>>
>>> immediate = btd_device_get_primary(device, IMMEDIATE_ALERT_UUID);
>>> txpower = btd_device_get_primary(device, TX_POWER_UUID);
>>> linkloss = btd_device_get_primary(device, LINK_LOSS_UUID);
>>>
>>> - return monitor_register(device, linkloss, txpower, immediate, &enabled);
>>> + if (linkloss)
>>> + err = monitor_register_linkloss(device, &enabled, linkloss);
>>> +
>>> + if (err >= 0 && txpower)
>>> + err = monitor_register_txpower(device, &enabled, txpower);
>>> +
>>> + if (err >= 0 && immediate)
>>> + err = monitor_register_immediate(device, &enabled, immediate);
>>> +
>>
>> I recommend to invert the logic here, checking if immediate alert is
>> available first. Immediate alert service is mandatory for Link Loss,
>> Path Loss(tx power) and find me.
>
> According to the code being removed in monitor_register(), the
> linkloss service seems independent from immediate alert. In fact, I
> tried to clarify this in the spec and what I found is that the
> linkloss service is mandatory while the rest are optional. Am I
> missing something?
Indeed, they are independent services. I messed everything up, the
Proximity/Find Profile and their roles.
Immediate Alert service is required for Proximity Path Loss and Find Me.
>
> In any case, to start with, it makes sense that txpower gets
> registered last to improve readability.
>
> Also note that I tried to express the dependencies you mention in
> update_monitor() and the registration functions assuming that, once
> split, the profiles can be probed in any arbitrary order (i.e. txpower
> probed before immediate alert).
>
>>
>>> + return err;
>>> }
>>>
>>> static void monitor_device_remove(struct btd_profile *p,
>>> struct btd_device *device)
>>> {
>>> - monitor_unregister(device);
>>> + monitor_unregister_immediate(device);
>>> + monitor_unregister_txpower(device);
>>> + monitor_unregister_linkloss(device);
>>> }
>>>
>>> static struct btd_profile pxp_monitor_profile = {
>>> diff --git a/profiles/proximity/monitor.c b/profiles/proximity/monitor.c
>>> index 597f161..489ccdd 100644
>>> --- a/profiles/proximity/monitor.c
>>> +++ b/profiles/proximity/monitor.c
>>> @@ -34,6 +34,7 @@
>>> #include <stdio.h>
>>> #include <stdlib.h>
>>> #include <sys/stat.h>
>>> +#include <glib.h>
>>>
>>> #include <bluetooth/bluetooth.h>
>>>
>>> @@ -83,6 +84,22 @@ struct monitor {
>>> guint attioid;
>>> };
>>>
>>> +static GSList *monitors = NULL;
>>> +
>>> +static struct monitor *find_monitor(struct btd_device *device)
>>> +{
>>> + GSList *l;
>>> +
>>> + for (l = monitors; l; l = l->next) {
>>> + struct monitor *monitor = l->data;
>>> +
>>> + if (monitor->device == device)
>>> + return monitor;
>>> + }
>>> +
>>> + return NULL;
>>> +}
>>> +
>>> static void write_proximity_config(struct btd_device *device, const char *alert,
>>> const char *level)
>>> {
>>> @@ -580,33 +597,25 @@ static void monitor_destroy(gpointer user_data)
>>> {
>>> struct monitor *monitor = user_data;
>>>
>>> - if (monitor->immediateto)
>>> - g_source_remove(monitor->immediateto);
>>> -
>>> - if (monitor->attioid)
>>> - btd_device_remove_attio_callback(monitor->device,
>>> - monitor->attioid);
>>> - if (monitor->attrib)
>>> - g_attrib_unref(monitor->attrib);
>>> -
>>> btd_device_unref(monitor->device);
>>> - g_free(monitor->linkloss);
>>> - g_free(monitor->immediate);
>>> - g_free(monitor->txpower);
>>> g_free(monitor->linklosslevel);
>>> g_free(monitor->immediatelevel);
>>> g_free(monitor->signallevel);
>>> g_free(monitor);
>>> +
>>> + monitors = g_slist_remove(monitors, monitor);
>>> }
>>>
>>> -int monitor_register(struct btd_device *device,
>>> - struct gatt_primary *linkloss, struct gatt_primary *txpower,
>>> - struct gatt_primary *immediate, struct enabled *enabled)
>>> +static struct monitor *register_monitor(struct btd_device *device)
>>> {
>>> const char *path = device_get_path(device);
>>> struct monitor *monitor;
>>> char *level;
>>>
>>> + monitor = find_monitor(device);
>>> + if (monitor != NULL)
>>> + return monitor;
>>> +
>>> level = read_proximity_config(device, "LinkLossAlertLevel");
>>>
>>> monitor = g_new0(struct monitor, 1);
>>> @@ -615,6 +624,8 @@ int monitor_register(struct btd_device *device,
>>> monitor->signallevel = g_strdup("unknown");
>>> monitor->immediatelevel = g_strdup("none");
>>>
>>> + monitors = g_slist_append(monitors, monitor);
>>> +
>>> if (g_dbus_register_interface(btd_get_dbus_connection(), path,
>>> PROXIMITY_INTERFACE,
>>> NULL, NULL, monitor_device_properties,
>>> @@ -622,55 +633,178 @@ int monitor_register(struct btd_device *device,
>>> error("D-Bus failed to register %s interface",
>>> PROXIMITY_INTERFACE);
>>> monitor_destroy(monitor);
>>> - return -1;
>>> + return NULL;
>>> }
>>>
>>> DBG("Registered interface %s on path %s", PROXIMITY_INTERFACE, path);
>>>
>>> - if (linkloss && enabled->linkloss) {
>>> - monitor->linkloss = g_new0(struct att_range, 1);
>>> - monitor->linkloss->start = linkloss->range.start;
>>> - monitor->linkloss->end = linkloss->range.end;
>>> -
>>> - monitor->enabled.linkloss = TRUE;
>>> - }
>>> -
>>> - if (immediate) {
>>> - if (txpower && enabled->pathloss) {
>>> - monitor->txpower = g_new0(struct att_range, 1);
>>> - monitor->txpower->start = txpower->range.start;
>>> - monitor->txpower->end = txpower->range.end;
>>> -
>>> - monitor->enabled.pathloss = TRUE;
>>> - }
>>> -
>>> - if (enabled->pathloss || enabled->findme) {
>>> - monitor->immediate = g_new0(struct att_range, 1);
>>> - monitor->immediate->start = immediate->range.start;
>>> - monitor->immediate->end = immediate->range.end;
>>> - }
>>> + return monitor;
>>> +}
>>>
>>> - monitor->enabled.findme = enabled->findme;
>>> - }
>>> +static void update_monitor(struct monitor *monitor)
>>> +{
>>> + if (monitor->txpower != NULL && monitor->immediate != NULL)
>>> + monitor->enabled.pathloss = TRUE;
>>> + else
>>> + monitor->enabled.pathloss = FALSE;
>>>
>>> DBG("Link Loss: %s, Path Loss: %s, FindMe: %s",
>>> monitor->enabled.linkloss ? "TRUE" : "FALSE",
>>> monitor->enabled.pathloss ? "TRUE" : "FALSE",
>>> monitor->enabled.findme ? "TRUE" : "FALSE");
>>>
>>> - if (monitor->enabled.linkloss || monitor->enabled.pathloss)
>>> - monitor->attioid = btd_device_add_attio_callback(device,
>>> + if (!monitor->enabled.linkloss && !monitor->enabled.pathloss)
>>> + return;
>>> +
>>> + if (monitor->attioid != 0)
>>> + return;
>>> +
>>> + monitor->attioid = btd_device_add_attio_callback(monitor->device,
>>> attio_connected_cb,
>>> attio_disconnected_cb,
>>> monitor);
>>> +}
>>> +
>>> +int monitor_register_linkloss(struct btd_device *device,
>>> + struct enabled *enabled,
>>> + struct gatt_primary *linkloss)
>>> +{
>>> + struct monitor *monitor;
>>> +
>>> + if (!enabled->linkloss)
>>> + return 0;
>>
>> Link loss requires immediate alert service, it is necessary to
>> investigate if make sense to add this verification here or check the
>> value before calling this function.
>> The same comment is valid for path loss(tx power).
>
> Regarding link-loss, please confirm that this dependency to immediate
> alert actually exists.
As mentioned previously, only path loss requires immediate alert.
>
> Regarding tx-power, as mentioned before, the motivation was the fact
> that, if the profiles get split, the exact order in which they are
> probed is undefined. Therefore, monitor_register_txpower() doesn't
> actually set enabled.pathloss to true, but instead delegates this work
> to update_monitor(), which should gracefully handle both possible
> probe combinations (immediate alert probed before or after txpower).
>
>>
>>> +
>>> + monitor = register_monitor(device);
>>> + if (monitor == NULL)
>>> + return -1;
>>> +
>>> + monitor->linkloss = g_new0(struct att_range, 1);
>>> + monitor->linkloss->start = linkloss->range.start;
>>> + monitor->linkloss->end = linkloss->range.end;
>>> + monitor->enabled.linkloss = TRUE;
>>> +
>>> + update_monitor(monitor);
>>>
>>> return 0;
>>> }
>>>
>>> -void monitor_unregister(struct btd_device *device)
>>> +int monitor_register_txpower(struct btd_device *device,
>>> + struct enabled *enabled,
>>> + struct gatt_primary *txpower)
>>> {
>>> + struct monitor *monitor;
>>> +
>>> + if (!enabled->pathloss)
>>> + return 0;
>>> +
>>> + monitor = register_monitor(device);
>>> + if (monitor == NULL)
>>> + return -1;
>>> +
>>> + monitor->txpower = g_new0(struct att_range, 1);
>>> + monitor->txpower->start = txpower->range.start;
>>> + monitor->txpower->end = txpower->range.end;
>>> +
>>> + update_monitor(monitor);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +int monitor_register_immediate(struct btd_device *device,
>>> + struct enabled *enabled,
>>> + struct gatt_primary *immediate)
>>> +{
>>> + struct monitor *monitor;
>>> +
>>> + if (!enabled->pathloss && !enabled->findme)
>>> + return 0;
>>> +
>>> + monitor = register_monitor(device);
>>> + if (monitor == NULL)
>>> + return -1;
>>> +
>>> + monitor->immediate = g_new0(struct att_range, 1);
>>> + monitor->immediate->start = immediate->range.start;
>>> + monitor->immediate->end = immediate->range.end;
>>> + monitor->enabled.findme = enabled->findme;
>>> +
>>> + update_monitor(monitor);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void cleanup_monitor(struct monitor *monitor)
>>> +{
>>> + struct btd_device *device = monitor->device;
>>> const char *path = device_get_path(device);
>>>
>>> + if (monitor->immediate != NULL || monitor->txpower != NULL)
>>> + return;
>>> +
>>> + if (monitor->immediateto != 0) {
>>> + g_source_remove(monitor->immediateto);
>>> + monitor->immediateto = 0;
>>> + }
>>> +
>>> + if (monitor->attioid != 0) {
>>> + btd_device_remove_attio_callback(device, monitor->attioid);
>>> + monitor->attioid = 0;
>>> + }
>>> +
>>> + if (monitor->attrib != NULL) {
>>> + g_attrib_unref(monitor->attrib);
>>> + monitor->attrib = NULL;
>>> + }
>>> +
>>> + if (monitor->linkloss != NULL)
>>> + return;
>>
>> Why this checking is here instead of check in the beginning of this function?
>
> My understanding of the previous code was that the cleanup above
> (before the linkloss check) belonged to immediate alert and txpower.
> Having a careful look at the code I realized that monitor->attioid and
> monitor->attrib should be moved to the end of the function, since
> linkloss makes use of them.
>
> Whether monitor->immediateto should also come after this check or not
> depends again on what the exact dependencies are.
Ok..
F.Y.I. the timeout is not even defined in the spec. We added this
harmless alert "reset" just because we noticed that it was required
for some implementations.
>
>>
>>> +
>>> g_dbus_unregister_interface(btd_get_dbus_connection(), path,
>>> PROXIMITY_INTERFACE);
>>> }
>>> +
>>> +void monitor_unregister_linkloss(struct btd_device *device)
>>> +{
>>> + struct monitor *monitor;
>>> +
>>> + monitor = find_monitor(device);
>>> + if (monitor == NULL)
>>> + return;
>>> +
>>> + g_free(monitor->linkloss);
>>> + monitor->linkloss = NULL;
>>> + monitor->enabled.linkloss = TRUE;
>>
>> If you are unregistering a service, is it necessary to set
>> enabled.linkloss = TRUE?
>
> My mistake, this should have been FALSE.
>
>>
>>> +
>>> + cleanup_monitor(monitor);
>>> +}
>>> +
>>> +void monitor_unregister_txpower(struct btd_device *device)
>>> +{
>>> + struct monitor *monitor;
>>> +
>>> + monitor = find_monitor(device);
>>> + if (monitor == NULL)
>>> + return;
>>> +
>>> + g_free(monitor->txpower);
>>> + monitor->txpower = NULL;
>>> + monitor->enabled.pathloss = FALSE;
>>
>> Same question here.
>
> The unregister function try to support partial unregistration to be
> consistent with the registration procedure, but perhaps this is
> overkill since typically all profiles will be removed at once.
>
> In fact, many other profiles simplify the unregistration by just
> removing more than one profile at the same time (see for example
> audio_device_unregister()). Any thoughts on this? Is such a
> simplification encouraged?
Maybe in the next round. It is better to keep your patch series small ;-)
Regards,
Claudio
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
