[Merge] lp:~mandel/location-service/better-position-selection into lp:location-service

[Thomas Voß]

Review: Needs Fixing

Thanks for the changes, a high-level remark first: The ProviderSelectionStrategy implementation is not the right place to add this functionality. Instead, the functionality should be added here:

  http://bazaar.launchpad.net/~phablet-team/location-service/trunk/view/head:/src/location_service/com/ubuntu/location/engine.cpp#L205

location::Engine is the central hub receiving and dispatching updates and it is meant to carry out such tasks as proposed in this MP.

Also make sure that an automated test case is added in http://bazaar.launchpad.net/~phablet-team/location-service/trunk/view/head:/tests/engine_test.cpp. I would think that introducing a location::PositionUpdateFilter interface together with a default implementation should help a lot here. You can pass the location::PositionUpdateFilter to the location::Engine constructor as a functional dependency and subsequently test if the Engine calls into that object for position updates.

Rest of my comments inline.

Diff comments:

> === modified file 'src/location_service/com/ubuntu/location/non_selecting_provider_selection_policy.cpp'
> --- src/location_service/com/ubuntu/location/non_selecting_provider_selection_policy.cpp	2014-10-28 18:28:29 +0000
> +++ src/location_service/com/ubuntu/location/non_selecting_provider_selection_policy.cpp	2015-04-22 08:44:52 +0000
> @@ -16,10 +16,13 @@
>   * Authored by: Thomas Voß <thomas.voss at canonical.com>
>   */
>  
> +
> +#include <mutex>
> +
> +#include <com/ubuntu/location/logging.h>
> +#include <com/ubuntu/location/optional.h>
>  #include <com/ubuntu/location/non_selecting_provider_selection_policy.h>
>  
> -#include <set>
> -
>  namespace location = com::ubuntu::location;
>  
>  namespace
> @@ -53,7 +56,23 @@
>          {
>              connections.push_back(provider->updates().position.connect([this](const location::Update<location::Position>& update)
>              {
> -                mutable_updates().position(update);
> +                std::lock_guard<std::mutex> lock(position_update_mutex);
> +                auto data = update.value;
> +                LOG(INFO) << "Update recived from " << std::chrono::system_clock::to_time_t(update.when) << " with the following data:\n"

Please make this VLOG(10). Otherwise, we will see a lot of log spam.

> +                    << "longitud:\t" << data.longitude  << "\nlatitude:\t" << data.latitude  << "\naltidue:\t"
> +                    << data.altitude;
> +                if (is_better_position_update(update))
> +                {
> +                    LOG(INFO) << "Updating position";
> +                    last_position_update = update;
> +                    mutable_updates().position(update);
> +                }
> +                else
> +                {
> +                    // use the old location to mimic that we are getting updates at a decent rate
> +                    LOG(INFO) << "Sending old update";
> +                    mutable_updates().position(*last_position_update);
> +                }
>              }));
>  
>              connections.push_back(provider->updates().heading.connect([this](const location::Update<location::Heading>& update)
> @@ -69,6 +88,34 @@
>  
>      }
>  
> +    // decide if the update is better than the last one sent
> +    bool is_better_position_update(const location::Update<location::Position>& update)
> +    {
> +        if (!last_position_update)

I don't think we have to rely on an Optional value here. Instead, the default constructed Update<Position> instance should be good enough. Its timestamp is initialized to the beginning of time (and with that older than any other update), and its horizontal accuracy should be +inf. With that, the algorithm can easily work without the Optional and the special case.

> +        {
> +            // first update, ergo is better
> +            return true;
> +        }
> +
> +        std::chrono::minutes limit(2);

Please make this parameter an argument to the filter as proposed in the initial comment.

> +        auto timeDelta = update.when - last_position_update->when;
> +        auto isSignificantlyNewer = timeDelta > limit;
> +        auto isSignificantlyOlder = timeDelta < (-1 * limit);

Not sure I understand the intention here. As far as I can see, timeDelta is always positive.

> +
> +        // if the time diff is bigger than 2 mins, we are more interested in the new one, else the old one is better
> +        if (isSignificantlyNewer) {

Hmmm, so we potentially discard updates from high-frequency providers if the last update is not older than 'limit'? Not sure I understand the reasoning here, specifically in the case of doing positioning with a high velocity, e.g., in a car.

> +            return true;
> +        } else if (isSignificantlyOlder) {
> +            return false;
> +        }
> +
> +        // we are within a time range where we can get a valid update that is more accurate
> +        // TODO: Track the provider so that we just accept the GPS one before

We should not rely on knowing the actual provider here. The only required inputs to the algorithm are recency and accuracy, potentially an estimate of the overall update frequency for a given provider. But that's pretty much it.

> +        return last_position_update->value.accuracy.horizontal && update.value.accuracy.horizontal
> +            && *last_position_update->value.accuracy.horizontal >= *update.value.accuracy.horizontal;
> +
> +    }
> +
>      // We always match :)
>      bool matches_criteria(const location::Criteria&) override
>      {
> @@ -138,6 +185,11 @@
>  
>      std::set<location::Provider::Ptr> providers;
>      std::vector<core::ScopedConnection> connections;
> +
> + private:
> +    // keep track of the latests sent update
> +    location::Optional<location::Update<location::Position>> last_position_update;
> +    std::mutex position_update_mutex;
>  };
>  }
>  
> 


-- 
https://code.launchpad.net/~mandel/location-service/better-position-selection/+merge/257037
Your team Ubuntu Phablet Team is subscribed to branch lp:location-service.