Merge lp:~mandel/location-service/better-position-selection into lp:location-service/trunk
- better-position-selection
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Jim Hodapp |
Approved revision: | 202 |
Merged at revision: | 187 |
Proposed branch: | lp:~mandel/location-service/better-position-selection |
Merge into: | lp:location-service/trunk |
Prerequisite: | lp:~mandel/location-service/remove-pimpl |
Diff against target: |
494 lines (+407/-2) 8 files modified
src/location_service/com/ubuntu/location/CMakeLists.txt (+1/-0) src/location_service/com/ubuntu/location/engine.cpp (+5/-2) src/location_service/com/ubuntu/location/engine.h (+3/-0) src/location_service/com/ubuntu/location/time_based_update_policy.cpp (+121/-0) src/location_service/com/ubuntu/location/time_based_update_policy.h (+70/-0) src/location_service/com/ubuntu/location/update_policy.h (+75/-0) tests/CMakeLists.txt (+1/-0) tests/time_based_update_policy_test.cpp (+131/-0) |
To merge this branch: | bzr merge lp:~mandel/location-service/better-position-selection |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jim Hodapp (community) | code | Approve | |
PS Jenkins bot | continuous-integration | Approve | |
Thomas Voß (community) | Needs Fixing | ||
Review via email: mp+257037@code.launchpad.net |
Commit message
Improve the selection of the bag of providers to ensure that the locations used are within a reasonable time margin.
Description of the change
Improve the selection of the bag of providers to ensure that the locations used are within a reasonable time margin.
Thomas Voß (thomas-voss) wrote : | # |
One other bit: The predominant style in the codebase is snake_style. Please change the camelCase instances accordingly.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:193
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
- 194. By Manuel de la Peña
-
Merged remove-pimpl into better-
position- selection.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:194
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
- 195. By Manuel de la Peña
-
Move the heuristics to its own class to make it easier to have diff options. Move the position selection to the engine.
- 196. By Manuel de la Peña
-
Add missing files.
Thomas Voß (thomas-voss) wrote : | # |
Looks better, but some comments inline.
- 197. By Manuel de la Peña
-
Made changes following the comments from the reviews.
- 198. By Manuel de la Peña
-
Add missing space.
- 199. By Manuel de la Peña
-
Merged remove-pimpl into better-
position- selection. - 200. By Manuel de la Peña
-
Remove unused var.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:196
http://
Executed test runs:
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:200
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Jim Hodapp (jhodapp) wrote : | # |
Some suggestions inline below.
- 201. By Manuel de la Peña
-
Made changes following the code reviews.
- 202. By Manuel de la Peña
-
Set var to const.
Preview Diff
1 | === modified file 'src/location_service/com/ubuntu/location/CMakeLists.txt' | |||
2 | --- src/location_service/com/ubuntu/location/CMakeLists.txt 2015-01-25 12:45:30 +0000 | |||
3 | +++ src/location_service/com/ubuntu/location/CMakeLists.txt 2015-04-23 21:30:38 +0000 | |||
4 | @@ -27,6 +27,7 @@ | |||
5 | 27 | proxy_provider.cpp | 27 | proxy_provider.cpp |
6 | 28 | satellite_based_positioning_state.cpp | 28 | satellite_based_positioning_state.cpp |
7 | 29 | settings.cpp | 29 | settings.cpp |
8 | 30 | time_based_update_policy.cpp | ||
9 | 30 | set_name_for_thread.cpp | 31 | set_name_for_thread.cpp |
10 | 31 | wifi_and_cell_reporting_state.cpp | 32 | wifi_and_cell_reporting_state.cpp |
11 | 32 | 33 | ||
12 | 33 | 34 | ||
13 | === modified file 'src/location_service/com/ubuntu/location/engine.cpp' | |||
14 | --- src/location_service/com/ubuntu/location/engine.cpp 2015-01-25 12:44:20 +0000 | |||
15 | +++ src/location_service/com/ubuntu/location/engine.cpp 2015-04-23 21:30:38 +0000 | |||
16 | @@ -24,6 +24,8 @@ | |||
17 | 24 | #include <stdexcept> | 24 | #include <stdexcept> |
18 | 25 | #include <unordered_map> | 25 | #include <unordered_map> |
19 | 26 | 26 | ||
20 | 27 | #include "time_based_update_policy.h" | ||
21 | 28 | |||
22 | 27 | namespace cul = com::ubuntu::location; | 29 | namespace cul = com::ubuntu::location; |
23 | 28 | 30 | ||
24 | 29 | const cul::SatelliteBasedPositioningState cul::Engine::Configuration::Defaults::satellite_based_positioning_state; | 31 | const cul::SatelliteBasedPositioningState cul::Engine::Configuration::Defaults::satellite_based_positioning_state; |
25 | @@ -33,7 +35,8 @@ | |||
26 | 33 | cul::Engine::Engine(const cul::ProviderSelectionPolicy::Ptr& provider_selection_policy, | 35 | cul::Engine::Engine(const cul::ProviderSelectionPolicy::Ptr& provider_selection_policy, |
27 | 34 | const cul::Settings::Ptr& settings) | 36 | const cul::Settings::Ptr& settings) |
28 | 35 | : provider_selection_policy(provider_selection_policy), | 37 | : provider_selection_policy(provider_selection_policy), |
30 | 36 | settings(settings) | 38 | settings(settings), |
31 | 39 | update_policy(std::make_shared<cul::TimeBasedUpdatePolicy>()) | ||
32 | 37 | { | 40 | { |
33 | 38 | if (!provider_selection_policy) throw std::runtime_error | 41 | if (!provider_selection_policy) throw std::runtime_error |
34 | 39 | { | 42 | { |
35 | @@ -204,7 +207,7 @@ | |||
36 | 204 | // We should come up with a better heuristic here. | 207 | // We should come up with a better heuristic here. |
37 | 205 | auto cpr = provider->updates().position.connect([this](const cul::Update<cul::Position>& src) | 208 | auto cpr = provider->updates().position.connect([this](const cul::Update<cul::Position>& src) |
38 | 206 | { | 209 | { |
40 | 207 | updates.reference_location = src; | 210 | updates.reference_location = update_policy->verify_update(src); |
41 | 208 | }); | 211 | }); |
42 | 209 | 212 | ||
43 | 210 | std::lock_guard<std::mutex> lg(guard); | 213 | std::lock_guard<std::mutex> lg(guard); |
44 | 211 | 214 | ||
45 | === modified file 'src/location_service/com/ubuntu/location/engine.h' | |||
46 | --- src/location_service/com/ubuntu/location/engine.h 2015-01-14 13:41:54 +0000 | |||
47 | +++ src/location_service/com/ubuntu/location/engine.h 2015-04-23 21:30:38 +0000 | |||
48 | @@ -33,6 +33,8 @@ | |||
49 | 33 | #include <mutex> | 33 | #include <mutex> |
50 | 34 | #include <set> | 34 | #include <set> |
51 | 35 | 35 | ||
52 | 36 | #include "update_policy.h" | ||
53 | 37 | |||
54 | 36 | namespace com | 38 | namespace com |
55 | 37 | { | 39 | { |
56 | 38 | namespace ubuntu | 40 | namespace ubuntu |
57 | @@ -193,6 +195,7 @@ | |||
58 | 193 | std::map<Provider::Ptr, ProviderConnections> providers; | 195 | std::map<Provider::Ptr, ProviderConnections> providers; |
59 | 194 | ProviderSelectionPolicy::Ptr provider_selection_policy; | 196 | ProviderSelectionPolicy::Ptr provider_selection_policy; |
60 | 195 | Settings::Ptr settings; | 197 | Settings::Ptr settings; |
61 | 198 | UpdatePolicy::Ptr update_policy; | ||
62 | 196 | }; | 199 | }; |
63 | 197 | 200 | ||
64 | 198 | /** @brief Pretty prints the given status to the given stream. */ | 201 | /** @brief Pretty prints the given status to the given stream. */ |
65 | 199 | 202 | ||
66 | === added file 'src/location_service/com/ubuntu/location/time_based_update_policy.cpp' | |||
67 | --- src/location_service/com/ubuntu/location/time_based_update_policy.cpp 1970-01-01 00:00:00 +0000 | |||
68 | +++ src/location_service/com/ubuntu/location/time_based_update_policy.cpp 2015-04-23 21:30:38 +0000 | |||
69 | @@ -0,0 +1,121 @@ | |||
70 | 1 | /* | ||
71 | 2 | * Copyright © 2015 Canonical Ltd. | ||
72 | 3 | * | ||
73 | 4 | * This program is free software: you can redistribute it and/or modify it | ||
74 | 5 | * under the terms of the GNU Lesser General Public License version 3, | ||
75 | 6 | * as published by the Free Software Foundation. | ||
76 | 7 | * | ||
77 | 8 | * This program is distributed in the hope that it will be useful, | ||
78 | 9 | * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
79 | 10 | * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
80 | 11 | * GNU Lesser General Public License for more details. | ||
81 | 12 | * | ||
82 | 13 | * You should have received a copy of the GNU Lesser General Public License | ||
83 | 14 | * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
84 | 15 | * | ||
85 | 16 | * Authored by: Manuel de la Pena <manuel.delapena@canonical.com> | ||
86 | 17 | */ | ||
87 | 18 | |||
88 | 19 | #include "time_based_update_policy.h" | ||
89 | 20 | |||
90 | 21 | namespace com | ||
91 | 22 | { | ||
92 | 23 | namespace ubuntu | ||
93 | 24 | { | ||
94 | 25 | namespace location | ||
95 | 26 | { | ||
96 | 27 | |||
97 | 28 | std::chrono::minutes TimeBasedUpdatePolicy::default_timeout() | ||
98 | 29 | { | ||
99 | 30 | static const std::chrono::minutes default_limit(2); | ||
100 | 31 | return default_limit; | ||
101 | 32 | } | ||
102 | 33 | |||
103 | 34 | TimeBasedUpdatePolicy::TimeBasedUpdatePolicy(std::chrono::minutes mins) | ||
104 | 35 | : limit(mins) | ||
105 | 36 | { | ||
106 | 37 | |||
107 | 38 | } | ||
108 | 39 | |||
109 | 40 | const location::Update<location::Position>& TimeBasedUpdatePolicy::verify_update(const location::Update<location::Position>& update) | ||
110 | 41 | { | ||
111 | 42 | std::lock_guard<std::mutex> guard(position_update_mutex); | ||
112 | 43 | bool use_new_update; | ||
113 | 44 | if (is_significantly_newer(last_position_update, update, limit)) | ||
114 | 45 | { | ||
115 | 46 | use_new_update = true; | ||
116 | 47 | } | ||
117 | 48 | else if (is_significantly_older(last_position_update, update, limit)) | ||
118 | 49 | { | ||
119 | 50 | use_new_update = false; | ||
120 | 51 | } | ||
121 | 52 | else | ||
122 | 53 | { | ||
123 | 54 | // if the update has happened within a reasonable amount of time we will just use it if it is more accurate | ||
124 | 55 | // that the previous one. | ||
125 | 56 | use_new_update = last_position_update.value.accuracy.horizontal && update.value.accuracy.horizontal | ||
126 | 57 | && *last_position_update.value.accuracy.horizontal >= *update.value.accuracy.horizontal; | ||
127 | 58 | } | ||
128 | 59 | |||
129 | 60 | if (use_new_update) | ||
130 | 61 | { | ||
131 | 62 | last_position_update = update; | ||
132 | 63 | return update; | ||
133 | 64 | } | ||
134 | 65 | else | ||
135 | 66 | { | ||
136 | 67 | return last_position_update; | ||
137 | 68 | } | ||
138 | 69 | } | ||
139 | 70 | |||
140 | 71 | |||
141 | 72 | const location::Update<location::Heading>& TimeBasedUpdatePolicy::verify_update(const location::Update<location::Heading>& update) | ||
142 | 73 | { | ||
143 | 74 | std::lock_guard<std::mutex> guard(heading_update_mutex); | ||
144 | 75 | bool use_new_update; | ||
145 | 76 | if (is_significantly_newer(last_heading_update, update, limit)) | ||
146 | 77 | { | ||
147 | 78 | use_new_update = true; | ||
148 | 79 | } | ||
149 | 80 | else if (is_significantly_older(last_heading_update, update, limit)) | ||
150 | 81 | { | ||
151 | 82 | use_new_update = false; | ||
152 | 83 | } | ||
153 | 84 | if (use_new_update) | ||
154 | 85 | { | ||
155 | 86 | last_heading_update = update; | ||
156 | 87 | return update; | ||
157 | 88 | } | ||
158 | 89 | else | ||
159 | 90 | { | ||
160 | 91 | return last_heading_update; | ||
161 | 92 | } | ||
162 | 93 | } | ||
163 | 94 | |||
164 | 95 | const location::Update<location::Velocity>& TimeBasedUpdatePolicy::verify_update(const location::Update<location::Velocity>& update) | ||
165 | 96 | { | ||
166 | 97 | std::lock_guard<std::mutex> guard(velocity_update_mutex); | ||
167 | 98 | bool use_new_update; | ||
168 | 99 | if (is_significantly_newer(last_velocity_update, update, limit)) | ||
169 | 100 | { | ||
170 | 101 | use_new_update = true; | ||
171 | 102 | } | ||
172 | 103 | else if (is_significantly_older(last_velocity_update, update, limit)) | ||
173 | 104 | { | ||
174 | 105 | use_new_update = false; | ||
175 | 106 | } | ||
176 | 107 | |||
177 | 108 | if (use_new_update) | ||
178 | 109 | { | ||
179 | 110 | last_velocity_update = update; | ||
180 | 111 | return update; | ||
181 | 112 | } | ||
182 | 113 | else | ||
183 | 114 | { | ||
184 | 115 | return last_velocity_update; | ||
185 | 116 | } | ||
186 | 117 | } | ||
187 | 118 | |||
188 | 119 | } | ||
189 | 120 | } | ||
190 | 121 | } | ||
191 | 0 | \ No newline at end of file | 122 | \ No newline at end of file |
192 | 1 | 123 | ||
193 | === added file 'src/location_service/com/ubuntu/location/time_based_update_policy.h' | |||
194 | --- src/location_service/com/ubuntu/location/time_based_update_policy.h 1970-01-01 00:00:00 +0000 | |||
195 | +++ src/location_service/com/ubuntu/location/time_based_update_policy.h 2015-04-23 21:30:38 +0000 | |||
196 | @@ -0,0 +1,70 @@ | |||
197 | 1 | /* | ||
198 | 2 | * Copyright © 2015 Canonical Ltd. | ||
199 | 3 | * | ||
200 | 4 | * This program is free software: you can redistribute it and/or modify it | ||
201 | 5 | * under the terms of the GNU Lesser General Public License version 3, | ||
202 | 6 | * as published by the Free Software Foundation. | ||
203 | 7 | * | ||
204 | 8 | * This program is distributed in the hope that it will be useful, | ||
205 | 9 | * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
206 | 10 | * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
207 | 11 | * GNU Lesser General Public License for more details. | ||
208 | 12 | * | ||
209 | 13 | * You should have received a copy of the GNU Lesser General Public License | ||
210 | 14 | * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
211 | 15 | * | ||
212 | 16 | * Authored by: Manuel de la Pena <manuel.delapena@canonical.com> | ||
213 | 17 | */ | ||
214 | 18 | #ifndef LOCATION_SERVICE_UBUNTU_LOCATION_SERVICE_TIMES_BASED_UPDATE_POLICY_H_ | ||
215 | 19 | #define LOCATION_SERVICE_UBUNTU_LOCATION_SERVICE_TIMES_BASED_UPDATE_POLICY_H_ | ||
216 | 20 | |||
217 | 21 | #include <chrono> | ||
218 | 22 | #include <mutex> | ||
219 | 23 | |||
220 | 24 | #include "update_policy.h" | ||
221 | 25 | |||
222 | 26 | namespace com | ||
223 | 27 | { | ||
224 | 28 | namespace ubuntu | ||
225 | 29 | { | ||
226 | 30 | namespace location | ||
227 | 31 | { | ||
228 | 32 | |||
229 | 33 | // An interface that can be implemented to add heuristics on how heading, position or velocity updates will be chosen. | ||
230 | 34 | // This class ensures that the best update possible is chosen within a reasonable time bracket. | ||
231 | 35 | class TimeBasedUpdatePolicy : public UpdatePolicy { | ||
232 | 36 | |||
233 | 37 | public: | ||
234 | 38 | TimeBasedUpdatePolicy(std::chrono::minutes mins=default_timeout()); | ||
235 | 39 | TimeBasedUpdatePolicy(const TimeBasedUpdatePolicy&) = delete; | ||
236 | 40 | ~TimeBasedUpdatePolicy() = default; | ||
237 | 41 | |||
238 | 42 | // Return if the given position update will be verified as the new position in the engine. | ||
239 | 43 | const location::Update<location::Position>& verify_update(const location::Update<location::Position>& update) override; | ||
240 | 44 | // Return if the given heading update will be verified as the new heading in the engine. | ||
241 | 45 | const location::Update<location::Heading>& verify_update(const location::Update<location::Heading>& update) override; | ||
242 | 46 | // Return if the given velocity update will be verified as the new velocity in the engine. | ||
243 | 47 | const location::Update<location::Velocity>& verify_update(const location::Update<location::Velocity>& update) override; | ||
244 | 48 | |||
245 | 49 | static std::chrono::minutes default_timeout(); | ||
246 | 50 | |||
247 | 51 | protected: | ||
248 | 52 | // not private to simplify the testing but should be private | ||
249 | 53 | location::Update<location::Position> last_position_update; | ||
250 | 54 | location::Update<location::Heading> last_heading_update; | ||
251 | 55 | location::Update<location::Velocity> last_velocity_update; | ||
252 | 56 | |||
253 | 57 | private: | ||
254 | 58 | // callbacks can happen in diff threads, make sure multi-threading will work in this class | ||
255 | 59 | std::mutex position_update_mutex; | ||
256 | 60 | std::mutex heading_update_mutex; | ||
257 | 61 | std::mutex velocity_update_mutex; | ||
258 | 62 | // used to calculate the time accepted bracket | ||
259 | 63 | std::chrono::minutes limit; | ||
260 | 64 | }; | ||
261 | 65 | |||
262 | 66 | } | ||
263 | 67 | } | ||
264 | 68 | } | ||
265 | 69 | |||
266 | 70 | #endif //LOCATION_SERVICE_UBUNTU_LOCATION_SERVICE_TIMES_BASED_UPDATE_POLICY_H_ | ||
267 | 0 | 71 | ||
268 | === added file 'src/location_service/com/ubuntu/location/update_policy.h' | |||
269 | --- src/location_service/com/ubuntu/location/update_policy.h 1970-01-01 00:00:00 +0000 | |||
270 | +++ src/location_service/com/ubuntu/location/update_policy.h 2015-04-23 21:30:38 +0000 | |||
271 | @@ -0,0 +1,75 @@ | |||
272 | 1 | /* | ||
273 | 2 | * Copyright © 2015 Canonical Ltd. | ||
274 | 3 | * | ||
275 | 4 | * This program is free software: you can redistribute it and/or modify it | ||
276 | 5 | * under the terms of the GNU Lesser General Public License version 3, | ||
277 | 6 | * as published by the Free Software Foundation. | ||
278 | 7 | * | ||
279 | 8 | * This program is distributed in the hope that it will be useful, | ||
280 | 9 | * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
281 | 10 | * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
282 | 11 | * GNU Lesser General Public License for more details. | ||
283 | 12 | * | ||
284 | 13 | * You should have received a copy of the GNU Lesser General Public License | ||
285 | 14 | * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
286 | 15 | * | ||
287 | 16 | * Authored by: Manuel de la Pena <manuel.delapena@canonical.com> | ||
288 | 17 | */ | ||
289 | 18 | #ifndef LOCATION_SERVICE_UBUNTU_LOCATION_SERVICE_UPDATE_POLICY_H_ | ||
290 | 19 | #define LOCATION_SERVICE_UBUNTU_LOCATION_SERVICE_UPDATE_POLICY_H_ | ||
291 | 20 | |||
292 | 21 | #include <memory> | ||
293 | 22 | |||
294 | 23 | #include <com/ubuntu/location/heading.h> | ||
295 | 24 | #include <com/ubuntu/location/position.h> | ||
296 | 25 | #include <com/ubuntu/location/update.h> | ||
297 | 26 | #include <com/ubuntu/location/velocity.h> | ||
298 | 27 | |||
299 | 28 | namespace com | ||
300 | 29 | { | ||
301 | 30 | namespace ubuntu | ||
302 | 31 | { | ||
303 | 32 | namespace location | ||
304 | 33 | { | ||
305 | 34 | |||
306 | 35 | // An interface that can be implemented to add heuristics on how heading, position or velocity updateswill be chosen. | ||
307 | 36 | // This class allows developers to inject different heuristics in the engine to perform the update selection | ||
308 | 37 | // so that the app developers can take advantage of it. | ||
309 | 38 | class UpdatePolicy { | ||
310 | 39 | public: | ||
311 | 40 | typedef std::shared_ptr<UpdatePolicy> Ptr; | ||
312 | 41 | |||
313 | 42 | UpdatePolicy(const UpdatePolicy&) = delete; | ||
314 | 43 | UpdatePolicy(UpdatePolicy&&) = delete; | ||
315 | 44 | UpdatePolicy& operator=(const UpdatePolicy&) = delete; | ||
316 | 45 | virtual ~UpdatePolicy() = default; | ||
317 | 46 | |||
318 | 47 | // Return if the given position update will be verified as the new position in the engine. | ||
319 | 48 | virtual const location::Update<location::Position>& verify_update(const location::Update<location::Position>& update) = 0; | ||
320 | 49 | // Return if the given heading update will be verified as the new heading in the engine. | ||
321 | 50 | virtual const location::Update<location::Heading>& verify_update(const location::Update<location::Heading>& update) = 0; | ||
322 | 51 | // Return if the given velocity update will be verified as the new velocity in the engine. | ||
323 | 52 | virtual const location::Update<location::Velocity>& verify_update(const location::Update<location::Velocity>& update) = 0; | ||
324 | 53 | protected: | ||
325 | 54 | UpdatePolicy() = default; | ||
326 | 55 | |||
327 | 56 | template <class T> bool is_significantly_newer(const location::Update<T> last_update, const location::Update<T> update, std::chrono::minutes limit) const | ||
328 | 57 | { | ||
329 | 58 | auto delta = update.when - last_update.when; | ||
330 | 59 | return delta > limit; | ||
331 | 60 | } | ||
332 | 61 | |||
333 | 62 | template <class T> bool is_significantly_older(const location::Update<T> last_update, const location::Update<T> update, std::chrono::minutes limit) const | ||
334 | 63 | { | ||
335 | 64 | auto delta = update.when - last_update.when; | ||
336 | 65 | return delta < (-1 * limit); | ||
337 | 66 | } | ||
338 | 67 | |||
339 | 68 | |||
340 | 69 | }; | ||
341 | 70 | } | ||
342 | 71 | } | ||
343 | 72 | } | ||
344 | 73 | |||
345 | 74 | #endif // LOCATION_SERVICE_UBUNTU_LOCATION_SERVICE_UPDATE_POLICY_H_ | ||
346 | 75 | |||
347 | 0 | 76 | ||
348 | === modified file 'tests/CMakeLists.txt' | |||
349 | --- tests/CMakeLists.txt 2014-11-14 11:26:45 +0000 | |||
350 | +++ tests/CMakeLists.txt 2015-04-23 21:30:38 +0000 | |||
351 | @@ -80,6 +80,7 @@ | |||
352 | 80 | LOCATION_SERVICE_ADD_TEST(engine_test engine_test.cpp) | 80 | LOCATION_SERVICE_ADD_TEST(engine_test engine_test.cpp) |
353 | 81 | LOCATION_SERVICE_ADD_TEST(harvester_test harvester_test.cpp) | 81 | LOCATION_SERVICE_ADD_TEST(harvester_test harvester_test.cpp) |
354 | 82 | LOCATION_SERVICE_ADD_TEST(demultiplexing_reporter_test demultiplexing_reporter_test.cpp) | 82 | LOCATION_SERVICE_ADD_TEST(demultiplexing_reporter_test demultiplexing_reporter_test.cpp) |
355 | 83 | LOCATION_SERVICE_ADD_TEST(time_based_update_policy_test time_based_update_policy_test.cpp) | ||
356 | 83 | 84 | ||
357 | 84 | if (NET_CPP_FOUND) | 85 | if (NET_CPP_FOUND) |
358 | 85 | LOCATION_SERVICE_ADD_TEST(ichnaea_reporter_test ichnaea_reporter_test.cpp) | 86 | LOCATION_SERVICE_ADD_TEST(ichnaea_reporter_test ichnaea_reporter_test.cpp) |
359 | 86 | 87 | ||
360 | === added file 'tests/time_based_update_policy_test.cpp' | |||
361 | --- tests/time_based_update_policy_test.cpp 1970-01-01 00:00:00 +0000 | |||
362 | +++ tests/time_based_update_policy_test.cpp 2015-04-23 21:30:38 +0000 | |||
363 | @@ -0,0 +1,131 @@ | |||
364 | 1 | /* | ||
365 | 2 | * Copyright © 2015 Canonical Ltd. | ||
366 | 3 | * | ||
367 | 4 | * This program is free software: you can redistribute it and/or modify it | ||
368 | 5 | * under the terms of the GNU Lesser General Public License version 3, | ||
369 | 6 | * as published by the Free Software Foundation. | ||
370 | 7 | * | ||
371 | 8 | * This program is distributed in the hope that it will be useful, | ||
372 | 9 | * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
373 | 10 | * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
374 | 11 | * GNU Lesser General Public License for more details. | ||
375 | 12 | * | ||
376 | 13 | * You should have received a copy of the GNU Lesser General Public License | ||
377 | 14 | * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
378 | 15 | * | ||
379 | 16 | * Authored by: Manuel de la Pena <manuel.delapena@canonical.com> | ||
380 | 17 | */ | ||
381 | 18 | #include <com/ubuntu/location/time_based_update_policy.h> | ||
382 | 19 | |||
383 | 20 | #include <gtest/gtest.h> | ||
384 | 21 | |||
385 | 22 | using namespace ::testing; | ||
386 | 23 | namespace cul = com::ubuntu::location; | ||
387 | 24 | |||
388 | 25 | namespace | ||
389 | 26 | { | ||
390 | 27 | auto timestamp = com::ubuntu::location::Clock::now(); | ||
391 | 28 | |||
392 | 29 | com::ubuntu::location::Update<com::ubuntu::location::Position> reference_position_update | ||
393 | 30 | { | ||
394 | 31 | { | ||
395 | 32 | com::ubuntu::location::wgs84::Latitude{9. * com::ubuntu::location::units::Degrees}, | ||
396 | 33 | com::ubuntu::location::wgs84::Longitude{53. * com::ubuntu::location::units::Degrees}, | ||
397 | 34 | com::ubuntu::location::wgs84::Altitude{-2. * com::ubuntu::location::units::Meters}, | ||
398 | 35 | }, | ||
399 | 36 | timestamp | ||
400 | 37 | }; | ||
401 | 38 | } | ||
402 | 39 | |||
403 | 40 | // make certain internal details public so that we can set the last update | ||
404 | 41 | class PublicTimeBasedUpdatePolicy : public cul::TimeBasedUpdatePolicy { | ||
405 | 42 | public: | ||
406 | 43 | PublicTimeBasedUpdatePolicy(std::chrono::minutes mins) : cul::TimeBasedUpdatePolicy(mins) {} | ||
407 | 44 | using cul::TimeBasedUpdatePolicy::last_position_update; | ||
408 | 45 | using cul::TimeBasedUpdatePolicy::last_heading_update; | ||
409 | 46 | using cul::TimeBasedUpdatePolicy::last_velocity_update; | ||
410 | 47 | }; | ||
411 | 48 | |||
412 | 49 | TEST(TimeBasedUpdatePolicy, policy_ignores_updates_that_are_too_old) | ||
413 | 50 | { | ||
414 | 51 | auto policy = std::make_shared<PublicTimeBasedUpdatePolicy>(std::chrono::minutes(2)); | ||
415 | 52 | policy->last_position_update = reference_position_update; | ||
416 | 53 | |||
417 | 54 | com::ubuntu::location::Update<com::ubuntu::location::Position> old_update | ||
418 | 55 | { | ||
419 | 56 | { | ||
420 | 57 | com::ubuntu::location::wgs84::Latitude{10. * com::ubuntu::location::units::Degrees}, | ||
421 | 58 | com::ubuntu::location::wgs84::Longitude{60. * com::ubuntu::location::units::Degrees}, | ||
422 | 59 | com::ubuntu::location::wgs84::Altitude{10. * com::ubuntu::location::units::Meters} | ||
423 | 60 | }, | ||
424 | 61 | timestamp - std::chrono::minutes(5) | ||
425 | 62 | }; | ||
426 | 63 | policy->verify_update(old_update); | ||
427 | 64 | |||
428 | 65 | ASSERT_NE(policy->last_position_update.value.latitude, old_update.value.latitude); | ||
429 | 66 | ASSERT_EQ(policy->last_position_update.value.latitude, reference_position_update.value.latitude); | ||
430 | 67 | |||
431 | 68 | ASSERT_NE(policy->last_position_update.value.longitude, old_update.value.longitude); | ||
432 | 69 | ASSERT_EQ(policy->last_position_update.value.longitude, reference_position_update.value.longitude); | ||
433 | 70 | |||
434 | 71 | ASSERT_NE(policy->last_position_update.value.altitude, old_update.value.altitude); | ||
435 | 72 | ASSERT_EQ(policy->last_position_update.value.altitude, reference_position_update.value.altitude); | ||
436 | 73 | } | ||
437 | 74 | |||
438 | 75 | TEST(TimeBasedUpdatePolicy, policy_uses_very_recent_updates) | ||
439 | 76 | { | ||
440 | 77 | auto policy = std::make_shared<PublicTimeBasedUpdatePolicy>(std::chrono::minutes(2)); | ||
441 | 78 | |||
442 | 79 | policy->last_position_update = reference_position_update; | ||
443 | 80 | |||
444 | 81 | com::ubuntu::location::Update<com::ubuntu::location::Position> new_update | ||
445 | 82 | { | ||
446 | 83 | { | ||
447 | 84 | com::ubuntu::location::wgs84::Latitude{10. * com::ubuntu::location::units::Degrees}, | ||
448 | 85 | com::ubuntu::location::wgs84::Longitude{60. * com::ubuntu::location::units::Degrees}, | ||
449 | 86 | com::ubuntu::location::wgs84::Altitude{10. * com::ubuntu::location::units::Meters} | ||
450 | 87 | }, | ||
451 | 88 | timestamp + std::chrono::minutes(3) | ||
452 | 89 | }; | ||
453 | 90 | |||
454 | 91 | policy->verify_update(new_update); | ||
455 | 92 | |||
456 | 93 | ASSERT_EQ(policy->last_position_update.value.latitude, new_update.value.latitude); | ||
457 | 94 | ASSERT_NE(policy->last_position_update.value.latitude, reference_position_update.value.latitude); | ||
458 | 95 | |||
459 | 96 | ASSERT_EQ(policy->last_position_update.value.longitude, new_update.value.longitude); | ||
460 | 97 | ASSERT_NE(policy->last_position_update.value.longitude, reference_position_update.value.longitude); | ||
461 | 98 | |||
462 | 99 | ASSERT_EQ(policy->last_position_update.value.altitude, new_update.value.altitude); | ||
463 | 100 | ASSERT_NE(policy->last_position_update.value.altitude, reference_position_update.value.altitude); | ||
464 | 101 | } | ||
465 | 102 | |||
466 | 103 | TEST(TimeBasedUpdatePolicy, policy_ignores_inaccurate_updates) | ||
467 | 104 | { | ||
468 | 105 | auto policy = std::make_shared<PublicTimeBasedUpdatePolicy>(std::chrono::minutes(2)); | ||
469 | 106 | reference_position_update.value.accuracy.horizontal = 1. * com::ubuntu::location::units::Meters; | ||
470 | 107 | policy->last_position_update = reference_position_update; | ||
471 | 108 | |||
472 | 109 | com::ubuntu::location::Update<com::ubuntu::location::Position> new_update | ||
473 | 110 | { | ||
474 | 111 | { | ||
475 | 112 | com::ubuntu::location::wgs84::Latitude{10. * com::ubuntu::location::units::Degrees}, | ||
476 | 113 | com::ubuntu::location::wgs84::Longitude{60. * com::ubuntu::location::units::Degrees}, | ||
477 | 114 | com::ubuntu::location::wgs84::Altitude{10. * com::ubuntu::location::units::Meters}, | ||
478 | 115 | }, | ||
479 | 116 | timestamp + std::chrono::minutes(1) | ||
480 | 117 | }; | ||
481 | 118 | new_update.value.accuracy.horizontal = 8. * com::ubuntu::location::units::Meters; | ||
482 | 119 | |||
483 | 120 | policy->verify_update(new_update); | ||
484 | 121 | ASSERT_TRUE(*new_update.value.accuracy.horizontal > *reference_position_update.value.accuracy.horizontal); | ||
485 | 122 | |||
486 | 123 | ASSERT_NE(policy->last_position_update.value.latitude, new_update.value.latitude); | ||
487 | 124 | ASSERT_EQ(policy->last_position_update.value.latitude, reference_position_update.value.latitude); | ||
488 | 125 | |||
489 | 126 | ASSERT_NE(policy->last_position_update.value.longitude, new_update.value.longitude); | ||
490 | 127 | ASSERT_EQ(policy->last_position_update.value.longitude, reference_position_update.value.longitude); | ||
491 | 128 | |||
492 | 129 | ASSERT_NE(policy->last_position_update.value.altitude, new_update.value.altitude); | ||
493 | 130 | ASSERT_EQ(policy->last_position_update.value.altitude, reference_position_update.value.altitude); | ||
494 | 131 | } |
Thanks for the changes, a high-level remark first: The ProviderSelecti onStrategy implementation is not the right place to add this functionality. Instead, the functionality should be added here:
http:// bazaar. launchpad. net/~phablet- team/location- service/ trunk/view/ head:/src/ location_ service/ com/ubuntu/ location/ engine. cpp#L205
location::Engine is the central hub receiving and dispatching updates and it is meant to carry out such tasks as proposed in this MP.
Also make sure that an automated test case is added in http:// bazaar. launchpad. net/~phablet- team/location- service/ trunk/view/ head:/tests/ engine_ test.cpp. I would think that introducing a location: :PositionUpdate Filter interface together with a default implementation should help a lot here. You can pass the location: :PositionUpdate Filter to the location::Engine constructor as a functional dependency and subsequently test if the Engine calls into that object for position updates.
Rest of my comments inline.