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

[Jim Hodapp]

Review: Needs Fixing

Some suggestions inline below.

Diff comments:

> === modified file 'src/location_service/com/ubuntu/location/CMakeLists.txt'
> --- src/location_service/com/ubuntu/location/CMakeLists.txt	2015-01-25 12:45:30 +0000
> +++ src/location_service/com/ubuntu/location/CMakeLists.txt	2015-04-23 17:22:36 +0000
> @@ -27,6 +27,7 @@
>    proxy_provider.cpp
>    satellite_based_positioning_state.cpp
>    settings.cpp
> +  time_based_update_policy.cpp
>    set_name_for_thread.cpp
>    wifi_and_cell_reporting_state.cpp
>  
> 
> === modified file 'src/location_service/com/ubuntu/location/engine.cpp'
> --- src/location_service/com/ubuntu/location/engine.cpp	2015-01-25 12:44:20 +0000
> +++ src/location_service/com/ubuntu/location/engine.cpp	2015-04-23 17:22:36 +0000
> @@ -24,6 +24,8 @@
>  #include <stdexcept>
>  #include <unordered_map>
>  
> +#include "time_based_update_policy.h"
> +
>  namespace cul = com::ubuntu::location;
>  
>  const cul::SatelliteBasedPositioningState cul::Engine::Configuration::Defaults::satellite_based_positioning_state;
> @@ -33,7 +35,8 @@
>  cul::Engine::Engine(const cul::ProviderSelectionPolicy::Ptr& provider_selection_policy,
>                      const cul::Settings::Ptr& settings)
>            : provider_selection_policy(provider_selection_policy),
> -            settings(settings)
> +            settings(settings),
> +            update_policy(std::make_shared<cul::TimeBasedUpdatePolicy>())
>  {
>      if (!provider_selection_policy) throw std::runtime_error
>      {
> @@ -204,7 +207,7 @@
>      // We should come up with a better heuristic here.
>      auto cpr = provider->updates().position.connect([this](const cul::Update<cul::Position>& src)
>      {
> -        updates.reference_location = src;
> +        updates.reference_location = update_policy->verify_update(src);
>      });
>  
>      std::lock_guard<std::mutex> lg(guard);
> 
> === modified file 'src/location_service/com/ubuntu/location/engine.h'
> --- src/location_service/com/ubuntu/location/engine.h	2015-01-14 13:41:54 +0000
> +++ src/location_service/com/ubuntu/location/engine.h	2015-04-23 17:22:36 +0000
> @@ -33,6 +33,8 @@
>  #include <mutex>
>  #include <set>
>  
> +#include "update_policy.h"
> +
>  namespace com
>  {
>  namespace ubuntu
> @@ -193,6 +195,7 @@
>      std::map<Provider::Ptr, ProviderConnections> providers;
>      ProviderSelectionPolicy::Ptr provider_selection_policy;
>      Settings::Ptr settings;
> +    UpdatePolicy::Ptr update_policy;
>  };
>  
>  /** @brief Pretty prints the given status to the given stream. */
> 
> === added file 'src/location_service/com/ubuntu/location/time_based_update_policy.cpp'
> --- src/location_service/com/ubuntu/location/time_based_update_policy.cpp	1970-01-01 00:00:00 +0000
> +++ src/location_service/com/ubuntu/location/time_based_update_policy.cpp	2015-04-23 17:22:36 +0000
> @@ -0,0 +1,119 @@
> +/*
> + * Copyright © 2015 Canonical Ltd.
> + *
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License version 3,
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Authored by: Manuel de la Pena <manuel.delapena at canonical.com>
> + */
> +
> +#include "time_based_update_policy.h"
> +
> +namespace com
> +{
> +namespace ubuntu
> +{
> +namespace location
> +{
> +
> +std::chrono::minutes TimeBasedUpdatePolicy::default_timeout()
> +{
> +    static std::chrono::minutes default_limit(2);

add const

> +    return default_limit;
> +}
> +
> +TimeBasedUpdatePolicy::TimeBasedUpdatePolicy(std::chrono::minutes mins)
> +    : limit(mins)
> +{
> +
> +}
> +
> +const location::Update<location::Position>& TimeBasedUpdatePolicy::verify_update(const location::Update<location::Position>& update)
> +{
> +    std::lock_guard<std::mutex> guard(position_update_mutex);
> +    bool use_new_update;
> +    if (is_significantly_newer(last_position_update, update, limit))
> +    {
> +        use_new_update = true;
> +    }
> +    else if (is_significantly_older(last_position_update, update, limit))
> +    {
> +        use_new_update = false;
> +    }
> +    else
> +    {
> +        use_new_update = last_position_update.value.accuracy.horizontal && update.value.accuracy.horizontal

Add a comment here describing in simple wording what this is trying to determine (this will add clarity for someone not familiar with this codebase).

> +            && *last_position_update.value.accuracy.horizontal >= *update.value.accuracy.horizontal;
> +    }
> +
> +    if (use_new_update)
> +    {
> +        last_position_update = update;
> +        return update;
> +    }
> +    else
> +    {
> +        return last_position_update;
> +    }
> +}
> +
> +
> +const location::Update<location::Heading>& TimeBasedUpdatePolicy::verify_update(const location::Update<location::Heading>& update)
> +{
> +    std::lock_guard<std::mutex> guard(heading_update_mutex);
> +    bool use_new_update;
> +    if (is_significantly_newer(last_heading_update, update, limit))
> +    {
> +        use_new_update = true;
> +    }
> +    else if (is_significantly_older(last_heading_update, update, limit))
> +    {
> +        use_new_update = false;
> +    }
> +    if (use_new_update)
> +    {
> +        last_heading_update = update;
> +        return update;
> +    }
> +    else
> +    {
> +        return last_heading_update;
> +    }
> +}
> +
> +const location::Update<location::Velocity>& TimeBasedUpdatePolicy::verify_update(const location::Update<location::Velocity>& update)
> +{
> +    std::lock_guard<std::mutex> guard(velocity_update_mutex);
> +    bool use_new_update;
> +    if (is_significantly_newer(last_velocity_update, update, limit))
> +    {
> +        use_new_update = true;
> +    }
> +    else if (is_significantly_older(last_velocity_update, update, limit))
> +    {
> +        use_new_update = false;
> +    }
> +
> +    if (use_new_update)
> +    {
> +        last_velocity_update = update;
> +        return update;
> +    }
> +    else
> +    {
> +        return last_velocity_update;
> +    }
> +}
> +
> +}
> +}
> +}
> \ No newline at end of file
> 
> === added file 'src/location_service/com/ubuntu/location/time_based_update_policy.h'
> --- src/location_service/com/ubuntu/location/time_based_update_policy.h	1970-01-01 00:00:00 +0000
> +++ src/location_service/com/ubuntu/location/time_based_update_policy.h	2015-04-23 17:22:36 +0000
> @@ -0,0 +1,71 @@
> +/*
> + * Copyright © 2015 Canonical Ltd.
> + *
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License version 3,
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Authored by: Manuel de la Pena <manuel.delapena at canonical.com>
> + */
> +#ifndef LOCATION_SERVICE_UBUNTU_LOCATION_SERVICE_TIMES_BASED_UPDATE_POLICY_H_
> +#define LOCATION_SERVICE_UBUNTU_LOCATION_SERVICE_TIMES_BASED_UPDATE_POLICY_H_
> +
> +#include <chrono>
> +#include <mutex>
> +
> +#include "update_policy.h"
> +
> +namespace com
> +{
> +namespace ubuntu
> +{
> +namespace location
> +{
> +
> +// An interface that can be implemented to add heuristics on how update will be chosen. This class

Change it be "heading, position or velocity update" instead of "update" for increased clarity.

> +// allows developers to inject different heuristics in the engine to perform the update selection
> +// so that the app developers can take advantage of it.
> +class TimeBasedUpdatePolicy : public UpdatePolicy {
> +
> + public:
> +    TimeBasedUpdatePolicy(std::chrono::minutes mins=default_timeout());
> +    TimeBasedUpdatePolicy(const TimeBasedUpdatePolicy&) = delete;
> +    ~TimeBasedUpdatePolicy() = default;
> +
> +    // Return if the given position update will be verifyed as the new position in the engine.

type: should be "verified"

> +    const location::Update<location::Position>& verify_update(const location::Update<location::Position>& update) override;
> +    // Return if the given heading update will be verifyed as the new heading in the engine.

type: should be "verified"

> +    const location::Update<location::Heading>& verify_update(const location::Update<location::Heading>& update) override;
> +    // Return if the given velocity update will be verifyed as the new velocity in the engine.

type: should be "verified"

> +    const location::Update<location::Velocity>& verify_update(const location::Update<location::Velocity>& update) override;
> +
> +    static std::chrono::minutes default_timeout();
> +
> + protected:
> +    // not private to simplify the testing but should be private

Can you make the specific testing classes be friends with this class to allow for private variables?

> +    location::Update<location::Position> last_position_update;
> +    location::Update<location::Heading> last_heading_update;
> +    location::Update<location::Velocity> last_velocity_update;
> +
> + private:
> +    // callbacks can happen in diff threads, make sure multi-threading will work in this class
> +    std::mutex position_update_mutex;
> +    std::mutex heading_update_mutex;
> +    std::mutex velocity_update_mutex;
> +    // used to calculate the time accepted bracket
> +    std::chrono::minutes limit;
> +};
> +
> +}
> +}
> +}
> +
> +#endif //LOCATION_SERVICE_UBUNTU_LOCATION_SERVICE_TIMES_BASED_UPDATE_POLICY_H_
> 
> === added file 'src/location_service/com/ubuntu/location/update_policy.h'
> --- src/location_service/com/ubuntu/location/update_policy.h	1970-01-01 00:00:00 +0000
> +++ src/location_service/com/ubuntu/location/update_policy.h	2015-04-23 17:22:36 +0000
> @@ -0,0 +1,75 @@
> +/*
> + * Copyright © 2015 Canonical Ltd.
> + *
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License version 3,
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Authored by: Manuel de la Pena <manuel.delapena at canonical.com>
> + */
> +#ifndef LOCATION_SERVICE_UBUNTU_LOCATION_SERVICE_UPDATE_POLICY_H_
> +#define LOCATION_SERVICE_UBUNTU_LOCATION_SERVICE_UPDATE_POLICY_H_
> +
> +#include <memory>
> +
> +#include <com/ubuntu/location/heading.h>
> +#include <com/ubuntu/location/position.h>
> +#include <com/ubuntu/location/update.h>
> +#include <com/ubuntu/location/velocity.h>
> +
> +namespace com
> +{
> +namespace ubuntu
> +{
> +namespace location
> +{
> +
> +// An interface that can be implemented to add heuristics on how update will be chosen. This class

Change it be "heading, position or velocity update" instead of "update" for increased clarity.

> +// allows developers to inject different heuristics in the engine to perform the update selection
> +// so that the app developers can take advantage of it.
> +class UpdatePolicy {
> + public:
> +    typedef std::shared_ptr<UpdatePolicy> Ptr;
> +
> +    UpdatePolicy(const UpdatePolicy&) = delete;
> +    UpdatePolicy(UpdatePolicy&&) = delete;
> +    UpdatePolicy& operator=(const UpdatePolicy&) = delete;
> +    virtual ~UpdatePolicy() = default;
> +
> +    // Return if the given position update will be verified as the new position in the engine.
> +    virtual const location::Update<location::Position>& verify_update(const location::Update<location::Position>& update) = 0;
> +    // Return if the given heading update will be verified as the new heading in the engine.
> +    virtual const location::Update<location::Heading>& verify_update(const location::Update<location::Heading>& update) = 0;
> +    // Return if the given velocity update will be verified as the new velocity in the engine.
> +    virtual const location::Update<location::Velocity>& verify_update(const location::Update<location::Velocity>& update) = 0;
> + protected:
> +    UpdatePolicy() = default;
> +
> +    template <class T> bool is_significantly_newer(const location::Update<T> last_update, const location::Update<T> update, std::chrono::minutes limit)
> +    {
> +       auto delta = update.when - last_update.when;

add const

> +       return delta > limit;
> +    }
> +
> +    template <class T> bool is_significantly_older(const location::Update<T> last_update, const location::Update<T> update, std::chrono::minutes limit)
> +    {
> +       auto delta = update.when - last_update.when;

add const

> +       return delta < (-1 * limit);
> +    }
> +
> +
> +};
> +}
> +}
> +}
> +
> +#endif // LOCATION_SERVICE_UBUNTU_LOCATION_SERVICE_UPDATE_POLICY_H_
> +
> 
> === modified file 'tests/CMakeLists.txt'
> --- tests/CMakeLists.txt	2014-11-14 11:26:45 +0000
> +++ tests/CMakeLists.txt	2015-04-23 17:22:36 +0000
> @@ -80,6 +80,7 @@
>  LOCATION_SERVICE_ADD_TEST(engine_test engine_test.cpp)
>  LOCATION_SERVICE_ADD_TEST(harvester_test harvester_test.cpp)
>  LOCATION_SERVICE_ADD_TEST(demultiplexing_reporter_test demultiplexing_reporter_test.cpp)
> +LOCATION_SERVICE_ADD_TEST(time_based_update_policy_test time_based_update_policy_test.cpp)
>  
>  if (NET_CPP_FOUND)
>    LOCATION_SERVICE_ADD_TEST(ichnaea_reporter_test ichnaea_reporter_test.cpp)
> 
> === added file 'tests/time_based_update_policy_test.cpp'
> --- tests/time_based_update_policy_test.cpp	1970-01-01 00:00:00 +0000
> +++ tests/time_based_update_policy_test.cpp	2015-04-23 17:22:36 +0000
> @@ -0,0 +1,131 @@
> +/*
> + * Copyright © 2015 Canonical Ltd.
> + *
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License version 3,
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Authored by: Manuel de la Pena <manuel.delapena at canonical.com>
> + */
> +#include <com/ubuntu/location/time_based_update_policy.h>
> +
> +#include <gtest/gtest.h>
> +
> +using namespace ::testing;
> +namespace cul = com::ubuntu::location;
> +
> +namespace
> +{
> +    auto timestamp = com::ubuntu::location::Clock::now();
> +
> +    com::ubuntu::location::Update<com::ubuntu::location::Position> reference_position_update
> +            {
> +                    {
> +                            com::ubuntu::location::wgs84::Latitude{9. * com::ubuntu::location::units::Degrees},
> +                            com::ubuntu::location::wgs84::Longitude{53. * com::ubuntu::location::units::Degrees},
> +                            com::ubuntu::location::wgs84::Altitude{-2. * com::ubuntu::location::units::Meters},
> +                    },
> +                    timestamp
> +            };
> +}
> +
> +// make certain internal details public so that we can set the last update
> +class PublicTimeBasedUpdatePolicy : public cul::TimeBasedUpdatePolicy {
> + public:
> +    PublicTimeBasedUpdatePolicy(std::chrono::minutes mins) :  cul::TimeBasedUpdatePolicy(mins) {}
> +    using cul::TimeBasedUpdatePolicy::last_position_update;
> +    using cul::TimeBasedUpdatePolicy::last_heading_update;
> +    using cul::TimeBasedUpdatePolicy::last_velocity_update;
> +};
> +
> +TEST(TimeBasedUpdatePolicy, policy_ignores_updates_that_are_too_old)
> +{
> +    auto policy = std::make_shared<PublicTimeBasedUpdatePolicy>(std::chrono::minutes(2));
> +    policy->last_position_update = reference_position_update;
> +
> +    com::ubuntu::location::Update<com::ubuntu::location::Position> old_update
> +            {
> +                    {
> +                            com::ubuntu::location::wgs84::Latitude{10. * com::ubuntu::location::units::Degrees},
> +                            com::ubuntu::location::wgs84::Longitude{60. * com::ubuntu::location::units::Degrees},
> +                            com::ubuntu::location::wgs84::Altitude{10. * com::ubuntu::location::units::Meters}
> +                    },
> +                    timestamp - std::chrono::minutes(5)
> +            };
> +    policy->verify_update(old_update);
> +
> +    ASSERT_NE(policy->last_position_update.value.latitude, old_update.value.latitude);
> +    ASSERT_EQ(policy->last_position_update.value.latitude, reference_position_update.value.latitude);
> +
> +    ASSERT_NE(policy->last_position_update.value.longitude, old_update.value.longitude);
> +    ASSERT_EQ(policy->last_position_update.value.longitude, reference_position_update.value.longitude);
> +
> +    ASSERT_NE(policy->last_position_update.value.altitude, old_update.value.altitude);
> +    ASSERT_EQ(policy->last_position_update.value.altitude, reference_position_update.value.altitude);
> +}
> +
> +TEST(TimeBasedUpdatePolicy, policy_uses_very_recent_updates)
> +{
> +    auto policy = std::make_shared<PublicTimeBasedUpdatePolicy>(std::chrono::minutes(2));
> +
> +    policy->last_position_update = reference_position_update;
> +
> +    com::ubuntu::location::Update<com::ubuntu::location::Position> new_update
> +            {
> +                    {
> +                            com::ubuntu::location::wgs84::Latitude{10. * com::ubuntu::location::units::Degrees},
> +                            com::ubuntu::location::wgs84::Longitude{60. * com::ubuntu::location::units::Degrees},
> +                            com::ubuntu::location::wgs84::Altitude{10. * com::ubuntu::location::units::Meters}
> +                    },
> +                    timestamp + std::chrono::minutes(3)
> +            };
> +
> +    policy->verify_update(new_update);
> +
> +    ASSERT_EQ(policy->last_position_update.value.latitude, new_update.value.latitude);
> +    ASSERT_NE(policy->last_position_update.value.latitude, reference_position_update.value.latitude);
> +
> +    ASSERT_EQ(policy->last_position_update.value.longitude, new_update.value.longitude);
> +    ASSERT_NE(policy->last_position_update.value.longitude, reference_position_update.value.longitude);
> +
> +    ASSERT_EQ(policy->last_position_update.value.altitude, new_update.value.altitude);
> +    ASSERT_NE(policy->last_position_update.value.altitude, reference_position_update.value.altitude);
> +}
> +
> +TEST(TimeBasedUpdatePolicy, policy_ignores_inaccurate_updates)
> +{
> +    auto policy = std::make_shared<PublicTimeBasedUpdatePolicy>(std::chrono::minutes(2));
> +    reference_position_update.value.accuracy.horizontal = 1. * com::ubuntu::location::units::Meters;
> +    policy->last_position_update = reference_position_update;
> +
> +    com::ubuntu::location::Update<com::ubuntu::location::Position> new_update
> +            {
> +                    {
> +                            com::ubuntu::location::wgs84::Latitude{10. * com::ubuntu::location::units::Degrees},
> +                            com::ubuntu::location::wgs84::Longitude{60. * com::ubuntu::location::units::Degrees},
> +                            com::ubuntu::location::wgs84::Altitude{10. * com::ubuntu::location::units::Meters},
> +                    },
> +                    timestamp + std::chrono::minutes(1)
> +            };
> +    new_update.value.accuracy.horizontal = 8. * com::ubuntu::location::units::Meters;
> +
> +    policy->verify_update(new_update);
> +    ASSERT_TRUE(*new_update.value.accuracy.horizontal > *reference_position_update.value.accuracy.horizontal);
> +
> +    ASSERT_NE(policy->last_position_update.value.latitude, new_update.value.latitude);
> +    ASSERT_EQ(policy->last_position_update.value.latitude, reference_position_update.value.latitude);
> +
> +    ASSERT_NE(policy->last_position_update.value.longitude, new_update.value.longitude);
> +    ASSERT_EQ(policy->last_position_update.value.longitude, reference_position_update.value.longitude);
> +
> +    ASSERT_NE(policy->last_position_update.value.altitude, new_update.value.altitude);
> +    ASSERT_EQ(policy->last_position_update.value.altitude, reference_position_update.value.altitude);
> +}
> 


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