Merge lp:~ssweeny/location-service/delayed-providers.15.04 into lp:location-service/15.04

[Scott Sweeny]

Add the concept of a delayed providers. This will allow providers that are slow to start the chance to be started and later be connected accordingly.

Add the concept of a delayed providers. This will allow providers that are slow to start the chance to be started and later be connected accordingly.

[Thomas Voß (thomas-voss)]

Please note that this change requires a rebuild of lp:espoo as the location::Provider interface is altered.

review: Needs Information

[Thomas Voß (thomas-voss)]

A few niggles inline.

review: Needs Fixing

[lp:~ssweeny/location-service/delayed-providers.15.04]

199. By Scott Sweeny on 2015-10-27

Address review comments

200. By Scott Sweeny on 2015-10-27

Add logging output (move here from espoo-delayed-providers MP)

201. By Scott Sweeny on 2015-10-27

Add a log message that I missed

[Thomas Voß (thomas-voss)]

LGTM.

review: Approve

[Alberto Mardegan (mardy)]

I'm coming from a programming style that keeps virtual methods to a minimum, and I do understand if that might not be of your liking, so feel free to discard this comment. :-)

Wouldn't it be possible to implement the same without breaking the ABI?

At least, I see no reason why these methods need to be virtual:

    virtual core::Signal<bool>& boot_state_changed();
    virtual const core::Signal<bool>& boot_state_changed() const;

given that you are providing a default implementation that I don't see any reason to override. As for the getter:

    virtual BootState boot_state() const;

this could be made non virtual by adding a protected method like

    void set_boot_state(BootState state);

that subclasses can call to set the boot state. This method would also take care of emitting the boot_state_changed() signal.

The only method for which a virtual declaration makes a lot of sense is request_boot(), but OTOH I wonder if we could remove it altogether: we could just specify that boot should start when the provider's constructor is invoked, or (if we really need a two-phase initialization) when the

    virtual const Controller::Ptr& state_controller() const;

method is first invoked (it's a virtual method, so the subclass can use it to start the initialization). I can see that purists might not like to have this method be overloaded with the booting role too, but it's something that can make sense, if clearly explained in a doc comment.

It all depends on whether preserving ABI is a goal or not.

Unmerged revisions

201. By Scott Sweeny on 2015-10-27

Add a log message that I missed

200. By Scott Sweeny on 2015-10-27

Add logging output (move here from espoo-delayed-providers MP)

199. By Scott Sweeny on 2015-10-27

Address review comments

198. By Manuel de la Peña on 2015-10-22

Added support for delayed providers.

=== modified file 'include/location_service/com/ubuntu/location/provider.h'
--- include/location_service/com/ubuntu/location/provider.h	2015-01-21 20:13:28 +0000
+++ include/location_service/com/ubuntu/location/provider.h	2015-10-27 20:49:11 +0000
@@ -150,6 +150,8 @@
         friend class Provider;
         explicit Controller(Provider& instance);
 
+        virtual void on_provider_booted();
+
     private:
         Provider& instance;
         std::atomic<int> position_updates_counter;
@@ -172,6 +174,20 @@
         core::Signal<Update<std::set<SpaceVehicle>>> svs;
     };
 
+    /**
+     * @brief Wraps al those sigals that are related to the boot of the provider.
+     */
+    struct Signals
+    {
+        core::Signal<bool> booted;
+    };
+
+    enum class BootState {
+        NOT_BOOTED,
+        BOOTING,
+        BOOTED
+    };
+
     virtual ~Provider() = default;
 
     Provider(const Provider&) = delete;
@@ -189,6 +205,18 @@
     virtual const Controller::Ptr& state_controller() const;
 
     /**
+     * @brief Returns the booted signal to track the booting state of the provider.
+     * @return The signal that will be triggered once the provider has booted.
+     */
+    virtual core::Signal<bool>& boot_state_changed();
+
+    /**
+     * @brief Returns the booted signal to track the booting state of the provider.
+     * @return The signal that will be triggered once the provider has booted.
+     */
+    virtual const core::Signal<bool>& boot_state_changed() const;
+
+    /**
      * @brief Checks if the provider supports a specific feature.
      * @param f Feature to test for
      * @return true iff the provider supports the feature.
@@ -210,6 +238,18 @@
     virtual bool matches_criteria(const Criteria& criteria);
 
     /**
+     * @brief States the booting state of the provider.
+     * @return The boot state of the provider.
+     */
+    virtual BootState boot_state() const;
+
+    /**
+     * @brief Indicates to the provider that it should start the boot process in order to be able to
+     *        be used by the engine.
+     */
+    virtual void request_boot();
+
+    /**
      * @brief Called by the engine whenever the wifi and cell ID reporting state changes.
      * @param state The new state.
      */
