Merge lp:~mandel/location-service/tvoss-time-delta-fixes into lp:location-service/trunk

[Manuel de la Peña]

Improve heuristics and add them as parts of the config.

Improve the heuristics code and add it as part of the config.

[Jim Hodapp (jhodapp)]

One minor change inline below.

review: Needs Fixing (code)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:194
http://jenkins.qa.ubuntu.com/job/location-service-ci/372/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-amd64-ci/44
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/44
        deb: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/44/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-i386-ci/44

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/location-service-ci/372/rebuild

review: Approve (continuous-integration)

[lp:~mandel/location-service/tvoss-time-delta-fixes]

195. By Manuel de la Peña on 2015-04-27

Added code according to the reviews.

[Jim Hodapp (jhodapp)]

Looks good.

review: Approve (code)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:195
http://jenkins.qa.ubuntu.com/job/location-service-ci/375/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/location-service-vivid-amd64-ci/47/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/47
        deb: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/47/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-i386-ci/47

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/location-service-ci/375/rebuild

review: Needs Fixing (continuous-integration)

[lp:~mandel/location-service/tvoss-time-delta-fixes]

196. By Manuel de la Peña on 2015-04-29

Merged verbose-logging into tvoss-time-delta-fixes.

197. By Manuel de la Peña on 2015-04-29

Merged verbose-logging into tvoss-time-delta-fixes.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:197
http://jenkins.qa.ubuntu.com/job/location-service-ci/378/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-amd64-ci/50
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/50
        deb: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/50/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/location-service-vivid-i386-ci/50/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/location-service-ci/378/rebuild

review: Needs Fixing (continuous-integration)

[Thomas Voß (thomas-voss)]

LGTM.

review: Approve

[lp:~mandel/location-service/tvoss-time-delta-fixes]

198. By Manuel de la Peña on 2015-05-26

Merged verbose-logging into tvoss-time-delta-fixes.

Unmerged revisions

198. By Manuel de la Peña on 2015-05-26

Merged verbose-logging into tvoss-time-delta-fixes.

197. By Manuel de la Peña on 2015-04-29

Merged verbose-logging into tvoss-time-delta-fixes.

196. By Manuel de la Peña on 2015-04-29

Merged verbose-logging into tvoss-time-delta-fixes.

195. By Manuel de la Peña on 2015-04-27

Added code according to the reviews.

194. By Manuel de la Peña on 2015-04-27

Merged verbose-logging into tvoss-time-delta-fixes.

193. By Manuel de la Peña on 2015-04-27

Merged verbose-logging into tvoss-time-delta-fixes.

192. By Manuel de la Peña on 2015-04-27

Merge with tvoss fixes.

=== modified file 'examples/service/service.cpp'
--- examples/service/service.cpp	2014-11-14 11:26:45 +0000
+++ examples/service/service.cpp	2015-05-26 16:07:17 +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-05-26 16:07:17 +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-05-26 16:07:17 +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 const delta = update.when - last_update.when;
+    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-05-26 16:07:17 +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-05-26 16:07:17 +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-05-26 16:07:17 +0000
+++ src/location_service/com/ubuntu/location/service/daemon.cpp	2015-05-26 16:07:17 +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-05-26 16:07:17 +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-05-26 16:07:17 +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-05-26 16:07:17 +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-05-26 16:07:17 +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-05-26 16:07:17 +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-05-26 16:07:17 +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-05-26 16:07:17 +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-05-26 16:07:17 +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));
 }