Merge lp:~ssweeny/location-service/fix-lp1570878 into lp:location-service/trunk

[Scott Sweeny]

Fusion provider: Always use an update that came from the same source as the previous used update (fixes LP: #1570878)

Fix a bug in the fusion provider in which it is too strict about filtering updates which are slightly less accurate than previous updates. The new behavior is to automatically use an update which comes from the same source as the previous used update.

Tested by driving around on local highways and observing that I get position updates around once per second or better.

[Jim Hodapp (jhodapp)]

Looks good to me after discussing a few questions that I had about the code with Scott. I would prefer to not have the value.value syntax but Scott told me it's not possible to avoid that without breaking the provider API.

review: Approve (code)

[Thomas Voß (thomas-voss)]

LGTM.

review: Approve

=== modified file 'include/location_service/com/ubuntu/location/fusion_provider.h'
--- include/location_service/com/ubuntu/location/fusion_provider.h	2016-03-10 03:02:31 +0000
+++ include/location_service/com/ubuntu/location/fusion_provider.h	2016-06-08 22:07:21 +0000
@@ -48,7 +48,7 @@
     void stop_velocity_updates() override;
 
 private:
-    Optional<Update<Position>> last_position;
+    Optional<WithSource<Update<Position>>> last_position;
     std::set<Provider::Ptr> providers;
     std::vector<core::ScopedConnection> connections;
 };

=== modified file 'include/location_service/com/ubuntu/location/newer_or_more_accurate_update_selector.h'
--- include/location_service/com/ubuntu/location/newer_or_more_accurate_update_selector.h	2016-03-10 03:02:31 +0000
+++ include/location_service/com/ubuntu/location/newer_or_more_accurate_update_selector.h	2016-06-08 22:07:21 +0000
@@ -31,23 +31,28 @@
 public:
     typedef std::shared_ptr<NewerOrMoreAccurateUpdateSelector> Ptr;
 