@@ -270,12 +310,14 @@
      */
     virtual void stop_velocity_updates();
 
-private:
+protected:
+    // protected to allow better white box testing
     struct
     {
         Features features = Features::none;
         Requirements requirements = Requirements::none;
         Updates updates;
+        Signals signals;
         Controller::Ptr controller = Controller::Ptr{};
     } d;
 };

=== modified file 'src/location_service/com/ubuntu/location/provider.cpp'
--- src/location_service/com/ubuntu/location/provider.cpp	2015-01-23 19:07:09 +0000
+++ src/location_service/com/ubuntu/location/provider.cpp	2015-10-27 20:49:11 +0000
@@ -16,9 +16,11 @@
  * Authored by: Thomas Voß <[e-mail]>
  */
 #include <com/ubuntu/location/provider.h>
+#include <com/ubuntu/location/logging.h>
 
 #include <atomic>
 #include <bitset>
+#include <functional>
 #include <memory>
 
 namespace cul = com::ubuntu::location;
@@ -51,12 +53,15 @@
 
 void cul::Provider::Controller::start_position_updates()
 {
-    if (position_updates_counter < 0)
+    if (position_updates_counter < 0) {
         return;
+    }
 
     if (++position_updates_counter == 1)
     {
-        instance.start_position_updates();
+        if (instance.boot_state() == BootState::BOOTED) {
+            instance.start_position_updates();
+        }
     }
 }
 
@@ -67,7 +72,8 @@
 
     if (--position_updates_counter == 0)
     {
-        instance.stop_position_updates();
+        if (instance.boot_state() == BootState::BOOTED)
+            instance.stop_position_updates();
     }
 }
 
@@ -83,7 +89,8 @@
 
     if (++heading_updates_counter == 1)
     {
-        instance.start_heading_updates();
+        if (instance.boot_state() == BootState::BOOTED)
+            instance.start_heading_updates();
     }
 }
 
@@ -94,7 +101,8 @@
 
     if (--heading_updates_counter == 0)
     {
-        instance.stop_heading_updates();
+        if (instance.boot_state() == BootState::BOOTED)
+            instance.stop_heading_updates();
     }
 }
 
@@ -110,7 +118,8 @@
 
     if (++velocity_updates_counter == 1)
     {
-        instance.start_velocity_updates();
+        if (instance.boot_state() == BootState::BOOTED)
+            instance.start_velocity_updates();
     }
 }
 
@@ -121,7 +130,8 @@
 
     if (--velocity_updates_counter == 0)
     {
-        instance.stop_velocity_updates();
+        if (instance.boot_state() == BootState::BOOTED)
+            instance.stop_velocity_updates();
     }
 }
 
@@ -130,12 +140,43 @@
     return velocity_updates_counter > 0;
 }
 
+void cul::Provider::Controller::on_provider_booted()
+{
+    if (instance.boot_state() != BootState::BOOTED)
+        return;
+
+    LOG(INFO) << "Provider was booted";
+
+    // we need to propagate the state of the provider using the internal counters as references
+    if (position_updates_counter > 0) {
+        LOG(INFO) << "Starting position updates";
+        instance.start_position_updates();
+    }
+    if (heading_updates_counter > 0) {
+        LOG(INFO) << "Starting heading updates";
+        instance.start_heading_updates();
+    }
+    if (velocity_updates_counter > 0) {
+        LOG(INFO) << "Starting velocity updates";
+        instance.start_velocity_updates();
+    }
+}
+
 cul::Provider::Controller::Controller(cul::Provider& instance)
     : instance(instance),
       position_updates_counter(0),
       heading_updates_counter(0),
       velocity_updates_counter(0)
-{  
+{
+    using namespace std::placeholders;
+
+    if (instance.boot_state() != BootState::BOOTED) {
+        // if the instance has not booted and is delayed we need to keep track of its booting state
+        instance.boot_state_changed().connect(std::bind(&Controller::on_provider_booted, this));
+        if (instance.boot_state() != BootState::BOOTING) {
+            instance.request_boot();
+        }
+    }
 }
 
 const cul::Provider::Controller::Ptr& cul::Provider::state_controller() const
@@ -158,11 +199,28 @@
     return false;
 }
 
