[Merge] lp:~mandel/location-service/tvoss-time-delta-fixes into lp:location-service
Jim Hodapp
jim.hodapp at canonical.com
Mon Apr 27 21:39:41 UTC 2015
Review: Needs Fixing code
One minor change inline below.
Diff comments:
> === modified file 'examples/service/service.cpp'
> --- examples/service/service.cpp 2014-11-14 11:26:45 +0000
> +++ examples/service/service.cpp 2015-04-27 19:46:27 +0000
> @@ -171,7 +171,7 @@
> {
> incoming,
> outgoing,
> - config.the_engine(instantiated_providers, config.the_provider_selection_policy(), settings),
> + config.the_engine(instantiated_providers, config.the_provider_selection_policy(), config.the_update_policy(), settings),
> config.the_permission_manager(incoming),
> culs::Harvester::Configuration
> {
>
> === modified file 'include/location_service/com/ubuntu/location/service/configuration.h'
> --- include/location_service/com/ubuntu/location/service/configuration.h 2014-11-14 11:33:05 +0000
> +++ include/location_service/com/ubuntu/location/service/configuration.h 2015-04-27 19:46:27 +0000
> @@ -22,6 +22,7 @@
>
> #include <com/ubuntu/location/provider.h>
> #include <com/ubuntu/location/provider_selection_policy.h>
> +#include <com/ubuntu/location/update_policy.h>
>
> #include <set>
>
> @@ -43,9 +44,11 @@
>
> virtual std::shared_ptr<Engine> the_engine(
> const std::set<Provider::Ptr>& provider_set,
> - const ProviderSelectionPolicy::Ptr& provider_selection_policy) = 0;
> + const ProviderSelectionPolicy::Ptr& provider_selection_policy,
> + const UpdatePolicy::Ptr& update_policy) = 0;
>
> virtual ProviderSelectionPolicy::Ptr the_provider_selection_policy() = 0;
> + virtual UpdatePolicy::Ptr the_update_policy() = 0;
> virtual std::set<Provider::Ptr> the_provider_set() = 0;
> virtual PermissionManager::Ptr the_permission_manager() = 0;
>
>
> === modified file 'include/location_service/com/ubuntu/location/update.h'
> --- include/location_service/com/ubuntu/location/update.h 2013-12-10 09:42:54 +0000
> +++ include/location_service/com/ubuntu/location/update.h 2015-04-27 19:46:27 +0000
> @@ -20,6 +20,7 @@
>
> #include <com/ubuntu/location/clock.h>
>
> +#include <chrono>
> #include <ostream>
>
> namespace com
> @@ -41,7 +42,7 @@
> * @param [in] when The timestamp when the value was measured.
> */
> inline Update(const T& value = T{},
> - const Clock::Timestamp& when = Clock::now())
> + const Clock::Timestamp& when = Clock::Timestamp())
> : value{value}, when{when}
> {
> }
> @@ -73,6 +74,33 @@
> Clock::Timestamp when = Clock::beginning_of_time();
> };
>
> +/** @brief TimeDifference enumerates all possible results of comparing two updates. */
> +enum class TimeDifference
> +{
> + none, ///< There is no time difference.
> + older, ///< The update is older than the reference update.
> + newer ///< The update is newer than the reference update.
> +};
> +
> +/**
> + * @brief compare_update_timestamps returns a classification of the time delta between two updates.
> + * @returns TimeDifference::older if update - last_update exceeds limit.
> + * @returns TimeDifference::newer if -(update - last-update) exceeds limit.
> + * @returns TimeDifference::none if neither of the previous conditions holds true.
> + */
> +template <typename T>
> +TimeDifference compare_update_timestamps(const location::Update<T> last_update, const location::Update<T> update, const std::chrono::minutes& limit)
> +{
> + auto delta = update.when - last_update.when;
Add const
> + if (delta > limit)
> + return TimeDifference::newer;
> +
> + if (delta < (-1 * limit))
> + return TimeDifference::older;
> +
> + return TimeDifference::none;
> +}
> +
> /**
> * @brief Pretty-prints the update to the provided output stream.
> * @param out The stream to write to.
>
> === modified file 'src/location_service/com/ubuntu/location/engine.cpp'
> --- src/location_service/com/ubuntu/location/engine.cpp 2015-04-23 14:48:44 +0000
> +++ src/location_service/com/ubuntu/location/engine.cpp 2015-04-27 19:46:27 +0000
> @@ -33,16 +33,22 @@
> const cul::Engine::Status cul::Engine::Configuration::Defaults::engine_state;
>
> cul::Engine::Engine(const cul::ProviderSelectionPolicy::Ptr& provider_selection_policy,
> + const cul::UpdatePolicy::Ptr& update_policy,
> const cul::Settings::Ptr& settings)
> : provider_selection_policy(provider_selection_policy),
> settings(settings),
> - update_policy(std::make_shared<cul::TimeBasedUpdatePolicy>())
> + update_policy(update_policy)
> {
> if (!provider_selection_policy) throw std::runtime_error
> {
> "Cannot construct an engine given a null ProviderSelectionPolicy"
> };
>
> + if (!update_policy) throw std::runtime_error
> + {
> + "Cannot construct an engine given a null UpdatePolicy"
> + };
> +
> if (!settings) throw std::runtime_error
> {
> "Cannot construct an engine given a null Settings instance"
>
> === modified file 'src/location_service/com/ubuntu/location/engine.h'
> --- src/location_service/com/ubuntu/location/engine.h 2015-04-23 14:48:44 +0000
> +++ src/location_service/com/ubuntu/location/engine.h 2015-04-27 19:46:27 +0000
> @@ -24,6 +24,7 @@
> #include <com/ubuntu/location/provider_selection_policy.h>
> #include <com/ubuntu/location/satellite_based_positioning_state.h>
> #include <com/ubuntu/location/space_vehicle.h>
> +#include <com/ubuntu/location/update_policy.h>
> #include <com/ubuntu/location/wifi_and_cell_reporting_state.h>
>
> #include <com/ubuntu/location/settings.h>
> @@ -33,8 +34,6 @@
> #include <mutex>
> #include <set>
>
> -#include "update_policy.h"
> -
> namespace com
> {
> namespace ubuntu
> @@ -137,6 +136,7 @@
> };
>
> Engine(const ProviderSelectionPolicy::Ptr& provider_selection_policy,
> + const UpdatePolicy::Ptr& update_policy,
> const Settings::Ptr& settings);
>
> Engine(const Engine&) = delete;
>
> === modified file 'src/location_service/com/ubuntu/location/service/daemon.cpp'
> --- src/location_service/com/ubuntu/location/service/daemon.cpp 2015-04-27 19:46:26 +0000
> +++ src/location_service/com/ubuntu/location/service/daemon.cpp 2015-04-27 19:46:27 +0000
> @@ -211,7 +211,7 @@
> {
> config.incoming,
> config.outgoing,
> - dc.the_engine(instantiated_providers, dc.the_provider_selection_policy(), config.settings),
> + dc.the_engine(instantiated_providers, dc.the_provider_selection_policy(), dc.the_update_policy(), config.settings),
> dc.the_permission_manager(config.outgoing),
> location::service::Harvester::Configuration
> {
>
> === modified file 'src/location_service/com/ubuntu/location/service/default_configuration.cpp'
> --- src/location_service/com/ubuntu/location/service/default_configuration.cpp 2014-11-14 11:33:05 +0000
> +++ src/location_service/com/ubuntu/location/service/default_configuration.cpp 2015-04-27 19:46:27 +0000
> @@ -21,6 +21,7 @@
>
> #include <com/ubuntu/location/engine.h>
> #include <com/ubuntu/location/non_selecting_provider_selection_policy.h>
> +#include <com/ubuntu/location/time_based_update_policy.h>
>
> namespace cul = com::ubuntu::location;
> namespace culs = com::ubuntu::location::service;
> @@ -28,9 +29,10 @@
> cul::Engine::Ptr culs::DefaultConfiguration::the_engine(
> const std::set<cul::Provider::Ptr>& provider_set,
> const cul::ProviderSelectionPolicy::Ptr& provider_selection_policy,
> + const cul::UpdatePolicy::Ptr& update_policy,
> const cul::Settings::Ptr& settings)
> {
> - auto engine = std::make_shared<cul::Engine>(provider_selection_policy, settings);
> + auto engine = std::make_shared<cul::Engine>(provider_selection_policy, update_policy, settings);
> for (const auto& provider : provider_set)
> engine->add_provider(provider);
>
> @@ -42,6 +44,11 @@
> return std::make_shared<cul::NonSelectingProviderSelectionPolicy>();
> }
>
> +cul::UpdatePolicy::Ptr culs::DefaultConfiguration::the_update_policy()
> +{
> + return std::make_shared<cul::TimeBasedUpdatePolicy>();
> +}
> +
> std::set<cul::Provider::Ptr> culs::DefaultConfiguration::the_provider_set(
> const cul::Provider::Ptr& seed)
> {
>
> === modified file 'src/location_service/com/ubuntu/location/service/default_configuration.h'
> --- src/location_service/com/ubuntu/location/service/default_configuration.h 2014-11-14 11:33:05 +0000
> +++ src/location_service/com/ubuntu/location/service/default_configuration.h 2015-04-27 19:46:27 +0000
> @@ -45,6 +45,9 @@
> // as specified by a client application.
> virtual ProviderSelectionPolicy::Ptr the_provider_selection_policy();
>
> + // Creates an update policy instance to filter incoming updates from providers.
> + virtual UpdatePolicy::Ptr the_update_policy();
> +
> // Returns a set of providers, seeded with the seed provider if it is not null.
> virtual std::set<Provider::Ptr> the_provider_set(const Provider::Ptr& seed = Provider::Ptr {});
>
> @@ -53,6 +56,7 @@
> virtual std::shared_ptr<Engine> the_engine(
> const std::set<Provider::Ptr>& provider_set,
> const ProviderSelectionPolicy::Ptr& provider_selection_policy,
> + const UpdatePolicy::Ptr& update_policy,
> const Settings::Ptr& settings);
>
> // Instantiates an instance of the permission manager.
>
> === modified file 'src/location_service/com/ubuntu/location/time_based_update_policy.cpp'
> --- src/location_service/com/ubuntu/location/time_based_update_policy.cpp 2015-04-23 21:30:04 +0000
> +++ src/location_service/com/ubuntu/location/time_based_update_policy.cpp 2015-04-27 19:46:27 +0000
> @@ -31,91 +31,78 @@
> return default_limit;
> }
>
> -TimeBasedUpdatePolicy::TimeBasedUpdatePolicy(std::chrono::minutes mins)
> +TimeBasedUpdatePolicy::TimeBasedUpdatePolicy(const std::chrono::minutes& mins)
> : limit(mins)
> {
>
> }
>
> -const location::Update<location::Position>& TimeBasedUpdatePolicy::verify_update(const location::Update<location::Position>& update)
> +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
> - {
> - // if the update has happened within a reasonable amount of time we will just use it if it is more accurate
> - // that the previous one.
> - use_new_update = last_position_update.value.accuracy.horizontal && update.value.accuracy.horizontal
> - && *last_position_update.value.accuracy.horizontal >= *update.value.accuracy.horizontal;
> + bool use_new_update{false};
> + switch (compare_update_timestamps(last_update.position, update, limit))
> + {
> + case location::TimeDifference::newer:
> + use_new_update = true; break;
> + case location::TimeDifference::older:
> + use_new_update = false; break;
> + case location::TimeDifference::none:
> + // if the update has happened within a reasonable amount of time we will just use it if it is more accurate
> + // that the previous one.
> + use_new_update =
> + (last_update.position.value.accuracy.horizontal && update.value.accuracy.horizontal) &&
> + *last_update.position.value.accuracy.horizontal >= *update.value.accuracy.horizontal;
> + break;
> }
>
> if (use_new_update)
> - {
> - last_position_update = update;
> - return update;
> - }
> - else
> - {
> - return last_position_update;
> - }
> + last_update.position = update;
> +
> + return last_update.position;
> }
>
>
> -const location::Update<location::Heading>& TimeBasedUpdatePolicy::verify_update(const location::Update<location::Heading>& update)
> +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;
> - }
> + bool use_new_update{false};
> + switch (compare_update_timestamps(last_update.heading, update, limit))
> + {
> + case location::TimeDifference::newer:
> + use_new_update = true; break;
> + case location::TimeDifference::older:
> + use_new_update = false; break;
> + case location::TimeDifference::none:
> + use_new_update = false; break;
> + }
> +
> if (use_new_update)
> - {
> - last_heading_update = update;
> - return update;
> - }
> - else
> - {
> - return last_heading_update;
> - }
> + last_update.heading = update;
> +
> + return last_update.heading;
> }
>
> -const location::Update<location::Velocity>& TimeBasedUpdatePolicy::verify_update(const location::Update<location::Velocity>& update)
> +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;
> + bool use_new_update{false};
> + switch (compare_update_timestamps(last_update.velocity, update, limit))
> + {
> + case location::TimeDifference::newer:
> + use_new_update = true; break;
> + case location::TimeDifference::older:
> + use_new_update = false; break;
> + case location::TimeDifference::none:
> + use_new_update = false; break;
> }
>
> if (use_new_update)
> - {
> - last_velocity_update = update;
> - return update;
> - }
> - else
> - {
> - return last_velocity_update;
> - }
> -}
> -
> -}
> -}
> -}
> \ No newline at end of file
> + last_update.velocity = update;
> +
> + return last_update.velocity;
> +}
> +
> +}
> +}
> +}
>
> === modified file 'src/location_service/com/ubuntu/location/time_based_update_policy.h'
> --- src/location_service/com/ubuntu/location/time_based_update_policy.h 2015-04-23 21:14:50 +0000
> +++ src/location_service/com/ubuntu/location/time_based_update_policy.h 2015-04-27 19:46:27 +0000
> @@ -33,32 +33,33 @@
> // An interface that can be implemented to add heuristics on how heading, position or velocity updates will be chosen.
> // This class ensures that the best update possible is chosen within a reasonable time bracket.
> class TimeBasedUpdatePolicy : public UpdatePolicy {
> +public:
> + // Returns the default timeout value for the policy.
> + static std::chrono::minutes default_timeout();
>
> - public:
> - TimeBasedUpdatePolicy(std::chrono::minutes mins=default_timeout());
> - TimeBasedUpdatePolicy(const TimeBasedUpdatePolicy&) = delete;
> - ~TimeBasedUpdatePolicy() = default;
> + TimeBasedUpdatePolicy(const std::chrono::minutes& mins=default_timeout());
>
> // Return if the given position update will be verified as the new position in the engine.
> - const location::Update<location::Position>& verify_update(const location::Update<location::Position>& update) override;
> + location::Update<location::Position> verify_update(const location::Update<location::Position>& update);
> // Return if the given heading update will be verified as the new heading in the engine.
> - const location::Update<location::Heading>& verify_update(const location::Update<location::Heading>& update) override;
> + location::Update<location::Heading> verify_update(const location::Update<location::Heading>& update);
> // Return if the given velocity update will be verified as the new velocity in the engine.
> - 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
> - location::Update<location::Position> last_position_update;
> - location::Update<location::Heading> last_heading_update;
> - location::Update<location::Velocity> last_velocity_update;
> -
> - private:
> + location::Update<location::Velocity> verify_update(const location::Update<location::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;
> +
> + // The updates we are tracking within the policy.
> + struct
> + {
> + location::Update<location::Position> position;
> + location::Update<location::Heading> heading;
> + location::Update<location::Velocity> velocity;
> + } last_update;
> +
> // used to calculate the time accepted bracket
> std::chrono::minutes limit;
> };
>
> === modified file 'src/location_service/com/ubuntu/location/update_policy.h'
> --- src/location_service/com/ubuntu/location/update_policy.h 2015-04-23 21:14:50 +0000
> +++ src/location_service/com/ubuntu/location/update_policy.h 2015-04-27 19:46:27 +0000
> @@ -42,30 +42,18 @@
> UpdatePolicy(const UpdatePolicy&) = delete;
> UpdatePolicy(UpdatePolicy&&) = delete;
> UpdatePolicy& operator=(const UpdatePolicy&) = delete;
> + UpdatePolicy& operator=(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;
> + virtual 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;
> + virtual 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;
> + virtual 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) const
> - {
> - auto delta = update.when - last_update.when;
> - 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) const
> - {
> - auto delta = update.when - last_update.when;
> - return delta < (-1 * limit);
> - }
> -
> -
> };
> }
> }
>
> === modified file 'tests/acceptance_tests.cpp'
> --- tests/acceptance_tests.cpp 2015-02-13 13:19:18 +0000
> +++ tests/acceptance_tests.cpp 2015-04-27 19:46:27 +0000
> @@ -240,7 +240,7 @@
> {
> incoming,
> outgoing,
> - config.the_engine(config.the_provider_set(helper), config.the_provider_selection_policy(), null_settings()),
> + config.the_engine(config.the_provider_set(helper), config.the_provider_selection_policy(), config.the_update_policy(), null_settings()),
> config.the_permission_manager(incoming),
> cul::service::Harvester::Configuration
> {
> @@ -362,7 +362,7 @@
> {
> incoming,
> outgoing,
> - config.the_engine(config.the_provider_set(helper), config.the_provider_selection_policy(), null_settings()),
> + config.the_engine(config.the_provider_set(helper), config.the_provider_selection_policy(), config.the_update_policy(), null_settings()),
> config.the_permission_manager(incoming),
> cul::service::Harvester::Configuration
> {
> @@ -443,7 +443,7 @@
> {
> incoming,
> outgoing,
> - config.the_engine(config.the_provider_set(helper), config.the_provider_selection_policy(), null_settings()),
> + config.the_engine(config.the_provider_set(helper), config.the_provider_selection_policy(), config.the_update_policy(), null_settings()),
> config.the_permission_manager(incoming),
> cul::service::Harvester::Configuration
> {
> @@ -523,7 +523,7 @@
> {
> incoming,
> outgoing,
> - config.the_engine(config.the_provider_set(helper), config.the_provider_selection_policy(), null_settings()),
> + config.the_engine(config.the_provider_set(helper), config.the_provider_selection_policy(), config.the_update_policy(), null_settings()),
> config.the_permission_manager(incoming),
> cul::service::Harvester::Configuration
> {
> @@ -611,7 +611,7 @@
> {
> incoming,
> outgoing,
> - config.the_engine(config.the_provider_set(helper), config.the_provider_selection_policy(), null_settings()),
> + config.the_engine(config.the_provider_set(helper), config.the_provider_selection_policy(), config.the_update_policy(), null_settings()),
> config.the_permission_manager(incoming),
> cul::service::Harvester::Configuration
> {
>
> === modified file 'tests/engine_test.cpp'
> --- tests/engine_test.cpp 2015-01-21 20:04:56 +0000
> +++ tests/engine_test.cpp 2015-04-27 19:46:27 +0000
> @@ -52,6 +52,10 @@
> MOCK_METHOD1(on_reference_velocity_updated,
> void(const location::Update<location::Velocity>&));
>
> + void inject_position_update(const location::Update<location::Position>& update)
> + {
> + mutable_updates().position(update);
> + }
> };
>
> struct MockSettings : public location::Settings
> @@ -70,11 +74,46 @@
> {
> return std::make_shared<::testing::NiceMock<MockSettings>>();
> }
> +
> +struct NullUpdatePolicy : public location::UpdatePolicy
> +{
> + location::Update<location::Position> verify_update(const location::Update<location::Position>& update)
> + {
> + return update;
> + }
> +
> + location::Update<location::Heading> verify_update(const location::Update<location::Heading>& update)
> + {
> + return update;
> + }
> +
> + location::Update<location::Velocity> verify_update(const location::Update<location::Velocity>& update)
> + {
> + return update;
> + }
> +};
> +
> +struct MockUpdatePolicy : public location::UpdatePolicy
> +{
> + MOCK_METHOD1(verify_update, location::Update<location::Position>(const location::Update<location::Position>&));
> + MOCK_METHOD1(verify_update, location::Update<location::Heading>(const location::Update<location::Heading>&));
> + MOCK_METHOD1(verify_update, location::Update<location::Velocity>(const location::Update<location::Velocity>&));
> +};
> +
> +const location::Update<location::Position> reference_position_update
> +{
> + {
> + location::wgs84::Latitude{9. * location::units::Degrees},
> + location::wgs84::Longitude{53. * location::units::Degrees},
> + location::wgs84::Altitude{-2. * location::units::Meters}
> + },
> + location::Clock::now()
> +};
> }
>
> TEST(Engine, adding_and_removing_providers_inserts_and_erases_from_underlying_collection)
> {
> - location::Engine engine {std::make_shared<NullProviderSelectionPolicy>(), mock_settings()};
> + location::Engine engine {std::make_shared<NullProviderSelectionPolicy>(), std::make_shared<NullUpdatePolicy>(), mock_settings()};
>
> auto provider1 = std::make_shared<testing::NiceMock<MockProvider>>();
> auto provider2 = std::make_shared<testing::NiceMock<MockProvider>>();
> @@ -94,7 +133,7 @@
>
> TEST(Engine, adding_a_null_provider_throws)
> {
> - location::Engine engine {std::make_shared<NullProviderSelectionPolicy>(), mock_settings()};
> + location::Engine engine {std::make_shared<NullProviderSelectionPolicy>(), std::make_shared<NullUpdatePolicy>(), mock_settings()};
>
> EXPECT_ANY_THROW(engine.add_provider(location::Provider::Ptr {}););
> }
> @@ -126,6 +165,7 @@
> &policy,
> [](location::ProviderSelectionPolicy*) {}
> },
> + std::make_shared<NullUpdatePolicy>(),
> mock_settings()
> };
>
> @@ -144,7 +184,7 @@
> using namespace ::testing;
> auto provider = std::make_shared<NiceMock<MockProvider>>();
> auto selection_policy = std::make_shared<NiceMock<MockProviderSelectionPolicy>>();
> - location::Engine engine{selection_policy, mock_settings()};
> + location::Engine engine{selection_policy, std::make_shared<NullUpdatePolicy>(), mock_settings()};
> engine.add_provider(provider);
>
> EXPECT_CALL(*provider, on_wifi_and_cell_reporting_state_changed(_)).Times(1);
> @@ -154,9 +194,27 @@
> EXPECT_CALL(*provider, on_reference_velocity_updated(_)).Times(1);
>
> engine.configuration.wifi_and_cell_id_reporting_state = location::WifiAndCellIdReportingState::on;
> - engine.updates.reference_location = location::Update<location::Position>{};
> - engine.updates.reference_heading = location::Update<location::Heading>{};
> - engine.updates.reference_velocity = location::Update<location::Velocity>{};
> + engine.updates.reference_location = location::Update<location::Position>{location::Position{}, location::Clock::now()};
> + engine.updates.reference_heading = location::Update<location::Heading>{location::Heading{}, location::Clock::now()};
> + engine.updates.reference_velocity = location::Update<location::Velocity>{location::Velocity{}, location::Clock::now()};
> +}
> +
> +TEST(Engine, a_provider_position_update_is_passed_through_the_update_policy)
> +{
> + using namespace ::testing;
> +
> + auto provider = std::make_shared<NiceMock<MockProvider>>();
> + auto selection_policy = std::make_shared<NiceMock<MockProviderSelectionPolicy>>();
> + auto update_policy = std::make_shared<NiceMock<MockUpdatePolicy>>();
> +
> + EXPECT_CALL(*update_policy, verify_update(reference_position_update))
> + .Times(1)
> + .WillRepeatedly(Return(reference_position_update));
> +
> + location::Engine engine{selection_policy, update_policy, mock_settings()};
> + engine.add_provider(provider);
> +
> + provider->inject_position_update(reference_position_update);
> }
>
> /* TODO(tvoss): We have to disable these tests as the MP is being refactored to not break ABI.
> @@ -288,7 +346,7 @@
> .Times(1)
> .WillRepeatedly(Return(ss_engine_state.str()));
>
> - location::Engine engine{selection_policy, settings};
> + location::Engine engine{selection_policy, std::make_shared<NullUpdatePolicy>(), settings};
> }
>
> TEST(Engine, stores_state_from_settings_on_destruction)
> @@ -314,6 +372,6 @@
> .Times(1)
> .WillRepeatedly(Return(true));
>
> - {location::Engine engine{selection_policy, settings};}
> + {location::Engine engine{selection_policy, std::make_shared<NullUpdatePolicy>(), settings};}
> }
>
>
> === modified file 'tests/time_based_update_policy_test.cpp'
> --- tests/time_based_update_policy_test.cpp 2015-04-23 17:04:09 +0000
> +++ tests/time_based_update_policy_test.cpp 2015-04-27 19:46:27 +0000
> @@ -24,108 +24,70 @@
>
> 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;
> +const cul::Update<cul::Position> reference_position_update
> +{
> + {
> + cul::wgs84::Latitude{9. * cul::units::Degrees},
> + cul::wgs84::Longitude{53. * cul::units::Degrees},
> + cul::wgs84::Altitude{-2. * cul::units::Meters}
> + },
> + cul::Clock::now()
> };
> +}
>
> 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);
> + auto policy = std::make_shared<cul::TimeBasedUpdatePolicy>(std::chrono::minutes(2));
> + ASSERT_EQ(reference_position_update, policy->verify_update(reference_position_update));
> +
> + cul::Update<cul::Position> old_update
> + {
> + {
> + cul::wgs84::Latitude{10. * cul::units::Degrees},
> + cul::wgs84::Longitude{60. * cul::units::Degrees},
> + cul::wgs84::Altitude{10. * cul::units::Meters}
> + },
> + reference_position_update.when - std::chrono::minutes(5)
> + };
> + ASSERT_EQ(reference_position_update, policy->verify_update(old_update));
> }
>
> 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);
> + auto policy = std::make_shared<cul::TimeBasedUpdatePolicy>(std::chrono::minutes(2));
> + ASSERT_EQ(reference_position_update, policy->verify_update(reference_position_update));
> +
> + cul::Update<cul::Position> new_update
> + {
> + {
> + cul::wgs84::Latitude{10. * cul::units::Degrees},
> + cul::wgs84::Longitude{60. * cul::units::Degrees},
> + cul::wgs84::Altitude{10. * cul::units::Meters}
> + },
> + reference_position_update.when + std::chrono::minutes(3)
> + };
> +
> + ASSERT_EQ(new_update, policy->verify_update(new_update));
> }
>
> 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);
> + auto policy = std::make_shared<cul::TimeBasedUpdatePolicy>(std::chrono::minutes(2));
> +
> + auto obfuscated_update = reference_position_update;
> + obfuscated_update.value.accuracy.horizontal = 1. * cul::units::Meters;
> + ASSERT_EQ(obfuscated_update, policy->verify_update(obfuscated_update));
> +
> + cul::Update<cul::Position> new_update
> + {
> + {
> + cul::wgs84::Latitude{10. * cul::units::Degrees},
> + cul::wgs84::Longitude{60. * cul::units::Degrees},
> + cul::wgs84::Altitude{10. * cul::units::Meters},
> + },
> + reference_position_update.when + std::chrono::minutes(1)
> + };
> +
> + new_update.value.accuracy.horizontal = 8. * cul::units::Meters;
> + ASSERT_EQ(obfuscated_update, policy->verify_update(new_update));
> }
>
--
https://code.launchpad.net/~mandel/location-service/tvoss-time-delta-fixes/+merge/257573
Your team Ubuntu Phablet Team is subscribed to branch lp:location-service.
More information about the Ubuntu-reviews
mailing list