Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1360142187-15347-1-git-send-email-mikel.astiz.oss@gmail.com> <1360142187-15347-4-git-send-email-mikel.astiz.oss@gmail.com> Date: Fri, 15 Feb 2013 08:54:02 +0100 Message-ID: Subject: Re: [RFC v1 3/7] proximity: Split internal monitor registration API From: Mikel Astiz To: Claudio Takahasi Cc: linux-bluetooth@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Claudio, On Thu, Feb 14, 2013 at 5:37 PM, Claudio Takahasi wrote: > Hi Mikel: > > On Wed, Feb 6, 2013 at 6:16 AM, Mikel Astiz wrote: >> From: Mikel Astiz >> >> 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? 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 >> #include >> #include >> +#include >> >> #include >> >> @@ -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. 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. > >> + >> 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? > >> + >> + cleanup_monitor(monitor); >> +} >> + >> +void monitor_unregister_immediate(struct btd_device *device) >> +{ >> + struct monitor *monitor; >> + >> + monitor = find_monitor(device); >> + if (monitor == NULL) >> + return; >> + >> + g_free(monitor->immediate); >> + monitor->immediate = NULL; >> + monitor->enabled.findme = FALSE; >> + monitor->enabled.pathloss = FALSE; > > Same question here. > > > Regards, > Claudio Cheers, Mikel