Merge lp:~thomas-voss/location-service/allow-for-disabling-providers into lp:location-service/trunk

[Thomas Voß]

Allow for enabling/disabling providers.
Wire up engine state changes to enabling/disabling of providers.

Allow for enabling/disabling providers.
Wire up engine state changes to enabling/disabling of providers.

[Loïc Minier (lool)]

Looks really clean; I see the default is enabled, I guess I need to test that config updates actually trigger enabling/disabling as designed

[Loïc Minier (lool)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:152
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~thomas-voss/location-service/allow-for-disabling-providers/+merge/241707/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/location-service-ci/335/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/location-service-vivid-amd64-ci/7/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/7
        deb: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/7/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-i386-ci/7

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

review: Needs Fixing (continuous-integration)

[Loïc Minier (lool)]

I guess this is only useful combined with https://code.launchpad.net/~thomas-voss/location-service/make-engine-state-persistent-across-boots-and-otas/+merge/241784

[lp:~thomas-voss/location-service/allow-for-disabling-providers]

153. By Thomas Voß on 2015-01-12

Merge pre-requisite branch.
Fix FTBFS in test cases.

154. By Thomas Voß on 2015-01-21

Refactor the change to make it ABI stable.

155. By Thomas Voß on 2015-01-21

Make enable and disable non-virtual for now.

156. By Thomas Voß on 2015-01-22

Make sure that the provider is really stopped when disabling it.

157. By Thomas Voß on 2015-01-23

Only stop provider updates if they have been running.

158. By Thomas Voß on 2015-01-23

Anything lt 0 means: Provider is disabled.

159. By Thomas Voß on 2015-01-23

Make sure that we only enable providers that really should be enabled.

160. By Thomas Voß on 2015-01-23

Need this as context of the lamdba.

161. By Thomas Voß on 2015-01-23

Revert auto-enablement.

162. By Thomas Voß on 2015-01-23

A minor niggle.

163. By Thomas Voß on 2015-01-23

Merge pre-req branch.

164. By Thomas Voß on 2015-01-25

Make sure that newly added providers are disabled, too.

Unmerged revisions

=== modified file 'include/location_service/com/ubuntu/location/provider.h'
--- include/location_service/com/ubuntu/location/provider.h	2013-12-30 08:06:11 +0000
+++ include/location_service/com/ubuntu/location/provider.h	2014-11-13 16:49:18 +0000
@@ -82,11 +82,27 @@
     public:
         typedef std::shared_ptr<Controller> Ptr;
 
+        /** @brief Enumerates known states of a controller/provider. */
+        enum class State
+        {
+            disabled, ///< The provider is disabled.
+            enabled ///< The provider is enabled.
+        };
+
         virtual ~Controller() = default;
         Controller(const Controller&) = delete;
         Controller& operator=(const Controller&) = delete;
 
         /**
+         * @brief Returns the getable/setable/observable state of the controller.
+         *
+         * If the controller is in state disabled, all calls to start* or stop*
+         * have no effect and subsequent calls to is*running will return false.
+         *
+         */
+        virtual core::Property<State>& state();
+
+        /**
          * @brief Request to start position updates if not already running.
          */
         virtual void start_position_updates();
@@ -140,6 +156,7 @@
 
     private:
         Provider& instance;
+        core::Property<State> current_state;
         std::atomic<int> position_updates_counter;
         std::atomic<int> heading_updates_counter;
         std::atomic<int> velocity_updates_counter;
@@ -229,6 +246,16 @@
     virtual Updates& mutable_updates();
     
     /**
+     * @brief Disables the provider, empty by default.
+     */
+    virtual void disable();
+
+    /**
+     * @brief Enables the provider, empty by default.
+     */
+    virtual void enable();
+
+    /**
      * @brief Implementation-specific, empty by default.
      */
     virtual void start_position_updates();

=== modified file 'include/location_service/com/ubuntu/location/providers/remote/interface.h'
--- include/location_service/com/ubuntu/location/providers/remote/interface.h	2014-09-12 15:04:21 +0000
+++ include/location_service/com/ubuntu/location/providers/remote/interface.h	2014-11-13 16:49:18 +0000
@@ -66,6 +66,8 @@
     // Called by the engine whenever the reference velocity changed.
     DBUS_CPP_METHOD_DEF(OnReferenceVelocityChanged, remote::Interface)
 
+    DBUS_CPP_METHOD_DEF(Disable, remote::Interface)
+    DBUS_CPP_METHOD_DEF(Enable, remote::Interface)
     DBUS_CPP_METHOD_DEF(StartPositionUpdates, remote::Interface)
     DBUS_CPP_METHOD_DEF(StopPositionUpdates, remote::Interface)
     DBUS_CPP_METHOD_DEF(StartHeadingUpdates, remote::Interface)

=== modified file 'src/location_service/com/ubuntu/location/engine.cpp'
--- src/location_service/com/ubuntu/location/engine.cpp	2014-10-03 20:00:09 +0000
+++ src/location_service/com/ubuntu/location/engine.cpp	2014-11-13 16:49:18 +0000
@@ -31,21 +31,41 @@
         std::runtime_error("Cannot construct an engine given a null ProviderSelectionPolicy");
 
     // Setup behavior in case of configuration changes.
+    configuration.satellite_based_positioning_state.changed().connect([this](const SatelliteBasedPositioningState& state)
+    {
+        for_each_provider([state](const Provider::Ptr& provider)
+        {
+            if (provider->requires(cul::Provider::Requirements::satellites))
+            {
+                switch (state)
+                {
+                case SatelliteBasedPositioningState::on:
+                    provider->state_controller()->state() = cul::Provider::Controller::State::enabled;
+                    break;
+                case SatelliteBasedPositioningState::off:
+                    provider->state_controller()->state() = cul::Provider::Controller::State::disabled;
+                    break;
+                }
+            }
+        });
+    });
+
     configuration.engine_state.changed().connect([this](const Engine::Status& status)
     {
-        switch (status)
+        for_each_provider([status](const Provider::Ptr& provider)
         {
-        case Engine::Status::off:
-            for_each_provider([](const Provider::Ptr& provider)
+            switch (status)
             {
-                provider->state_controller()->stop_position_updates();
-                provider->state_controller()->stop_heading_updates();
-                provider->state_controller()->stop_velocity_updates();
-            });
-            break;
-        default:
-            break;
-        }
+            case Engine::Status::on:
+                provider->state_controller()->state() = cul::Provider::Controller::State::enabled;
+                break;
+            case Engine::Status::off:
+                provider->state_controller()->state() = cul::Provider::Controller::State::disabled;
+                break;
+            default:
+                break;
+            }
+        });
     });
 }
 