-    Update<Position> select(const Update<Position>& older,
-                            const Update<Position>& newer) override
+    WithSource<Update<Position>> select(const WithSource<Update<Position>>& older,
+                                        const WithSource<Update<Position>>& newer) override
     {
         // Basically copied this value from the Android fusion provider
         static const std::chrono::seconds cutoff(11);
 
+        // If the new position is from the same source as the old one then just use it
+        if (newer.source == older.source) {
+            return newer;
+        }
+
         // If the new position is newer by a significant margin then just use it
-        if (newer.when > older.when + cutoff) {
+        if (newer.value.when > older.value.when + cutoff) {
             return newer;
         }
 
         // Choose the position with the smaller accuracy circle if both have them
-        if (!older.value.accuracy.horizontal)
+        if (!older.value.value.accuracy.horizontal)
             return newer;
-        if (!newer.value.accuracy.horizontal)
+        if (!newer.value.value.accuracy.horizontal)
             return older;
-        if (newer.value.accuracy.horizontal < older.value.accuracy.horizontal)
+        if (newer.value.value.accuracy.horizontal < older.value.value.accuracy.horizontal)
             return newer;
         else
             return older;

=== modified file 'include/location_service/com/ubuntu/location/newer_update_selector.h'
--- include/location_service/com/ubuntu/location/newer_update_selector.h	2016-03-10 03:02:31 +0000
+++ include/location_service/com/ubuntu/location/newer_update_selector.h	2016-06-08 22:07:21 +0000
@@ -31,8 +31,8 @@
 public:
     typedef std::shared_ptr<NewerUpdateSelector> Ptr;
 
-    Update<Position> select (const Update<Position>& older,
-                             const Update<Position>& newer) override
+    virtual WithSource<Update<Position>> select(const WithSource<Update<Position>>& older,
+                                                const WithSource<Update<Position>>& newer) override
     {
         return newer;
     }

=== modified file 'include/location_service/com/ubuntu/location/update_selector.h'
--- include/location_service/com/ubuntu/location/update_selector.h	2016-03-10 03:02:31 +0000
+++ include/location_service/com/ubuntu/location/update_selector.h	2016-06-08 22:07:21 +0000
@@ -27,6 +27,10 @@
 {
 namespace location
 {
+
+template<typename T>
+struct WithSource { std::shared_ptr<Provider> source; T value; };
+
 class UpdateSelector
 {
 public:
@@ -36,8 +40,8 @@
     UpdateSelector& operator=(const UpdateSelector&) = delete;
     virtual ~UpdateSelector() = default;
 
-    virtual Update<Position> select(const Update<Position>& older,
-                                    const Update<Position>& newer) = 0;
+    virtual WithSource<Update<Position>> select(const WithSource<Update<Position>>& older,
+                                                const WithSource<Update<Position>>& newer) = 0;
 
 protected:
     UpdateSelector() = default;

=== modified file 'src/location_service/com/ubuntu/location/fusion_provider.cpp'
--- src/location_service/com/ubuntu/location/fusion_provider.cpp	2016-03-10 19:29:45 +0000
+++ src/location_service/com/ubuntu/location/fusion_provider.cpp	2016-06-08 22:07:21 +0000
@@ -37,7 +37,6 @@
            cul::Provider::Requirements::satellites;
 }
 
-
 cul::FusionProvider::FusionProvider(const std::set<location::Provider::Ptr>& providers, const UpdateSelector::Ptr& update_selector)
     : Provider{all_features(), all_requirements()},
       providers{providers}
@@ -46,14 +45,16 @@
     for (auto provider : providers)
     {
         connections.push_back(provider->updates().position.connect(
-              [this, update_selector](const cul::Update<cul::Position>& u)
+              [this, provider, update_selector](const cul::Update<cul::Position>& u)
               {
                   // if this is the first update, use it
                   if (!last_position) {
-                      mutable_updates().position(*(last_position = u));
+                      mutable_updates().position((*(last_position = WithSource<Update<Position>>{provider, u})).value);
+                      
+                  // otherwise use the selector
                   } else {
                       try {
-                          mutable_updates().position(*(last_position = update_selector->select(*last_position, u)));
+                          mutable_updates().position((*(last_position = update_selector->select(*last_position, WithSource<Update<Position>>{provider, u}))).value);
                       } catch (const std::exception& e) {
                           LOG(WARNING) << "Error while updating position";
                       }

=== modified file 'tests/provider_test.cpp'
--- tests/provider_test.cpp	2016-03-08 22:20:10 +0000
+++ tests/provider_test.cpp	2016-06-08 22:07:21 +0000
@@ -409,3 +409,33 @@
     mp2.inject_update(after);
 
 }
+
+TEST(FusionProvider, update_from_same_provider_is_chosen)
+{
+    using namespace ::testing;
+
+    NiceMock<MockProvider> mp1;
+
+    cul::Provider::Ptr p1{std::addressof(mp1), [](cul::Provider*){}};
+
+    std::set<cul::Provider::Ptr> providers{p1};
+
+    cul::FusionProvider fp{providers, std::make_shared<cul::NewerOrMoreAccurateUpdateSelector>()};
+
+    cul::Update<cul::Position> before, after;
+    before.when = cul::Clock::now() - std::chrono::seconds(5);
+    before.value = cul::Position(cul::wgs84::Latitude(), cul::wgs84::Longitude(), cul::wgs84::Altitude(), cul::Position::Accuracy::Horizontal{50*cul::units::Meters});
+    after.when = cul::Clock::now();
+    after.value = cul::Position(cul::wgs84::Latitude(), cul::wgs84::Longitude(), cul::wgs84::Altitude(), cul::Position::Accuracy::Horizontal{500*cul::units::Meters});
+
+    NiceMock<MockEventConsumer> mec;
+    // We should see the "newer" position even though it's less accurate since
+    // it came from the same source
+    EXPECT_CALL(mec, on_new_position(before)).Times(1);
+    EXPECT_CALL(mec, on_new_position(after)).Times(1);
+
+    fp.updates().position.connect([&mec](const cul::Update<cul::Position>& p){mec.on_new_position(p);});
+
+    mp1.inject_update(before);
+    mp1.inject_update(after);
+}