Merge lp:~mardy/location-service/last-known-position-15.04 into lp:location-service/15.04

[Alberto Mardegan]

Send last known position on session start

Send last known position on session start

[Thomas Voß (thomas-voss)]

LGTM.

review: Needs Fixing

[lp:~mardy/location-service/last-known-position-15.04]

204. By Alberto Mardegan on 2015-11-19

Address review comments

[Thomas Voß (thomas-voss)]

Thanks for the changes, final remark inline.

review: Needs Fixing

[Thomas Voß (thomas-voss)]

LGTM.

review: Approve

[lp:~mardy/location-service/last-known-position-15.04]

205. By Alberto Mardegan on 2015-11-25

Fix D-Bus session shutdown

=== 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-11-25 10:18:12 +0000
@@ -169,19 +169,28 @@
 
     // We wire up changes in the engine's configuration to the respective slots
     // of the provider.
-    auto cp = updates.reference_location.changed().connect([provider](const cul::Update<cul::Position>& pos)
-    {
-        provider->on_reference_location_updated(pos);
-    });
-
-    auto cv = updates.reference_velocity.changed().connect([provider](const cul::Update<cul::Velocity>& velocity)
-    {
-        provider->on_reference_velocity_updated(velocity);
-    });
-
-    auto ch = updates.reference_heading.changed().connect([provider](const cul::Update<cul::Heading>& heading)
-    {
-        provider->on_reference_heading_updated(heading);
+    auto cp = updates.last_known_location.changed().connect([provider](const cul::Optional<cul::Update<cul::Position>>& pos)
+    {
+        if (pos)
+        {
+            provider->on_reference_location_updated(pos.get());
+        }
+    });
+
+    auto cv = updates.last_known_velocity.changed().connect([provider](const cul::Optional<cul::Update<cul::Velocity>>& velocity)
+    {
+        if (velocity)
+        {
+            provider->on_reference_velocity_updated(velocity.get());
+        }
+    });
+
+    auto ch = updates.last_known_heading.changed().connect([provider](const cul::Optional<cul::Update<cul::Heading>>& heading)
+    {
+        if (heading)
+        {
+            provider->on_reference_heading_updated(heading.get());
+        }
     });
 
     auto cr = configuration.wifi_and_cell_id_reporting_state.changed().connect([provider](cul::WifiAndCellIdReportingState state)
@@ -207,7 +216,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 = update_policy->verify_update(src);
+        updates.last_known_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-04-23 14:48:44 +0000
+++ src/location_service/com/ubuntu/location/engine.h	2015-11-25 10:18:12 +0000
@@ -127,11 +127,11 @@
     struct Updates
     {
         /** The current best known reference location */
-        core::Property<Update<Position>> reference_location{};
+        core::Property<Optional<Update<Position>>> last_known_location{};
         /** The current best known velocity estimate. */
-        core::Property<Update<Velocity>> reference_velocity{};
+        core::Property<Optional<Update<Velocity>>> last_known_velocity{};
         /** The current best known heading estimate. */
-        core::Property<Update<Heading>> reference_heading{};
+        core::Property<Optional<Update<Heading>>> last_known_heading{};
         /** The current set of visible SpaceVehicles. */
         core::Property<std::map<SpaceVehicle::Key, SpaceVehicle>> visible_space_vehicles{};
     };

=== modified file 'src/location_service/com/ubuntu/location/service/implementation.cpp'
--- src/location_service/com/ubuntu/location/service/implementation.cpp	2014-08-13 13:08:22 +0000
+++ src/location_service/com/ubuntu/location/service/implementation.cpp	2015-11-25 10:18:12 +0000
@@ -113,10 +113,13 @@
               {
                   visible_space_vehicles() = svs;
               }),
-          configuration.engine->updates.reference_location.changed().connect(
-              [this](const cul::Update<cul::Position>& update)
+          configuration.engine->updates.last_known_location.changed().connect(
+              [this](const cul::Optional<cul::Update<cul::Position>>& update)
               {
-                  harvester.report_position_update(update);
+                  if (update)
+                  {
+                      harvester.report_position_update(update.get());
+                  }
               })
       }
 {
@@ -149,5 +152,21 @@
         new ProxyProvider{provider_selection}
     };
 
-    return session::Interface::Ptr{new culs::session::Implementation(proxy_provider)};
+    session::Interface::Ptr session_iface{new session::Implementation(proxy_provider)};
+    std::weak_ptr<session::Interface> session_weak{session_iface};
+    session_iface->updates().position_status.changed().connect([this, session_weak](const session::Interface::Updates::Status& status)
+    {
+        cul::Optional<cul::Update<cul::Position>> last_known_position =
+            configuration.engine->updates.last_known_location.get();
+        if (last_known_position &&
+            status == culs::session::Interface::Updates::Status::enabled)
+        {
+            // Immediately send the last known position to the client
+            if (auto session_iface = session_weak.lock())
+            {
+                session_iface->updates().position = last_known_position.get();
+            }
+        }
+    });
+    return session_iface;
 }

=== 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-11-25 10:18:12 +0000
@@ -53,8 +53,9 @@
     {
         // 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;
+        use_new_update = !last_position_update.value.accuracy.horizontal ||
+            (update.value.accuracy.horizontal
+             && *last_position_update.value.accuracy.horizontal >= *update.value.accuracy.horizontal);
     }
 
     if (use_new_update)
@@ -118,4 +119,4 @@
 
 }
 }