=== modified file 'src/location_service/com/ubuntu/location/provider.cpp'
--- src/location_service/com/ubuntu/location/provider.cpp	2014-06-20 07:40:34 +0000
+++ src/location_service/com/ubuntu/location/provider.cpp	2014-11-13 16:49:18 +0000
@@ -23,8 +23,16 @@
 
 namespace cul = com::ubuntu::location;
 
+core::Property<cul::Provider::Controller::State>& cul::Provider::Controller::state()
+{
+    return current_state;
+}
+
 void cul::Provider::Controller::start_position_updates()
 {
+    if (current_state == cul::Provider::Controller::State::disabled)
+        return;
+
     if (++position_updates_counter == 1)
     {
         instance.start_position_updates();
@@ -46,6 +54,9 @@
 
 void cul::Provider::Controller::start_heading_updates()
 {
+    if (current_state == cul::Provider::Controller::State::disabled)
+        return;
+
     if (++heading_updates_counter == 1)
     {
         instance.start_heading_updates();
@@ -67,6 +78,9 @@
 
 void cul::Provider::Controller::start_velocity_updates()
 {
+    if (current_state == cul::Provider::Controller::State::disabled)
+        return;
+
     if (++velocity_updates_counter == 1)
     {
         instance.start_velocity_updates();
@@ -88,10 +102,26 @@
 
 cul::Provider::Controller::Controller(cul::Provider& instance)
     : instance(instance),
+      current_state(cul::Provider::Controller::State::enabled),
       position_updates_counter(0),
       heading_updates_counter(0),
       velocity_updates_counter(0)
 {
+    current_state.changed().connect([this](cul::Provider::Controller::State state)
+    {
+        switch (state)
+        {
+        case cul::Provider::Controller::State::disabled:
+            Controller::instance.disable();
+            stop_position_updates();
+            stop_heading_updates();
+            stop_velocity_updates();
+            break;
+        case cul::Provider::Controller::State::enabled:
+            Controller::instance.enable();
+            break;
+        }
+    });
 }
 
 const cul::Provider::Controller::Ptr& cul::Provider::state_controller() const
@@ -149,6 +179,8 @@
 {
 }
 
+void cul::Provider::disable() {}
+void cul::Provider::enable() {}
 void cul::Provider::start_position_updates() {}
 void cul::Provider::stop_position_updates() {}
 void cul::Provider::start_heading_updates() {}

=== modified file 'src/location_service/com/ubuntu/location/providers/remote/provider.cpp'
--- src/location_service/com/ubuntu/location/providers/remote/provider.cpp	2014-10-27 21:58:16 +0000
+++ src/location_service/com/ubuntu/location/providers/remote/provider.cpp	2014-11-13 16:49:18 +0000
@@ -348,6 +348,38 @@
     });
 }
 
+void remote::Provider::Stub::disable()
+{
+    VLOG(10) << "> " << __PRETTY_FUNCTION__;
+    std::weak_ptr<Private> wp{d};
+    Runtime::instance().task.service.post([wp]()
+    {
+        auto sp = wp.lock();
+
+        if (not sp)
+            return;
+
+        throw_if_error(sp->stub.object->transact_method<remote::Interface::Disable, void>());
+    });
+    VLOG(10) << "< " << __PRETTY_FUNCTION__;
+}
+
+void remote::Provider::Stub::enable()
+{
+    VLOG(10) << "> " << __PRETTY_FUNCTION__;
+    std::weak_ptr<Private> wp{d};
+    Runtime::instance().task.service.post([wp]()
+    {
+        auto sp = wp.lock();
+
+        if (not sp)
+            return;
+
+        throw_if_error(sp->stub.object->transact_method<remote::Interface::Enable, void>());
+    });
+    VLOG(10) << "< " << __PRETTY_FUNCTION__;
+}
+
 void remote::Provider::Stub::start_position_updates()
 {
     VLOG(10) << "> " << __PRETTY_FUNCTION__;
@@ -563,6 +595,20 @@
         on_reference_velocity_updated(u);
     });
 
+    d->skeleton.object->install_method_handler<remote::Interface::Disable>([this](const dbus::Message::Ptr & msg)
+    {
+        VLOG(1) << "Disable";
+        disable();
+        d->bus->send(dbus::Message::make_method_return(msg));
+    });
+
+    d->skeleton.object->install_method_handler<remote::Interface::Enable>([this](const dbus::Message::Ptr & msg)
+    {
+        VLOG(1) << "Enable";
+        enable();
+        d->bus->send(dbus::Message::make_method_return(msg));
+    });
+
     d->skeleton.object->install_method_handler<remote::Interface::StartPositionUpdates>([this](const dbus::Message::Ptr & msg)
     {
         VLOG(1) << "StartPositionUpdates";
@@ -656,6 +702,16 @@
     d->impl->on_reference_heading_updated(heading);
 }
 
+void remote::Provider::Skeleton::disable()
+{
+    d->impl->state_controller()->state() = location::Provider::Controller::State::disabled;
+}
+
+void remote::Provider::Skeleton::enable()
+{
+    d->impl->state_controller()->state() = location::Provider::Controller::State::enabled;
+}
+
 void remote::Provider::Skeleton::start_position_updates()
 {
     d->impl->state_controller()->start_position_updates();

=== modified file 'src/location_service/com/ubuntu/location/providers/remote/provider.h'
--- src/location_service/com/ubuntu/location/providers/remote/provider.h	2014-10-27 21:58:16 +0000
+++ src/location_service/com/ubuntu/location/providers/remote/provider.h	2014-11-13 16:49:18 +0000
@@ -69,6 +69,9 @@
         virtual void on_reference_velocity_updated(const Update<Velocity>& velocity) override;
         virtual void on_reference_heading_updated(const Update<Heading>& heading) override;
 
+        virtual void disable() override;
+        virtual void enable() override;
+
         virtual void start_position_updates() override;
         virtual void stop_position_updates() override;
 
@@ -104,6 +107,9 @@
         virtual void on_reference_velocity_updated(const Update<Velocity>& velocity) override;
         virtual void on_reference_heading_updated(const Update<Heading>& heading) override;
 
+        virtual void disable() override;
+        virtual void enable() override;
+
         virtual void start_position_updates() override;
         virtual void stop_position_updates() override;
 

=== modified file 'tests/engine_test.cpp'
--- tests/engine_test.cpp	2014-06-01 17:56:58 +0000
+++ tests/engine_test.cpp	2014-11-13 16:49:18 +0000
@@ -35,6 +35,10 @@
     {
     }
 
+    MOCK_CONST_METHOD1(requires, bool(const location::Provider::Requirements&));
+
+    MOCK_METHOD0(disable, void());
+    MOCK_METHOD0(enable, void());
     MOCK_METHOD0(stop_position_updates, void());
     MOCK_METHOD0(stop_velocity_updates, void());
     MOCK_METHOD0(stop_heading_updates, void());
@@ -137,7 +141,7 @@
     engine.updates.reference_velocity = location::Update<location::Velocity>{};
 }
 
-TEST(Engine, switching_the_engine_off_results_in_updates_being_stopped)
+TEST(Engine, switching_the_engine_off_results_in_providers_being_disabled_and_updates_being_stopped)
 {
     using namespace ::testing;
 
@@ -150,9 +154,86 @@
     location::Engine engine{selection_policy};
     engine.add_provider(provider);
 
+    EXPECT_CALL(*provider, disable()).Times(1);
     EXPECT_CALL(*provider, stop_position_updates()).Times(1);
     EXPECT_CALL(*provider, stop_velocity_updates()).Times(1);
     EXPECT_CALL(*provider, stop_heading_updates()).Times(1);
 
     engine.configuration.engine_state = location::Engine::Status::off;
 }
+
+TEST(Engine, switching_the_engine_on_after_off_results_in_providers_being_enabled)
+{
+    using namespace ::testing;
+
+    auto provider = std::make_shared<NiceMock<MockProvider>>();
+
+    auto selection_policy = std::make_shared<NiceMock<MockProviderSelectionPolicy>>();
+    location::Engine engine{selection_policy};
+    engine.add_provider(provider);
+
+    EXPECT_CALL(*provider, disable()).Times(1);
+    EXPECT_CALL(*provider, enable()).Times(1);
+
+    engine.configuration.engine_state = location::Engine::Status::off;
+    engine.configuration.engine_state = location::Engine::Status::on;
+}
+
+TEST(Engine, switching_satellite_based_positioning_off_disables_providers_requiring_satellites)
+{
+    using namespace ::testing;
+
+    auto gps_provider = std::make_shared<NiceMock<MockProvider>>();
+    auto network_provider = std::make_shared<NiceMock<MockProvider>>();
+
+    ON_CALL(*gps_provider, requires(Ne(location::Provider::Requirements::satellites)))
+            .WillByDefault(Return(true));
+    ON_CALL(*gps_provider, requires(location::Provider::Requirements::satellites))
+            .WillByDefault(Return(true));
+    ON_CALL(*network_provider, requires(location::Provider::Requirements::satellites))
+            .WillByDefault(Return(false));
+
+    auto selection_policy = std::make_shared<NiceMock<MockProviderSelectionPolicy>>();
+    location::Engine engine{selection_policy};
+    engine.add_provider(gps_provider);
+    engine.add_provider(network_provider);
+
+    // Only the mocked gps provider requiring satellites will be disabled.
+    EXPECT_CALL(*gps_provider, disable()).Times(1);
+    // The network based provider will not be disabled.
+    EXPECT_CALL(*network_provider, disable()).Times(0);
+
+    engine.configuration.satellite_based_positioning_state = location::SatelliteBasedPositioningState::off;
+}
+
+TEST(Engine, switching_satellite_based_positioning_on_after_off_disables_and_enables_providers_requiring_satellites)
+{
+    using namespace ::testing;
+
+    auto gps_provider = std::make_shared<NiceMock<MockProvider>>();
+    auto network_provider = std::make_shared<NiceMock<MockProvider>>();
+
+    ON_CALL(*gps_provider, requires(Ne(location::Provider::Requirements::satellites)))
+            .WillByDefault(Return(true));
+    ON_CALL(*gps_provider, requires(location::Provider::Requirements::satellites))
+            .WillByDefault(Return(true));
+    ON_CALL(*network_provider, requires(location::Provider::Requirements::satellites))
+            .WillByDefault(Return(false));
+
+    auto selection_policy = std::make_shared<NiceMock<MockProviderSelectionPolicy>>();
+    location::Engine engine{selection_policy};
+    engine.add_provider(gps_provider);
+    engine.add_provider(network_provider);
+
+    // Only the mocked gps provider requiring satellites will be disabled.
+    EXPECT_CALL(*gps_provider, disable()).Times(1);
+    // Only the mocked gps provider requiring satellites will be disabled.
+    EXPECT_CALL(*gps_provider, enable()).Times(1);
+    // The network based provider will not be disabled.
+    EXPECT_CALL(*network_provider, disable()).Times(0);
+    // The network based provider will not be disabled.
+    EXPECT_CALL(*network_provider, enable()).Times(0);
+
+    engine.configuration.satellite_based_positioning_state = location::SatelliteBasedPositioningState::off;
+    engine.configuration.satellite_based_positioning_state = location::SatelliteBasedPositioningState::on;
+}

=== modified file 'tests/provider_test.cpp'
--- tests/provider_test.cpp	2014-06-20 07:40:34 +0000
+++ tests/provider_test.cpp	2014-11-13 16:49:18 +0000
@@ -186,6 +186,46 @@
     EXPECT_FALSE(provider.state_controller()->are_velocity_updates_running());
 }
 
+TEST(Provider, initial_state_of_controller_is_enabled)
+{
+    using namespace ::testing;
+
+    NiceMock<MockProvider> p;
+    EXPECT_EQ(cul::Provider::Controller::State::enabled, p.state_controller()->state());
+}
+
+TEST(Provider, starting_updates_on_a_disabled_provider_does_nothing)
+{
+    using namespace ::testing;
+
+    NiceMock<MockProvider> p;
+    EXPECT_CALL(p, start_position_updates()).Times(0);
+    EXPECT_CALL(p, start_heading_updates()).Times(0);
+    EXPECT_CALL(p, start_velocity_updates()).Times(0);
+
+    p.state_controller()->state() = cul::Provider::Controller::State::disabled;
+
+    p.state_controller()->start_position_updates();
+    p.state_controller()->start_heading_updates();
+    p.state_controller()->start_velocity_updates();
+}
+
+TEST(Provider, disabling_a_provider_stops_all_updates)
+{
+    using namespace ::testing;
+
+    NiceMock<MockProvider> p;
+    EXPECT_CALL(p, stop_position_updates()).Times(1);
+    EXPECT_CALL(p, stop_heading_updates()).Times(1);
+    EXPECT_CALL(p, stop_velocity_updates()).Times(1);
+
+    p.state_controller()->start_position_updates();
+    p.state_controller()->start_heading_updates();
+    p.state_controller()->start_velocity_updates();
+
+    p.state_controller()->state() = cul::Provider::Controller::State::disabled;
+}
+
 #include <com/ubuntu/location/proxy_provider.h>
 
 TEST(ProxyProvider, start_and_stop_of_updates_propagates_to_correct_providers)