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

Proposed by Scott Sweeny on 2015-10-22
Status: Needs review
Proposed branch: lp:~ssweeny/location-service/espoo-delayed-provider.15.04
Merge into: lp:location-service/15.04
Prerequisite: lp:~ssweeny/location-service/delayed-providers.15.04
Diff against target: 220 lines (+141/-5)
5 files modified
include/location_service/com/ubuntu/location/provider.h (+1/-1)
src/location_service/com/ubuntu/location/provider.cpp (+1/-1)
src/location_service/com/ubuntu/location/providers/remote/provider.cpp (+136/-1)
src/location_service/com/ubuntu/location/providers/remote/provider.h (+2/-1)
tests/mock_delayed_provider.h (+1/-1)
To merge this branch: bzr merge lp:~ssweeny/location-service/espoo-delayed-provider.15.04
Reviewer Review Type Date Requested Status
Thomas Voß (community) 2015-10-22 Needs Fixing on 2015-10-26
Review via email: mp+275424@code.launchpad.net

Commit message

Ensure that we use a delayed provider when the espoo service is not running.

Description of the change

Ensure that the remote provider creates a delayed provider in the case in which the service is not ready. This allows the location service to be started without having the here blobs running.

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

A remark inline about splitting changes between espoo-delayed.provider.15.04 and delayed-provider.15.04.

One other remark: I would prefer a set of more sophisticated test cases that exercise the timing logic. Otherwise, we would have to invest significant manual testing efforts for each landing to ensure correct behavior. Would you mind introducing such tests into this MP?

review: Needs Fixing
200. By Scott Sweeny on 2015-10-27

Merge delayed-provider branch

Scott Sweeny (ssweeny) wrote :

I'm not clear on what tests you're looking for that aren't part of the other MP. Could you elaborate?

Thomas Voß (thomas-voss) wrote :

I was thinking about tests exercising the actual timing, something along the lines of a misbehaving provider that takes a configurable amount of time to come up. It might also be a good idea to test the overall location::Engine behavior with a couple of those providers registered. I hope that helps to clarify my line of thought.

Unmerged revisions

200. By Scott Sweeny on 2015-10-27

Merge delayed-provider branch

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