-}
\ No newline at end of file
+}

=== modified file 'tests/acceptance_tests.cpp'
--- tests/acceptance_tests.cpp	2015-02-13 13:19:18 +0000
+++ tests/acceptance_tests.cpp	2015-11-25 10:18:12 +0000
@@ -663,6 +663,110 @@
     EXPECT_EQ(core::testing::ForkAndRunResult::empty, core::testing::fork_and_run(server, client));
 }
 
+TEST_F(LocationServiceStandalone, NewSessionsGetLastKnownPosition)
+{
+    core::testing::CrossProcessSync sync_start;
+
+    auto server = [this, &sync_start]()
+    {
+        SCOPED_TRACE("Server");
+
+        auto trap = core::posix::trap_signals_for_all_subsequent_threads({core::posix::Signal::sig_term});
+        trap->signal_raised().connect([trap](core::posix::Signal)
+        {
+            trap->stop();
+        });
+
+        auto incoming = session_bus();
+        auto outgoing = session_bus();
+
+        incoming->install_executor(core::dbus::asio::make_executor(incoming));
+        outgoing->install_executor(core::dbus::asio::make_executor(outgoing));
+
+        auto dummy = new DummyProvider();
+        cul::Provider::Ptr helper(dummy);
+
+        cul::service::DefaultConfiguration config;
+        cul::service::Implementation::Configuration configuration
+        {
+            incoming,
+            outgoing,
+            config.the_engine(config.the_provider_set(helper), config.the_provider_selection_policy(), null_settings()),
+            config.the_permission_manager(incoming),
+            cul::service::Harvester::Configuration
+            {
+                cul::connectivity::platform_default_manager(),
+                std::make_shared<NullReporter>()
+            }
+        };
+        auto location_service = std::make_shared<cul::service::Implementation>(configuration);
+
+        configuration.engine->updates.last_known_location.set(reference_position_update);
+        std::thread t1{[incoming](){incoming->run();}};
+        std::thread t2{[outgoing](){outgoing->run();}};
+
+        sync_start.try_signal_ready_for(std::chrono::milliseconds{500});
+
+        trap->run();
+
+        incoming->stop();
+        outgoing->stop();
+
+        if (t1.joinable())
+            t1.join();
+
+        if (t2.joinable())
+            t2.join();
+
+        return ::testing::Test::HasFailure() ? core::posix::exit::Status::failure : core::posix::exit::Status::success;
+    };
+
+    auto client = [this, &sync_start]()
+    {
+        SCOPED_TRACE("Client");
+
+        EXPECT_EQ(1, sync_start.wait_for_signal_ready_for(std::chrono::milliseconds{500}));
+
+        auto bus = session_bus();
+        bus->install_executor(dbus::asio::make_executor(bus));
+        std::thread t{[bus](){bus->run();}};
+
+        auto location_service = dbus::resolve_service_on_bus<
+            cul::service::Interface,
+            cul::service::Stub>(bus);
+
+        auto s1 = location_service->create_session_for_criteria(cul::Criteria{});
+
+        std::cout << "Successfully created session" << std::endl;
+
+        cul::Update<cul::Position> position;
+        auto c1 = s1->updates().position.changed().connect(
+            [&](const cul::Update<cul::Position>& new_position) {
+                std::cout << "On position updated: " << new_position << std::endl;
+                position = new_position;
+            });
+
+        std::cout << "Created event connections, starting updates..." << std::endl;
+
+        s1->updates().position_status = culss::Interface::Updates::Status::enabled;
+
+        std::cout << "done" << std::endl;
+
+        std::this_thread::sleep_for(std::chrono::milliseconds{1000});
+
+        bus->stop();
+
+        if (t.joinable())
+            t.join();
+
+        EXPECT_EQ(reference_position_update, position);
+
+        return ::testing::Test::HasFailure() ? core::posix::exit::Status::failure : core::posix::exit::Status::success;
+    };
+
+    EXPECT_EQ(core::testing::ForkAndRunResult::empty, core::testing::fork_and_run(server, client));
+}
+
 namespace
 {
 struct LocationServiceStandaloneLoad : public LocationServiceStandalone

=== modified file 'tests/engine_test.cpp'
--- tests/engine_test.cpp	2015-01-21 20:04:56 +0000
+++ tests/engine_test.cpp	2015-11-25 10:18:12 +0000
@@ -154,9 +154,9 @@
     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.last_known_location = location::Update<location::Position>{};
+    engine.updates.last_known_heading = location::Update<location::Heading>{};
+    engine.updates.last_known_velocity = location::Update<location::Velocity>{};
 }
 
 /* TODO(tvoss): We have to disable these tests as the MP is being refactored to not break ABI.