+cul::Provider::BootState cul::Provider::boot_state() const
+{
+    return cul::Provider::BootState::BOOTED;
+}
+
+void cul::Provider::request_boot() { }
+
 const cul::Provider::Updates& cul::Provider::updates() const
 {
     return d.updates;
 }
 
+core::Signal<bool>& cul::Provider::boot_state_changed()
+{
+    return d.signals.booted;
+}
+
+const core::Signal<bool>& cul::Provider::boot_state_changed() const
+{
+    return d.signals.booted;
+}
+
 cul::Provider::Provider(
     const cul::Provider::Features& features,
     const cul::Provider::Requirements& requirements)

=== modified file 'tests/controller_test.cpp'
--- tests/controller_test.cpp	2014-09-10 19:34:09 +0000
+++ tests/controller_test.cpp	2015-10-27 20:49:11 +0000
@@ -18,6 +18,7 @@
 #include <com/ubuntu/location/provider.h>
 
 #include "mock_provider.h"
+#include "mock_delayed_provider.h"
 
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
@@ -130,3 +131,215 @@
     EXPECT_FALSE(controller->are_velocity_updates_running());
     EXPECT_FALSE(controller->are_heading_updates_running());
 }
+
+TEST(Controller, controller_resumes_state_after_boot) {
+    using namespace ::testing;
+    using ::testing::Mock;
+
+    MockDelayedProvider provider;
+
+    // ignore what ever was performed on construction
+    Mock::VerifyAndClear(&provider);
+
+}
+
+TEST(Controller, controller_does_not_call_start_position_if_not_booted) {
+    using namespace ::testing;
+    using ::testing::Mock;
+
+    MockDelayedProvider provider;
+
+    // ignore what ever was performed on construction
+    Mock::VerifyAndClear(&provider);
+    // use a mock controller
+    auto controller = provider.state_controller();
+
+    EXPECT_CALL(provider, boot_state()).Times(Exactly(1))
+            .WillOnce(Return(cul::Provider::BootState::NOT_BOOTED));
+    EXPECT_CALL(provider, start_position_updates()).Times(Exactly(0));
+
+    controller->start_position_updates();
+    controller->start_position_updates();
+}
+
+TEST(Controller, controller_does_not_call_start_position_if_booting) {
+    using namespace ::testing;
+    using ::testing::Mock;
+
+    MockDelayedProvider provider;
+
+    // ignore what ever was performed on construction
+    Mock::VerifyAndClear(&provider);
+    // use a mock controller
+    auto controller = provider.state_controller();
+
+    EXPECT_CALL(provider, boot_state()).Times(Exactly(1))
+            .WillOnce(Return(cul::Provider::BootState::BOOTING));
+    EXPECT_CALL(provider, start_position_updates()).Times(Exactly(0));
+
+    controller->start_position_updates();
+}
+
+TEST(Controller, controller_does_not_call_start_heading_if_not_booted) {
+    using namespace ::testing;
+    using ::testing::Mock;
+
+    MockDelayedProvider provider;
+    EXPECT_CALL(provider, has_delayed_boot()).WillRepeatedly(Return(true));
+
+    // ignore what ever was performed on construction
+    Mock::VerifyAndClear(&provider);
+    // use a mock controller
+    auto controller = provider.state_controller();
+
+    EXPECT_CALL(provider, boot_state()).Times(Exactly(1))
+            .WillOnce(Return(cul::Provider::BootState::NOT_BOOTED));
+    EXPECT_CALL(provider, start_heading_updates()).Times(Exactly(0));
+
+    controller->start_heading_updates();
+}
+
+TEST(Controller, controller_does_not_call_start_heading_if_booting) {
+    using namespace ::testing;
+    using ::testing::Mock;
+
+    MockDelayedProvider provider;
+    EXPECT_CALL(provider, has_delayed_boot()).WillRepeatedly(Return(true));
+
+    // ignore what ever was performed on construction
+    Mock::VerifyAndClear(&provider);
+    // use a mock controller
+    auto controller = provider.state_controller();
+
+    EXPECT_CALL(provider, boot_state()).Times(Exactly(1))
+            .WillOnce(Return(cul::Provider::BootState::BOOTING));
+    EXPECT_CALL(provider, start_heading_updates()).Times(Exactly(0));
+
+    controller->start_heading_updates();
+}
+
+TEST(Controller, controller_does_not_call_start_velocity_if_not_booted) {
+    using namespace ::testing;
+    using ::testing::Mock;
+
+    MockDelayedProvider provider;
+    EXPECT_CALL(provider, has_delayed_boot()).WillRepeatedly(Return(true));
+
+    // ignore what ever was performed on construction
+    Mock::VerifyAndClear(&provider);
+    // use a mock controller
+    auto controller = provider.state_controller();
+
+    EXPECT_CALL(provider, boot_state()).Times(Exactly(1))
+            .WillOnce(Return(cul::Provider::BootState::NOT_BOOTED));
+    EXPECT_CALL(provider, start_heading_updates()).Times(Exactly(0));
+
+    controller->start_velocity_updates();
+}
+
+TEST(Controller, controller_does_not_call_start_velocity_if_booting) {
+    using namespace ::testing;
+    using ::testing::Mock;
+
+    MockDelayedProvider provider;
+    EXPECT_CALL(provider, has_delayed_boot()).WillRepeatedly(Return(true));
+
+    // ignore what ever was performed on construction
+    Mock::VerifyAndClear(&provider);
+    // use a mock controller
+    auto controller = provider.state_controller();
+
+    EXPECT_CALL(provider, boot_state()).Times(Exactly(1))
+            .WillOnce(Return(cul::Provider::BootState::BOOTING));
+    EXPECT_CALL(provider, start_heading_updates()).Times(Exactly(0));
+
+    controller->start_velocity_updates();
+}
+
+TEST(Controller, controller_does_not_call_stop_position_if_not_booted) {
+    using namespace ::testing;
+    using ::testing::Mock;
+
+    MockDelayedProvider provider;
+    EXPECT_CALL(provider, has_delayed_boot()).WillRepeatedly(Return(true));
+
+    // ignore what ever was performed on construction
+    Mock::VerifyAndClear(&provider);
+    // use a mock controller
+    auto controller = provider.state_controller();
+
+    EXPECT_CALL(provider, boot_state()).Times(Exactly(2))
+            .WillOnce(Return(cul::Provider::BootState::BOOTED))
+            .WillOnce(Return(cul::Provider::BootState::NOT_BOOTED));
+    EXPECT_CALL(provider, start_position_updates()).Times(Exactly(1));
+    EXPECT_CALL(provider, stop_position_updates()).Times(Exactly(0));
+
+    controller->start_position_updates();
+    controller->stop_position_updates();
+}
+
+TEST(Controller, controller_does_not_call_stop_position_if_booting) {
+    using namespace ::testing;
+    using ::testing::Mock;
+
+    MockDelayedProvider provider;
+    EXPECT_CALL(provider, has_delayed_boot()).WillRepeatedly(Return(true));
+
+    // ignore what ever was performed on construction
+    Mock::VerifyAndClear(&provider);
+    // use a mock controller
+    auto controller = provider.state_controller();
+
+    EXPECT_CALL(provider, boot_state()).Times(Exactly(2))
+            .WillOnce(Return(cul::Provider::BootState::BOOTED))
+            .WillOnce(Return(cul::Provider::BootState::BOOTING));
+    EXPECT_CALL(provider, start_position_updates()).Times(Exactly(1));
+    EXPECT_CALL(provider, stop_position_updates()).Times(Exactly(0));
+
+    controller->start_position_updates();
+    controller->stop_position_updates();
+}
+
+TEST(Controller, controller_does_not_call_stop_heading_if_not_booted) {
+    using namespace ::testing;
+    using ::testing::Mock;
+
+    MockDelayedProvider provider;
+    EXPECT_CALL(provider, has_delayed_boot()).WillRepeatedly(Return(true));
+
+    // ignore what ever was performed on construction
+    Mock::VerifyAndClear(&provider);
+    // use a mock controller
+    auto controller = provider.state_controller();
+
+    EXPECT_CALL(provider, boot_state()).Times(Exactly(2))
+            .WillOnce(Return(cul::Provider::BootState::BOOTED))
+            .WillOnce(Return(cul::Provider::BootState::BOOTING));
+    EXPECT_CALL(provider, start_heading_updates()).Times(Exactly(1));
+    EXPECT_CALL(provider, stop_heading_updates()).Times(Exactly(0));
+
+    controller->start_heading_updates();
+    controller->stop_heading_updates();
+}
+
+TEST(Controller, controller_does_not_call_stop_velocity_if_not_booted) {
+    using namespace ::testing;
+    using ::testing::Mock;
+
+    MockDelayedProvider provider;
+    EXPECT_CALL(provider, has_delayed_boot()).WillRepeatedly(Return(true));
+
+    // ignore what ever was performed on construction
+    Mock::VerifyAndClear(&provider);
+    // use a mock controller
+    auto controller = provider.state_controller();
+
+    EXPECT_CALL(provider, boot_state()).Times(Exactly(2))
+            .WillOnce(Return(cul::Provider::BootState::BOOTED))
+            .WillOnce(Return(cul::Provider::BootState::BOOTING));
+    EXPECT_CALL(provider, start_velocity_updates()).Times(Exactly(1));
+    EXPECT_CALL(provider, stop_velocity_updates()).Times(Exactly(0));
+
+    controller->start_velocity_updates();
+    controller->stop_velocity_updates();
+}