Ensure that we use a delayed provider when the espoo service is not running.

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-10-27 20:50:44 +0000
3+++ include/location_service/com/ubuntu/location/provider.h 2015-10-27 20:50:44 +0000
4@@ -241,7 +241,7 @@
5 * @brief States the booting state of the provider.
6 * @return The boot state of the provider.
7 */
8- virtual BootState boot_state() const;
9+ virtual BootState boot_state();
10
11 /**
12 * @brief Indicates to the provider that it should start the boot process in order to be able to
13
14=== modified file 'src/location_service/com/ubuntu/location/provider.cpp'
15--- src/location_service/com/ubuntu/location/provider.cpp 2015-10-27 20:50:44 +0000
16+++ src/location_service/com/ubuntu/location/provider.cpp 2015-10-27 20:50:44 +0000
17@@ -199,7 +199,7 @@
18 return false;
19 }
20
21-cul::Provider::BootState cul::Provider::boot_state() const
22+cul::Provider::BootState cul::Provider::boot_state()
23 {
24 return cul::Provider::BootState::BOOTED;
25 }
26
27=== modified file 'src/location_service/com/ubuntu/location/providers/remote/provider.cpp'
28--- src/location_service/com/ubuntu/location/providers/remote/provider.cpp 2015-04-14 18:54:00 +0000
29+++ src/location_service/com/ubuntu/location/providers/remote/provider.cpp 2015-10-27 20:50:44 +0000
30@@ -22,7 +22,9 @@
31
32 #include <com/ubuntu/location/logging.h>
33
34+#include <core/dbus/dbus.h>
35 #include <core/dbus/object.h>
36+#include <core/dbus/service_watcher.h>
37 #include <core/dbus/signal.h>
38 #include <core/dbus/asio/executor.h>
39
40@@ -30,6 +32,8 @@
41
42 #include <boost/asio.hpp>
43
44+#include <functional>
45+#include <mutex>
46 #include <thread>
47
48 namespace cul = com::ubuntu::location;
49@@ -39,6 +43,124 @@
50
51 namespace
52 {
53+// NotYet is a provider decoration that waits for a remote service to come up, before instantiating
54+// the actual stub instance.
55+class NotYet : public cul::Provider
56+{
57+public:
58+ // Creates a new instance, watching fo the given service_name on the given bus, invoking the
59+ // when_ready functor for constructing the provider instance whenever the service becomes ready.
60+ NotYet(const core::dbus::Bus::Ptr& bus, const std::string& service_name, const std::function<cul::Provider::Ptr()>& when_ready)
61+ : dbus{bus}, service_watcher{dbus.make_service_watcher(service_name)},
62+ service_registered
63+ {
64+ service_watcher->service_registered().connect(std::bind(&NotYet::on_service_registered, this, when_ready))
65+ }
66+ {
67+ }
68+
69+ void on_service_registered(const std::function<cul::Provider::Ptr()>& when_ready)
70+ {
71+ LOG(INFO) << "Remote location service appeared.";
72+ impl = when_ready();
73+ set_boot_state(cul::Provider::BootState::BOOTED);
74+ // notify the system we are booted so that we get the current state (started etc..)
75+ LOG(INFO) << "Emitting booted signal.";
76+ booted_signal()(true);
77+ }
78+
79+ bool matches_criteria(const cul::Criteria& criteria) override
80+ {
81+ if (impl) return impl->matches_criteria(criteria);
82+
83+ return false;
84+ }
85+
86+ bool supports(const cul::Provider::Features& f) const override
87+ {
88+ if (impl) return impl->supports(f);
89+ return false;
90+ }
91+
92+ bool requires(const cul::Provider::Requirements& r) const override
93+ {
94+ if (impl) return impl->requires(r);
95+ return true;
96+ }
97+
98+ void on_wifi_and_cell_reporting_state_changed(cul::WifiAndCellIdReportingState state) override
99+ {
100+ if (impl) impl->on_wifi_and_cell_reporting_state_changed(state);
101+ }
102+
103+ void on_reference_location_updated(const cul::Update<cul::Position>& position) override
104+ {
105+ if (impl) impl->on_reference_location_updated(position);
106+ }
107+
108+ void on_reference_velocity_updated(const cul::Update<cul::Velocity>& velocity) override
109+ {
110+ if (impl) impl->on_reference_velocity_updated(velocity);
111+ }
112+
113+ void on_reference_heading_updated(const cul::Update<cul::Heading>& heading) override
114+ {
115+ if (impl) impl->on_reference_heading_updated(heading);
116+ }
117+
118+ void start_position_updates() override
119+ {
120+ if (impl) impl->state_controller()->start_position_updates();
121+ }
122+
123+ void stop_position_updates() override
124+ {
125+ if (impl) impl->state_controller()->stop_position_updates();
126+ }
127+
128+ void start_heading_updates() override
129+ {
130+ if (impl) impl->state_controller()->start_heading_updates();
131+ }
132+
133+ void stop_heading_updates() override
134+ {
135+ if (impl) impl->state_controller()->stop_heading_updates();
136+ }
137+
138+ void start_velocity_updates() override
139+ {
140+ if (impl) impl->state_controller()->start_velocity_updates();
141+ }
142+
143+ void stop_velocity_updates() override
144+ {
145+ if (impl) impl->state_controller()->stop_velocity_updates();
146+ }
147+
148+ BootState boot_state()
149+ {
150+ std::lock_guard<std::mutex> guard(state_mutex);
151+ return state;
152+ }
153+
154+ void set_boot_state(BootState new_state)
155+ {
156+ LOG(INFO) << "Setting boot state.";
157+ std::lock_guard<std::mutex> guard(state_mutex);
158+ state = new_state;
159+ LOG(INFO) << "State was set.";
160+ }
161+
162+private:
163+ core::dbus::DBus dbus;
164+ std::unique_ptr<core::dbus::ServiceWatcher> service_watcher;
165+ core::ScopedConnection service_registered;
166+ cul::Provider::Ptr impl;
167+ cul::Provider::BootState state = cul::Provider::BootState::BOOTING;
168+ std::mutex state_mutex;
169+};
170+
171 struct Runtime
172 {
173 static Runtime& instance()
174@@ -191,7 +313,20 @@
175 auto service = dbus::Service::use_service(bus, name);
176 auto object = service->object_for_path(path);
177
178- return create_instance_with_config(remote::stub::Configuration{object});
179+ try
180+ {
181+ auto result = create_instance_with_config(remote::stub::Configuration{object});
182+ } catch (const std::runtime_error& e)
183+ {
184+ std::cerr << "Error creating remote provider, creating facade for watching the service and delayed initialization" << std::endl;
185+ std::cerr << e.what() << std::endl;
186+ }
187+
188+ return std::make_shared<NotYet>(bus, name, [bus, name, path]()
189+ {
190+ auto object = dbus::Service::use_service(bus, name)->object_for_path(path);
191+ return create_instance_with_config(remote::stub::Configuration{object});
192+ });
193 }
194
195 cul::Provider::Ptr remote::Provider::Stub::create_instance_with_config(const remote::stub::Configuration& config)
196
197=== modified file 'src/location_service/com/ubuntu/location/providers/remote/provider.h'
198--- src/location_service/com/ubuntu/location/providers/remote/provider.h 2015-01-26 19:11:46 +0000
199+++ src/location_service/com/ubuntu/location/providers/remote/provider.h 2015-10-27 20:50:44 +0000
200@@ -40,7 +40,8 @@
201 {
202 class Stub : public com::ubuntu::location::Provider, public std::enable_shared_from_this<Stub>
203 {
204- public:
205+ public:
206+
207 // For integration with the Provider factory.
208 static std::string class_name();
209
210
211=== modified file 'tests/mock_delayed_provider.h'
212--- tests/mock_delayed_provider.h 2015-10-27 20:50:44 +0000
213+++ tests/mock_delayed_provider.h 2015-10-27 20:50:44 +0000
214@@ -63,7 +63,7 @@
215 MOCK_METHOD0(stop_velocity_updates, void());
216
217 MOCK_CONST_METHOD0(has_delayed_boot, bool());
218- MOCK_CONST_METHOD0(boot_state, BootState());
219+ MOCK_METHOD0(boot_state, BootState());
220 MOCK_CONST_METHOD0(boot, void());
221
222

Subscribers

People subscribed via source and target branches