Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201006171333.BlueZ.v6.1.I2830b9c1212a64b062201ed9f2b71294f50ad22d@changeid>
 <CABBYNZ+D=jO-6vO_XPQQ-1TQmfuOjXtiOm_+v8x+ZsyM=d3WRQ@mail.gmail.com> <CABmPvSEYSxS=PJxwyOv4YT=1xrYZ8m23OBN6NOG3mZLog3Z0MA@mail.gmail.com>
In-Reply-To: <CABmPvSEYSxS=PJxwyOv4YT=1xrYZ8m23OBN6NOG3mZLog3Z0MA@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 13 Oct 2020 16:17:22 -0700
Message-ID: <CABBYNZJcmQWvfvqrVA=0G7Hr7GbXGpYSn24UaUhdZ9f=ch87SQ@mail.gmail.com>
Subject: Re: [BlueZ PATCH v6 1/6] adv_monitor: Implement RSSI Filter logic for
 background scanning
To:     Miao-chen Chou <mcchou@chromium.org>
Cc:     Bluetooth Kernel Mailing List <linux-bluetooth@vger.kernel.org>,
        Luiz Augusto von Dentz <luiz.von.dentz@intel.com>,
        Howard Chung <howardchung@google.com>,
        ChromeOS Bluetooth Upstreaming 
        <chromeos-bluetooth-upstreaming@chromium.org>,
        Alain Michaud <alainm@chromium.org>,
        Marcel Holtmann <marcel@holtmann.org>,
        Manish Mandlik <mmandlik@chromium.org>,
        Manish Mandlik <mmandlik@google.com>,
        Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Miao-chen,