=== added file 'tests/mock_delayed_provider.h'
--- tests/mock_delayed_provider.h	1970-01-01 00:00:00 +0000
+++ tests/mock_delayed_provider.h	2015-10-27 20:49:11 +0000
@@ -0,0 +1,89 @@
+/*
+ * Copyright © 2014 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: M
+ */
+#ifndef MOCK_DELAYED_PROVIDER_H_
+#define MOCK_DELAYED_PROVIDER_H_
+
+#include <com/ubuntu/location/provider.h>
+#include <com/ubuntu/location/logging.h>
+
+#include <gmock/gmock.h>
+
+struct PublicController : public com::ubuntu::location::Provider::Controller
+{
+
+    explicit PublicController(com::ubuntu::location::Provider& instance)
+            : com::ubuntu::location::Provider::Controller(instance) {}
+};
+
+struct MockDelayedProvider : public com::ubuntu::location::Provider
+{
+    MockDelayedProvider() : com::ubuntu::location::Provider()
+    {
+    }
+
+    MOCK_METHOD1(matches_criteria, bool(const com::ubuntu::location::Criteria&));
+
+    MOCK_CONST_METHOD1(supports, bool(const com::ubuntu::location::Provider::Features&));
+    MOCK_CONST_METHOD1(requires, bool(const com::ubuntu::location::Provider::Requirements&));
+
+    // Called by the engine whenever the wifi and cell ID reporting state changes.
+    MOCK_METHOD1(on_wifi_and_cell_reporting_state_changed, void(com::ubuntu::location::WifiAndCellIdReportingState state));
+
+    // Called by the engine whenever the reference location changed.
+    MOCK_METHOD1(on_reference_location_updated, void(const com::ubuntu::location::Update<com::ubuntu::location::Position>& position));
+
+    // Called by the engine whenever the reference velocity changed.
+    MOCK_METHOD1(on_reference_velocity_updated, void(const com::ubuntu::location::Update<com::ubuntu::location::Velocity>& velocity));
+
+    // Called by the engine whenever the reference heading changed.
+    MOCK_METHOD1(on_reference_heading_updated, void(const com::ubuntu::location::Update<com::ubuntu::location::Heading>& heading));
+
+    MOCK_METHOD0(start_position_updates, void());
+    MOCK_METHOD0(stop_position_updates, void());
+
+    MOCK_METHOD0(start_heading_updates, void());
+    MOCK_METHOD0(stop_heading_updates, void());
+
+    MOCK_METHOD0(start_velocity_updates, void());
+    MOCK_METHOD0(stop_velocity_updates, void());
+
+    MOCK_CONST_METHOD0(has_delayed_boot, bool());
+    MOCK_CONST_METHOD0(boot_state, BootState());
+    MOCK_CONST_METHOD0(boot, void());
+
+
+    // Inject a position update from the outside.
+    void inject_update(const com::ubuntu::location::Update<com::ubuntu::location::Position>& update)
+    {
+        mutable_updates().position(update);
+    }
+
+    // Inject a velocity update from the outside.
+    void inject_update(const com::ubuntu::location::Update<com::ubuntu::location::Velocity>& update)
+    {
+        mutable_updates().velocity(update);
+    }
+
+    // Inject a heading update from the outside.
+    void inject_update(const com::ubuntu::location::Update<com::ubuntu::location::Heading>& update)
+    {
+        mutable_updates().heading(update);
+    }
+};
+
+#endif // MOCK_PROVIDER_H_