Merge lp:~mandel/location-service/delayed-providers into lp:location-service/trunk

[Manuel de la Peña]

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.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

Please check inline comments.

review: Needs Fixing

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

See comment below

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:204
http://jenkins.qa.ubuntu.com/job/location-service-ci/389/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-amd64-ci/61
    FAILURE: http://jenkins.qa.ubuntu.com/job/location-service-vivid-armhf-ci/61/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-vivid-i386-ci/61

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

review: Needs Fixing (continuous-integration)

[Alfonso Sanchez-Beato (alfonsosanchezbeato)]

LGTM

review: Approve

[Jim Hodapp (jhodapp)]

Some issues to address, see comments inline below.

review: Needs Fixing (code)

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[lp:~mandel/location-service/delayed-providers]

206. By Manuel de la Peña on 2015-06-16

Made changes according to reviews.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

Unmerged revisions

206. By Manuel de la Peña on 2015-06-16

Made changes according to reviews.

205. By Manuel de la Peña on 2015-06-15

Update docs.

204. By Manuel de la Peña on 2015-06-15

Made chages as per review.

203. By Manuel de la Peña on 2015-06-15

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-06-16 08:14:07 +0000
@@ -150,6 +150,8 @@
         friend class Provider;
         explicit Controller(Provider& instance);
 
+        virtual void on_provider_booted(bool was_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>& booted_signal();
+
+    /**
+     * @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>& booted_signal() 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 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-06-16 08:14:07 +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,38 @@
     return velocity_updates_counter > 0;
 }
 
+void cul::Provider::Controller::on_provider_booted(bool was_booted)
+{
+    if (!was_booted)
+        return;
+
+    // we need to propagate the state of the provider using the internal counters as references
+    if (position_updates_counter > 0) {
+        instance.start_position_updates();
+    }
+    if (heading_updates_counter > 0) {
+        instance.start_heading_updates();
+    }
+    if (velocity_updates_counter > 0) {
+        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.booted_signal().connect(std::bind(&Controller::on_provider_booted, this, _1));
+        if (instance.boot_state() != BootState::BOOTING) {
+            instance.boot();
+        }
+    }
 }
 
 const cul::Provider::Controller::Ptr& cul::Provider::state_controller() const
@@ -158,11 +194,28 @@
     return false;
 }
 
+cul::Provider::BootState cul::Provider::boot_state() const
+{
+    return cul::Provider::BootState::BOOTED;
+}
+
+void cul::Provider::boot() { }
+
 const cul::Provider::Updates& cul::Provider::updates() const
 {
     return d.updates;
 }
 
+core::Signal<bool>& cul::Provider::booted_signal()
+{
+    return d.signals.booted;
+}
+
+const core::Signal<bool>& cul::Provider::booted_signal() 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-06-16 08:14:07 +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-06-16 08:14:07 +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_