On Mon, Oct 12, 2020 at 2:21 PM Miao-chen Chou <mcchou@chromium.org> wrote:
>
> Hi Luiz,
>
> All the following changes were addressed in my local v7. I will wait
> for your feedback on the other thread and send as a series.
>
> On Tue, Oct 6, 2020 at 11:07 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Miao,
> >
> > On Tue, Oct 6, 2020 at 5:17 PM Miao-chen Chou <mcchou@chromium.org> wrote:
> > >
> > > From: Manish Mandlik <mmandlik@google.com>
> > >
> > > This patch implements the RSSI Filter logic for background scanning.
> > >
> > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > Reviewed-by: Alain Michaud <alainm@chromium.org>
> > > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> > > Reviewed-by: Howard Chung <howardchung@google.com>
> > > ---
> > >
> > > (no changes since v5)
> > >
> > > Changes in v5:
> > > - Remove the use of unit test in commit message
> > >
> > > Changes in v3:
> > > - Fix commit message
> > >
> > >  doc/advertisement-monitor-api.txt |   5 +
> > >  src/adapter.c                     |   1 +
> > >  src/adv_monitor.c                 | 286 +++++++++++++++++++++++++++++-
> > >  src/adv_monitor.h                 |   4 +
> > >  4 files changed, 292 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/doc/advertisement-monitor-api.txt b/doc/advertisement-monitor-api.txt
> > > index e09b6fd25..92c8ffc38 100644
> > > --- a/doc/advertisement-monitor-api.txt
> > > +++ b/doc/advertisement-monitor-api.txt
> > > @@ -70,6 +70,11 @@ Properties   string Type [read-only]
> > >                         dBm indicates unset. The valid range of a timer is 1 to
> > >                         300 seconds while 0 indicates unset.
> > >
> > > +                       If the peer device advertising interval is greater than the
> > > +                       HighRSSIThresholdTimer, the device will never be found. Similarly,
> > > +                       if it is greater than LowRSSIThresholdTimer, the device will be
> > > +                       considered as lost. Consider configuring these values accordingly.
> > > +
> > >                 array{(uint8, uint8, array{byte})} Patterns [read-only, optional]
> > >
> > >                         If Type is set to 0x01, this must exist and has at least
> > > diff --git a/src/adapter.c b/src/adapter.c
> > > index c0053000a..6d0114a6b 100644
> > > --- a/src/adapter.c
> > > +++ b/src/adapter.c
> > > @@ -1214,6 +1214,7 @@ void btd_adapter_remove_device(struct btd_adapter *adapter,
> > >         adapter->connect_list = g_slist_remove(adapter->connect_list, dev);
> > >
> > >         adapter->devices = g_slist_remove(adapter->devices, dev);
> > > +       btd_adv_monitor_device_remove(adapter->adv_monitor_manager, dev);
> > >
> > >         adapter->discovery_found = g_slist_remove(adapter->discovery_found,
> > >                                                                         dev);
> > > diff --git a/src/adv_monitor.c b/src/adv_monitor.c
> > > index e441a5566..31ed30a00 100644
> > > --- a/src/adv_monitor.c
> > > +++ b/src/adv_monitor.c
> > > @@ -26,6 +26,7 @@
> > >
> > >  #include "adapter.h"
> > >  #include "dbus-common.h"
> > > +#include "device.h"
> > >  #include "log.h"
> > >  #include "src/error.h"
> > >  #include "src/shared/ad.h"
> > > @@ -35,6 +36,8 @@
> > >
> > >  #include "adv_monitor.h"
> > >
> > > +static void monitor_device_free(void *data);
> > > +
> > >  #define ADV_MONITOR_INTERFACE          "org.bluez.AdvertisementMonitor1"
> > >  #define ADV_MONITOR_MGR_INTERFACE      "org.bluez.AdvertisementMonitorManager1"
> > >
> > > @@ -95,15 +98,36 @@ struct adv_monitor {
> > >
> > >         enum monitor_state state;       /* MONITOR_STATE_* */
> > >
> > > -       int8_t high_rssi;               /* high RSSI threshold */
> > > -       uint16_t high_rssi_timeout;     /* high RSSI threshold timeout */
> > > -       int8_t low_rssi;                /* low RSSI threshold */
> > > -       uint16_t low_rssi_timeout;      /* low RSSI threshold timeout */
> > > +       int8_t high_rssi;               /* High RSSI threshold */
> > > +       uint16_t high_rssi_timeout;     /* High RSSI threshold timeout */
> > > +       int8_t low_rssi;                /* Low RSSI threshold */
> > > +       uint16_t low_rssi_timeout;      /* Low RSSI threshold timeout */
> > > +       struct queue *devices;          /* List of adv_monitor_device objects */
> > >
> > >         enum monitor_type type;         /* MONITOR_TYPE_* */
> > >         struct queue *patterns;
> > >  };
> > >
> > > +/* Some data like last_seen, timer/timeout values need to be maintained
> > > + * per device. struct adv_monitor_device maintains such data.
> > > + */
> > > +struct adv_monitor_device {
> > > +       struct adv_monitor *monitor;
> > > +       struct btd_device *device;
> > > +
> > > +       time_t high_rssi_first_seen;    /* Start time when RSSI climbs above
> > > +                                        * the high RSSI threshold
> > > +                                        */
> > > +       time_t low_rssi_first_seen;     /* Start time when RSSI drops below
> > > +                                        * the low RSSI threshold
> > > +                                        */
> > > +       time_t last_seen;               /* Time when last Adv was received */
> > > +       bool device_found;              /* State of the device - lost/found */
> > > +       guint device_lost_timer;        /* Timer to track if the device goes
> > > +                                        * offline/out-of-range
> > > +                                        */
> >
> > I guess we could just drop the device_ term from the last 2 fields, it
> > should be implicit from the object itself that is already called
> > device.
> Done.
>
> >
> > > +};
> > > +
> > >  struct app_match_data {
> > >         const char *owner;
> > >         const char *path;
> > > @@ -150,6 +174,9 @@ static void monitor_free(void *data)
> > >         g_dbus_proxy_unref(monitor->proxy);
> > >         g_free(monitor->path);
> > >
> > > +       queue_destroy(monitor->devices, monitor_device_free);
> > > +       monitor->devices = NULL;
> > > +
> > >         queue_destroy(monitor->patterns, pattern_free);
> > >
> > >         free(monitor);
> > > @@ -248,6 +275,7 @@ static struct adv_monitor *monitor_new(struct adv_monitor_app *app,
> > >         monitor->high_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
> > >         monitor->low_rssi = ADV_MONITOR_UNSET_RSSI;
> > >         monitor->low_rssi_timeout = ADV_MONITOR_UNSET_TIMER;
> > > +       monitor->devices = queue_new();
> > >
> > >         monitor->type = MONITOR_TYPE_NONE;
> > >         monitor->patterns = NULL;
> > > @@ -923,3 +951,253 @@ void btd_adv_monitor_manager_destroy(struct btd_adv_monitor_manager *manager)
> > >
> > >         manager_destroy(manager);
> > >  }
> > > +
> > > +/* Matches a device based on btd_device object */
> > > +static bool monitor_device_match(const void *a, const void *b)
> > > +{
> > > +       const struct adv_monitor_device *dev = a;
> > > +       const struct btd_device *device = b;
> > > +
> > > +       if (!dev)
> > > +               return false;
> >
> > Checks like the above may hide bugs where NULL objects are being added
> > to the list, if that is happening then we probably need to fix it.
> Added error logs here and elsewhere.
>
> >
> > > +       if (dev->device != device)
> > > +               return false;
> > > +
> > > +       return true;
> > > +}
> > > +
> > > +/* Frees a monitor device object */
> > > +static void monitor_device_free(void *data)
> > > +{
> > > +       struct adv_monitor_device *dev = data;
> > > +
> > > +       if (!dev)
> > > +               return;
> >
> > Ditto.
> Done.
>
> >
> > > +       if (dev->device_lost_timer) {
> > > +               g_source_remove(dev->device_lost_timer);
> > > +               dev->device_lost_timer = 0;
> > > +       }
> > > +
> > > +       dev->monitor = NULL;
> > > +       dev->device = NULL;
> > > +
> > > +       g_free(dev);
> > > +}
> > > +
> > > +/* Removes a device from monitor->devices list */
> > > +static void remove_device_from_monitor(void *data, void *user_data)
> > > +{
> > > +       struct adv_monitor *monitor = data;
> > > +       struct btd_device *device = user_data;
> > > +       struct adv_monitor_device *dev = NULL;
> > > +
> > > +       if (!monitor)
> > > +               return;
> >
> > Ditto.
> Done.
>
> >
> > > +       dev = queue_remove_if(monitor->devices, monitor_device_match, device);
> > > +       if (dev) {
> > > +               DBG("Device removed from the Adv Monitor at path %s",
> > > +                   monitor->path);
> > > +               monitor_device_free(dev);
> > > +       }
> > > +}
> > > +
> > > +/* Removes a device from every monitor in an app */
> > > +static void remove_device_from_app(void *data, void *user_data)
> > > +{
> > > +       struct adv_monitor_app *app = data;
> > > +       struct btd_device *device = user_data;
> > > +
> > > +       if (!app)
> > > +               return;
> >
> > Ditto.
>
> >
> > > +       queue_foreach(app->monitors, remove_device_from_monitor, device);
> > > +}
> > > +
> > > +/* Removes a device from every monitor in all apps */
> > > +void btd_adv_monitor_device_remove(struct btd_adv_monitor_manager *manager,
> > > +                                  struct btd_device *device)
> > > +{
> > > +       if (!manager || !device)
> > > +               return;
> > > +
> > > +       queue_foreach(manager->apps, remove_device_from_app, device);
> > > +}
> > > +
> > > +/* Creates a device object to track the per-device information */
> > > +static struct adv_monitor_device *monitor_device_create(
> > > +                       struct adv_monitor *monitor,
> > > +                       struct btd_device *device)
> > > +{
> > > +       struct adv_monitor_device *dev = NULL;
> > > +
> > > +       dev = g_try_malloc0(sizeof(struct adv_monitor_device));
> >
> > Please use new0 on new code.
> Done.
>
> >
> > > +       if (!dev)
> > > +               return NULL;
> > > +
> > > +       dev->monitor = monitor;
> > > +       dev->device = device;
> > > +
> > > +       queue_push_tail(monitor->devices, dev);
> > > +
> > > +       return dev;
> > > +}
> > > +
> > > +/* Includes found/lost device's object path into the dbus message */
> > > +static void report_device_state_setup(DBusMessageIter *iter, void *user_data)
> > > +{
> > > +       const char *path = device_get_path(user_data);
> > > +
> > > +       dbus_message_iter_append_basic(iter, DBUS_TYPE_OBJECT_PATH, &path);
> > > +}
> > > +
> > > +/* Handles a situation where the device goes offline/out-of-range */
> > > +static gboolean handle_device_lost_timeout(gpointer user_data)
> > > +{
> > > +       struct adv_monitor_device *dev = user_data;
> > > +       struct adv_monitor *monitor = dev->monitor;
> > > +       time_t curr_time = time(NULL);
> > > +
> > > +       DBG("Device Lost timeout triggered for device %p "
> > > +           "for the Adv Monitor at path %s", dev->device, monitor->path);
> > > +
> > > +       dev->device_lost_timer = 0;
> > > +
> > > +       if (dev->device_found && dev->last_seen) {
> > > +               /* We were tracking for the Low RSSI filter. Check if there is
> > > +                * any Adv received after the timeout function is invoked.
> > > +                * If not, report the Device Lost event.
> > > +                */
> > > +               if (difftime(curr_time, dev->last_seen) >=
> > > +                   monitor->low_rssi_timeout) {
> > > +                       dev->device_found = false;
> > > +
> > > +                       DBG("Calling DeviceLost() on Adv Monitor of owner %s "
> > > +                           "at path %s", monitor->app->owner, monitor->path);
> > > +
> > > +                       g_dbus_proxy_method_call(monitor->proxy, "DeviceLost",
> > > +                                                report_device_state_setup,
> > > +                                                NULL, dev->device, NULL);
> > > +               }
> > > +       }
> > > +
> > > +       return FALSE;
> > > +}
> > > +
> > > +/* Filters an Adv based on its RSSI value */
> > > +static void adv_monitor_filter_rssi(struct adv_monitor *monitor,
> > > +                                   struct btd_device *device, int8_t rssi)
> > > +{
> > > +       struct adv_monitor_device *dev = NULL;
> > > +       time_t curr_time = time(NULL);
> > > +       uint16_t adapter_id = monitor->app->manager->adapter_id;
> > > +
> > > +       /* If the RSSI thresholds and timeouts are not specified, report the
> > > +        * DeviceFound() event without tracking for the RSSI as the Adv has
> > > +        * already matched the pattern filter.
> > > +        */
> > > +       if (monitor->high_rssi == ADV_MONITOR_UNSET_RSSI &&
> > > +               monitor->low_rssi == ADV_MONITOR_UNSET_RSSI &&
> > > +               monitor->high_rssi_timeout == ADV_MONITOR_UNSET_TIMER &&
> > > +               monitor->low_rssi_timeout == ADV_MONITOR_UNSET_TIMER) {
> > > +               DBG("Calling DeviceFound() on Adv Monitor of owner %s "
> > > +                   "at path %s", monitor->app->owner, monitor->path);
> > > +
> > > +               g_dbus_proxy_method_call(monitor->proxy, "DeviceFound",
> > > +                                        report_device_state_setup, NULL,
> > > +                                        device, NULL);
> > > +
> > > +               return;
> > > +       }
> > > +
> > > +       dev = queue_find(monitor->devices, monitor_device_match, device);
> > > +       if (!dev)
> > > +               dev = monitor_device_create(monitor, device);
> >
> > There seems to be missing indentation here as the following if
> > statement should be nested otherwise the dev point is always evaluated
> > twice which is not optimal.
> Done.
>
> >
> > > +       if (!dev) {
> > > +               btd_error(adapter_id, "Failed to create Adv Monitor "
> > > +                                     "device object.");
> > > +               return;
> > > +       }
> >
> > Id put the code above into a function e.g. monitor_device_get so
> > whenever you need to find + create logic you can just use it.
> This is the only case of find + create logic, so I guess that the
> author intentionally leaves it as it is.

Fair enough, I thought Ive seen some other users of the find + create
logic but if it is just one let leave at that.

>
> >
> > > +
> > > +       if (dev->device_lost_timer) {
> > > +               g_source_remove(dev->device_lost_timer);
> > > +               dev->device_lost_timer = 0;
> > > +       }
> > > +
> > > +       /* Reset the timings of found/lost if a device has been offline for
> > > +        * longer than the high/low timeouts.
> > > +        */
> > > +       if (dev->last_seen) {
> > > +               if (difftime(curr_time, dev->last_seen) >
> > > +                   monitor->high_rssi_timeout) {
> > > +                       dev->high_rssi_first_seen = 0;
> > > +               }
> > > +
> > > +               if (difftime(curr_time, dev->last_seen) >
> > > +                   monitor->low_rssi_timeout) {
> > > +                       dev->low_rssi_first_seen = 0;
> > > +               }
> > > +       }
> > > +       dev->last_seen = curr_time;
> > > +
> > > +       /* Check for the found devices (if the device is not already found) */
> > > +       if (!dev->device_found && rssi > monitor->high_rssi) {
> > > +               if (dev->high_rssi_first_seen) {
> > > +                       if (difftime(curr_time, dev->high_rssi_first_seen) >=
> > > +                           monitor->high_rssi_timeout) {
> > > +                               dev->device_found = true;
> > > +
> > > +                               DBG("Calling DeviceFound() on Adv Monitor "
> > > +                                   "of owner %s at path %s",
> > > +                                   monitor->app->owner, monitor->path);
> > > +
> > > +                               g_dbus_proxy_method_call(
> > > +                                       monitor->proxy, "DeviceFound",
> > > +                                       report_device_state_setup, NULL,
> > > +                                       dev->device, NULL);
> > > +                       }
> > > +               } else {
> > > +                       dev->high_rssi_first_seen = curr_time;
> > > +               }
> > > +       } else {
> > > +               dev->high_rssi_first_seen = 0;
> > > +       }
> > > +
> > > +       /* Check for the lost devices (only if the device is already found, as
> > > +        * it doesn't make any sense to report the Device Lost event if the
> > > +        * device is not found yet)
> > > +        */
> > > +       if (dev->device_found && rssi < monitor->low_rssi) {
> > > +               if (dev->low_rssi_first_seen) {
> > > +                       if (difftime(curr_time, dev->low_rssi_first_seen) >=
> > > +                           monitor->low_rssi_timeout) {
> > > +                               dev->device_found = false;
> > > +
> > > +                               DBG("Calling DeviceLost() on Adv Monitor "
> > > +                                   "of owner %s at path %s",
> > > +                                   monitor->app->owner, monitor->path);
> > > +
> > > +                               g_dbus_proxy_method_call(
> > > +                                       monitor->proxy, "DeviceLost",
> > > +                                       report_device_state_setup, NULL,
> > > +                                       dev->device, NULL);
> > > +                       }
> > > +               } else {
> > > +                       dev->low_rssi_first_seen = curr_time;
> > > +               }
> > > +       } else {
> > > +               dev->low_rssi_first_seen = 0;
> > > +       }
> > > +
> > > +       /* Setup a timer to track if the device goes offline/out-of-range, only
> > > +        * if we are tracking for the Low RSSI Threshold. If we are tracking
> > > +        * the High RSSI Threshold, nothing needs to be done.
> > > +        */
> > > +       if (dev->device_found) {
> > > +               dev->device_lost_timer =
> > > +                       g_timeout_add_seconds(monitor->low_rssi_timeout,
> > > +                                             handle_device_lost_timeout, dev);
> > > +       }
> > > +}
> > > diff --git a/src/adv_monitor.h b/src/adv_monitor.h
> > > index 5cb372217..13d5d7282 100644
> > > --- a/src/adv_monitor.h
> > > +++ b/src/adv_monitor.h
> > > @@ -12,6 +12,7 @@
> > >  #define __ADV_MONITOR_H
> > >
> > >  struct mgmt;
> > > +struct btd_device;
> > >  struct btd_adapter;
> > >  struct btd_adv_monitor_manager;
> > >
> > > @@ -20,4 +21,7 @@ struct btd_adv_monitor_manager *btd_adv_monitor_manager_create(
> > >                                                 struct mgmt *mgmt);
> > >  void btd_adv_monitor_manager_destroy(struct btd_adv_monitor_manager *manager);
> > >
> > > +void btd_adv_monitor_device_remove(struct btd_adv_monitor_manager *manager,
> > > +                                  struct btd_device *device);
> > > +
> > >  #endif /* __ADV_MONITOR_H */
> > > --
> > > 2.26.2
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> Regards,
> Miao


-- 
Luiz Augusto von Dentz