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

Proposed by Scott Sweeny on 2015-10-22
Status: Approved
Approved by: Thomas Voß on 2015-10-28
Approved revision: 201
Proposed branch: lp:~ssweeny/location-service/delayed-providers.15.04
Merge into: lp:location-service/15.04
Diff against target: 567 lines (+411/-9)
4 files modified
include/location_service/com/ubuntu/location/provider.h (+43/-1)
src/location_service/com/ubuntu/location/provider.cpp (+66/-8)
tests/controller_test.cpp (+213/-0)
tests/mock_delayed_provider.h (+89/-0)
To merge this branch: bzr merge lp:~ssweeny/location-service/delayed-providers.15.04
Reviewer Review Type Date Requested Status
Thomas Voß (community) 2015-10-22 Approve on 2015-10-28
Review via email: mp+275384@code.launchpad.net

Commit message

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.

Description of the change

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.

To post a comment you must log in.
Thomas Voß (thomas-voss) wrote :

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) wrote :

A few niggles inline.

review: Needs Fixing
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) wrote :

LGTM.

review: Approve
Alberto Mardegan (mardy) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/location_service/com/ubuntu/location/provider.h'
2--- include/location_service/com/ubuntu/location/provider.h 2015-01-21 20:13:28 +0000
3+++ include/location_service/com/ubuntu/location/provider.h 2015-10-27 20:49:11 +0000
4@@ -150,6 +150,8 @@
5 friend class Provider;
6 explicit Controller(Provider& instance);
7
8+ virtual void on_provider_booted();
9+
10 private:
11 Provider& instance;
12 std::atomic<int> position_updates_counter;
13@@ -172,6 +174,20 @@
14 core::Signal<Update<std::set<SpaceVehicle>>> svs;
15 };
16
17+ /**
18+ * @brief Wraps al those sigals that are related to the boot of the provider.
19+ */
20+ struct Signals
21+ {
22+ core::Signal<bool> booted;
23+ };
24+
25+ enum class BootState {
26+ NOT_BOOTED,
27+ BOOTING,
28+ BOOTED
29+ };
30+
31 virtual ~Provider() = default;
32
33 Provider(const Provider&) = delete;
34@@ -189,6 +205,18 @@
35 virtual const Controller::Ptr& state_controller() const;
36
37 /**
38+ * @brief Returns the booted signal to track the booting state of the provider.
39+ * @return The signal that will be triggered once the provider has booted.
40+ */
41+ virtual core::Signal<bool>& boot_state_changed();
42+
43+ /**
44+ * @brief Returns the booted signal to track the booting state of the provider.
45+ * @return The signal that will be triggered once the provider has booted.
46+ */
47+ virtual const core::Signal<bool>& boot_state_changed() const;
48+
49+ /**
50 * @brief Checks if the provider supports a specific feature.
51 * @param f Feature to test for
52 * @return true iff the provider supports the feature.
53@@ -210,6 +238,18 @@
54 virtual bool matches_criteria(const Criteria& criteria);
55
56 /**
57+ * @brief States the booting state of the provider.
58+ * @return The boot state of the provider.
59+ */
60+ virtual BootState boot_state() const;
61+
62+ /**
63+ * @brief Indicates to the provider that it should start the boot process in order to be able to
64+ * be used by the engine.
65+ */
66+ virtual void request_boot();
67+
68+ /**
69 * @brief Called by the engine whenever the wifi and cell ID reporting state changes.
70 * @param state The new state.
71 */
72@@ -270,12 +310,14 @@
73 */
74 virtual void stop_velocity_updates();
75
76-private:
77+protected:
78+ // protected to allow better white box testing
79 struct
80 {
81 Features features = Features::none;
82 Requirements requirements = Requirements::none;
83 Updates updates;
84+ Signals signals;
85 Controller::Ptr controller = Controller::Ptr{};
86 } d;
87 };
88
89=== modified file 'src/location_service/com/ubuntu/location/provider.cpp'
90--- src/location_service/com/ubuntu/location/provider.cpp 2015-01-23 19:07:09 +0000
91+++ src/location_service/com/ubuntu/location/provider.cpp 2015-10-27 20:49:11 +0000
92@@ -16,9 +16,11 @@
93 * Authored by: Thomas Voß <thomas.voss@canonical.com>
94 */
95 #include <com/ubuntu/location/provider.h>
96+#include <com/ubuntu/location/logging.h>
97
98 #include <atomic>
99 #include <bitset>
100+#include <functional>
101 #include <memory>
102
103 namespace cul = com::ubuntu::location;
104@@ -51,12 +53,15 @@
105
106 void cul::Provider::Controller::start_position_updates()
107 {
108- if (position_updates_counter < 0)
109+ if (position_updates_counter < 0) {
110 return;
111+ }
112
113 if (++position_updates_counter == 1)
114 {
115- instance.start_position_updates();
116+ if (instance.boot_state() == BootState::BOOTED) {
117+ instance.start_position_updates();
118+ }
119 }
120 }
121
122@@ -67,7 +72,8 @@
123
124 if (--position_updates_counter == 0)
125 {
126- instance.stop_position_updates();
127+ if (instance.boot_state() == BootState::BOOTED)
128+ instance.stop_position_updates();
129 }
130 }
131
132@@ -83,7 +89,8 @@
133
134 if (++heading_updates_counter == 1)
135 {
136- instance.start_heading_updates();
137+ if (instance.boot_state() == BootState::BOOTED)
138+ instance.start_heading_updates();
139 }
140 }
141
142@@ -94,7 +101,8 @@
143
144 if (--heading_updates_counter == 0)
145 {
146- instance.stop_heading_updates();
147+ if (instance.boot_state() == BootState::BOOTED)
148+ instance.stop_heading_updates();
149 }
150 }
151
152@@ -110,7 +118,8 @@
153
154 if (++velocity_updates_counter == 1)
155 {
156- instance.start_velocity_updates();
157+ if (instance.boot_state() == BootState::BOOTED)
158+ instance.start_velocity_updates();
159 }
160 }
161
162@@ -121,7 +130,8 @@
163
164 if (--velocity_updates_counter == 0)
165 {
166- instance.stop_velocity_updates();
167+ if (instance.boot_state() == BootState::BOOTED)
168+ instance.stop_velocity_updates();
169 }
170 }
171
172@@ -130,12 +140,43 @@
173 return velocity_updates_counter > 0;
174 }
175
176+void cul::Provider::Controller::on_provider_booted()
177+{
178+ if (instance.boot_state() != BootState::BOOTED)
179+ return;
180+
181+ LOG(INFO) << "Provider was booted";
182+
183+ // we need to propagate the state of the provider using the internal counters as references
184+ if (position_updates_counter > 0) {
185+ LOG(INFO) << "Starting position updates";
186+ instance.start_position_updates();
187+ }
188+ if (heading_updates_counter > 0) {
189+ LOG(INFO) << "Starting heading updates";
190+ instance.start_heading_updates();
191+ }
192+ if (velocity_updates_counter > 0) {
193+ LOG(INFO) << "Starting velocity updates";
194+ instance.start_velocity_updates();
195+ }
196+}
197+
198 cul::Provider::Controller::Controller(cul::Provider& instance)
199 : instance(instance),
200 position_updates_counter(0),
201 heading_updates_counter(0),
202 velocity_updates_counter(0)
203-{
204+{
205+ using namespace std::placeholders;
206+
207+ if (instance.boot_state() != BootState::BOOTED) {
208+ // if the instance has not booted and is delayed we need to keep track of its booting state
209+ instance.boot_state_changed().connect(std::bind(&Controller::on_provider_booted, this));
210+ if (instance.boot_state() != BootState::BOOTING) {
211+ instance.request_boot();
212+ }
213+ }
214 }
215
216 const cul::Provider::Controller::Ptr& cul::Provider::state_controller() const
217@@ -158,11 +199,28 @@
218 return false;
219 }
220
221+cul::Provider::BootState cul::Provider::boot_state() const
222+{
223+ return cul::Provider::BootState::BOOTED;
224+}
225+
226+void cul::Provider::request_boot() { }
227+
228 const cul::Provider::Updates& cul::Provider::updates() const
229 {
230 return d.updates;
231 }
232
233+core::Signal<bool>& cul::Provider::boot_state_changed()
234+{
235+ return d.signals.booted;
236+}
237+
238+const core::Signal<bool>& cul::Provider::boot_state_changed() const
239+{
240+ return d.signals.booted;
241+}
242+
243 cul::Provider::Provider(
244 const cul::Provider::Features& features,
245 const cul::Provider::Requirements& requirements)
246
247=== modified file 'tests/controller_test.cpp'
248--- tests/controller_test.cpp 2014-09-10 19:34:09 +0000
249+++ tests/controller_test.cpp 2015-10-27 20:49:11 +0000
250@@ -18,6 +18,7 @@
251 #include <com/ubuntu/location/provider.h>
252
253 #include "mock_provider.h"
254+#include "mock_delayed_provider.h"
255
256 #include <gmock/gmock.h>
257 #include <gtest/gtest.h>
258@@ -130,3 +131,215 @@
259 EXPECT_FALSE(controller->are_velocity_updates_running());
260 EXPECT_FALSE(controller->are_heading_updates_running());
261 }
262+
263+TEST(Controller, controller_resumes_state_after_boot) {
264+ using namespace ::testing;
265+ using ::testing::Mock;
266+
267+ MockDelayedProvider provider;
268+
269+ // ignore what ever was performed on construction
270+ Mock::VerifyAndClear(&provider);
271+
272+}
273+
274+TEST(Controller, controller_does_not_call_start_position_if_not_booted) {
275+ using namespace ::testing;
276+ using ::testing::Mock;
277+
278+ MockDelayedProvider provider;
279+
280+ // ignore what ever was performed on construction
281+ Mock::VerifyAndClear(&provider);
282+ // use a mock controller
283+ auto controller = provider.state_controller();
284+
285+ EXPECT_CALL(provider, boot_state()).Times(Exactly(1))
286+ .WillOnce(Return(cul::Provider::BootState::NOT_BOOTED));
287+ EXPECT_CALL(provider, start_position_updates()).Times(Exactly(0));
288+
289+ controller->start_position_updates();
290+ controller->start_position_updates();
291+}
292+
293+TEST(Controller, controller_does_not_call_start_position_if_booting) {
294+ using namespace ::testing;
295+ using ::testing::Mock;
296+
297+ MockDelayedProvider provider;
298+
299+ // ignore what ever was performed on construction
300+ Mock::VerifyAndClear(&provider);
301+ // use a mock controller
302+ auto controller = provider.state_controller();
303+
304+ EXPECT_CALL(provider, boot_state()).Times(Exactly(1))
305+ .WillOnce(Return(cul::Provider::BootState::BOOTING));
306+ EXPECT_CALL(provider, start_position_updates()).Times(Exactly(0));
307+
308+ controller->start_position_updates();
309+}
310+
311+TEST(Controller, controller_does_not_call_start_heading_if_not_booted) {
312+ using namespace ::testing;
313+ using ::testing::Mock;
314+
315+ MockDelayedProvider provider;
316+ EXPECT_CALL(provider, has_delayed_boot()).WillRepeatedly(Return(true));
317+
318+ // ignore what ever was performed on construction
319+ Mock::VerifyAndClear(&provider);
320+ // use a mock controller
321+ auto controller = provider.state_controller();
322+
323+ EXPECT_CALL(provider, boot_state()).Times(Exactly(1))
324+ .WillOnce(Return(cul::Provider::BootState::NOT_BOOTED));
325+ EXPECT_CALL(provider, start_heading_updates()).Times(Exactly(0));
326+
327+ controller->start_heading_updates();
328+}
329+
330+TEST(Controller, controller_does_not_call_start_heading_if_booting) {
331+ using namespace ::testing;
332+ using ::testing::Mock;
333+
334+ MockDelayedProvider provider;
335+ EXPECT_CALL(provider, has_delayed_boot()).WillRepeatedly(Return(true));
336+
337+ // ignore what ever was performed on construction
338+ Mock::VerifyAndClear(&provider);
339+ // use a mock controller
340+ auto controller = provider.state_controller();
341+
342+ EXPECT_CALL(provider, boot_state()).Times(Exactly(1))
343+ .WillOnce(Return(cul::Provider::BootState::BOOTING));
344+ EXPECT_CALL(provider, start_heading_updates()).Times(Exactly(0));
345+
346+ controller->start_heading_updates();
347+}
348+
349+TEST(Controller, controller_does_not_call_start_velocity_if_not_booted) {
350+ using namespace ::testing;
351+ using ::testing::Mock;
352+
353+ MockDelayedProvider provider;
354+ EXPECT_CALL(provider, has_delayed_boot()).WillRepeatedly(Return(true));
355+
356+ // ignore what ever was performed on construction
357+ Mock::VerifyAndClear(&provider);
358+ // use a mock controller
359+ auto controller = provider.state_controller();
360+
361+ EXPECT_CALL(provider, boot_state()).Times(Exactly(1))
362+ .WillOnce(Return(cul::Provider::BootState::NOT_BOOTED));
363+ EXPECT_CALL(provider, start_heading_updates()).Times(Exactly(0));
364+
365+ controller->start_velocity_updates();
366+}
367+
368+TEST(Controller, controller_does_not_call_start_velocity_if_booting) {
369+ using namespace ::testing;
370+ using ::testing::Mock;
371+
372+ MockDelayedProvider provider;
373+ EXPECT_CALL(provider, has_delayed_boot()).WillRepeatedly(Return(true));
374+
375+ // ignore what ever was performed on construction
376+ Mock::VerifyAndClear(&provider);
377+ // use a mock controller
378+ auto controller = provider.state_controller();
379+
380+ EXPECT_CALL(provider, boot_state()).Times(Exactly(1))
381+ .WillOnce(Return(cul::Provider::BootState::BOOTING));
382+ EXPECT_CALL(provider, start_heading_updates()).Times(Exactly(0));
383+
384+ controller->start_velocity_updates();
385+}
386+
387+TEST(Controller, controller_does_not_call_stop_position_if_not_booted) {
388+ using namespace ::testing;
389+ using ::testing::Mock;
390+
391+ MockDelayedProvider provider;
392+ EXPECT_CALL(provider, has_delayed_boot()).WillRepeatedly(Return(true));
393+
394+ // ignore what ever was performed on construction
395+ Mock::VerifyAndClear(&provider);
396+ // use a mock controller
397+ auto controller = provider.state_controller();
398+
399+ EXPECT_CALL(provider, boot_state()).Times(Exactly(2))
400+ .WillOnce(Return(cul::Provider::BootState::BOOTED))
401+ .WillOnce(Return(cul::Provider::BootState::NOT_BOOTED));
402+ EXPECT_CALL(provider, start_position_updates()).Times(Exactly(1));
403+ EXPECT_CALL(provider, stop_position_updates()).Times(Exactly(0));
404+
405+ controller->start_position_updates();
406+ controller->stop_position_updates();
407+}
408+
409+TEST(Controller, controller_does_not_call_stop_position_if_booting) {
410+ using namespace ::testing;
411+ using ::testing::Mock;
412+
413+ MockDelayedProvider provider;
414+ EXPECT_CALL(provider, has_delayed_boot()).WillRepeatedly(Return(true));
415+
416+ // ignore what ever was performed on construction
417+ Mock::VerifyAndClear(&provider);
418+ // use a mock controller
419+ auto controller = provider.state_controller();
420+
421+ EXPECT_CALL(provider, boot_state()).Times(Exactly(2))
422+ .WillOnce(Return(cul::Provider::BootState::BOOTED))
423+ .WillOnce(Return(cul::Provider::BootState::BOOTING));
424+ EXPECT_CALL(provider, start_position_updates()).Times(Exactly(1));
425+ EXPECT_CALL(provider, stop_position_updates()).Times(Exactly(0));
426+
427+ controller->start_position_updates();
428+ controller->stop_position_updates();
429+}
430+
431+TEST(Controller, controller_does_not_call_stop_heading_if_not_booted) {
432+ using namespace ::testing;
433+ using ::testing::Mock;
434+
435+ MockDelayedProvider provider;
436+ EXPECT_CALL(provider, has_delayed_boot()).WillRepeatedly(Return(true));
437+
438+ // ignore what ever was performed on construction
439+ Mock::VerifyAndClear(&provider);
440+ // use a mock controller
441+ auto controller = provider.state_controller();
442+
443+ EXPECT_CALL(provider, boot_state()).Times(Exactly(2))
444+ .WillOnce(Return(cul::Provider::BootState::BOOTED))
445+ .WillOnce(Return(cul::Provider::BootState::BOOTING));
446+ EXPECT_CALL(provider, start_heading_updates()).Times(Exactly(1));
447+ EXPECT_CALL(provider, stop_heading_updates()).Times(Exactly(0));
448+
449+ controller->start_heading_updates();
450+ controller->stop_heading_updates();
451+}
452+
453+TEST(Controller, controller_does_not_call_stop_velocity_if_not_booted) {
454+ using namespace ::testing;
455+ using ::testing::Mock;
456+
457+ MockDelayedProvider provider;
458+ EXPECT_CALL(provider, has_delayed_boot()).WillRepeatedly(Return(true));
459+
460+ // ignore what ever was performed on construction
461+ Mock::VerifyAndClear(&provider);
462+ // use a mock controller
463+ auto controller = provider.state_controller();
464+
465+ EXPECT_CALL(provider, boot_state()).Times(Exactly(2))
466+ .WillOnce(Return(cul::Provider::BootState::BOOTED))
467+ .WillOnce(Return(cul::Provider::BootState::BOOTING));
468+ EXPECT_CALL(provider, start_velocity_updates()).Times(Exactly(1));
469+ EXPECT_CALL(provider, stop_velocity_updates()).Times(Exactly(0));
470+
471+ controller->start_velocity_updates();
472+ controller->stop_velocity_updates();
473+}
474
475=== added file 'tests/mock_delayed_provider.h'
476--- tests/mock_delayed_provider.h 1970-01-01 00:00:00 +0000
477+++ tests/mock_delayed_provider.h 2015-10-27 20:49:11 +0000
478@@ -0,0 +1,89 @@
479+/*
480+ * Copyright © 2014 Canonical Ltd.
481+ *
482+ * This program is free software: you can redistribute it and/or modify it
483+ * under the terms of the GNU Lesser General Public License version 3,
484+ * as published by the Free Software Foundation.
485+ *
486+ * This program is distributed in the hope that it will be useful,
487+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
488+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
489+ * GNU Lesser General Public License for more details.
490+ *
491+ * You should have received a copy of the GNU Lesser General Public License
492+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
493+ *
494+ * Authored by: M
495+ */
496+#ifndef MOCK_DELAYED_PROVIDER_H_
497+#define MOCK_DELAYED_PROVIDER_H_
498+
499+#include <com/ubuntu/location/provider.h>
500+#include <com/ubuntu/location/logging.h>
501+
502+#include <gmock/gmock.h>
503+
504+struct PublicController : public com::ubuntu::location::Provider::Controller
505+{
506+
507+ explicit PublicController(com::ubuntu::location::Provider& instance)
508+ : com::ubuntu::location::Provider::Controller(instance) {}
509+};
510+
511+struct MockDelayedProvider : public com::ubuntu::location::Provider
512+{
513+ MockDelayedProvider() : com::ubuntu::location::Provider()
514+ {
515+ }
516+
517+ MOCK_METHOD1(matches_criteria, bool(const com::ubuntu::location::Criteria&));
518+
519+ MOCK_CONST_METHOD1(supports, bool(const com::ubuntu::location::Provider::Features&));
520+ MOCK_CONST_METHOD1(requires, bool(const com::ubuntu::location::Provider::Requirements&));
521+
522+ // Called by the engine whenever the wifi and cell ID reporting state changes.
523+ MOCK_METHOD1(on_wifi_and_cell_reporting_state_changed, void(com::ubuntu::location::WifiAndCellIdReportingState state));
524+
525+ // Called by the engine whenever the reference location changed.
526+ MOCK_METHOD1(on_reference_location_updated, void(const com::ubuntu::location::Update<com::ubuntu::location::Position>& position));
527+
528+ // Called by the engine whenever the reference velocity changed.
529+ MOCK_METHOD1(on_reference_velocity_updated, void(const com::ubuntu::location::Update<com::ubuntu::location::Velocity>& velocity));
530+
531+ // Called by the engine whenever the reference heading changed.
532+ MOCK_METHOD1(on_reference_heading_updated, void(const com::ubuntu::location::Update<com::ubuntu::location::Heading>& heading));
533+
534+ MOCK_METHOD0(start_position_updates, void());
535+ MOCK_METHOD0(stop_position_updates, void());
536+
537+ MOCK_METHOD0(start_heading_updates, void());
538+ MOCK_METHOD0(stop_heading_updates, void());
539+
540+ MOCK_METHOD0(start_velocity_updates, void());
541+ MOCK_METHOD0(stop_velocity_updates, void());
542+
543+ MOCK_CONST_METHOD0(has_delayed_boot, bool());
544+ MOCK_CONST_METHOD0(boot_state, BootState());
545+ MOCK_CONST_METHOD0(boot, void());
546+
547+
548+ // Inject a position update from the outside.
549+ void inject_update(const com::ubuntu::location::Update<com::ubuntu::location::Position>& update)
550+ {
551+ mutable_updates().position(update);
552+ }
553+
554+ // Inject a velocity update from the outside.
555+ void inject_update(const com::ubuntu::location::Update<com::ubuntu::location::Velocity>& update)
556+ {
557+ mutable_updates().velocity(update);
558+ }
559+
560+ // Inject a heading update from the outside.
561+ void inject_update(const com::ubuntu::location::Update<com::ubuntu::location::Heading>& update)
562+ {
563+ mutable_updates().heading(update);
564+ }
565+};
566+
567+#endif // MOCK_PROVIDER_H_

Subscribers

People subscribed via source and target branches