[Merge] lp:~mandel/location-service/better-position-selection into lp:location-service
Thomas Voß
thomas.voss at canonical.com
Thu Apr 23 15:13:53 UTC 2015
Review: Needs Fixing
Looks better, but some comments inline.
Diff comments:
> === modified file 'src/location_service/com/ubuntu/location/CMakeLists.txt'
> --- src/location_service/com/ubuntu/location/CMakeLists.txt 2015-01-25 12:45:30 +0000
> +++ src/location_service/com/ubuntu/location/CMakeLists.txt 2015-04-23 14:53:52 +0000
> @@ -27,6 +27,7 @@
> proxy_provider.cpp
> satellite_based_positioning_state.cpp
> settings.cpp
> + time_based_update_policy.cpp
> set_name_for_thread.cpp
> wifi_and_cell_reporting_state.cpp
>
>
> === modified file 'src/location_service/com/ubuntu/location/engine.cpp'
> --- src/location_service/com/ubuntu/location/engine.cpp 2015-01-25 12:44:20 +0000
> +++ src/location_service/com/ubuntu/location/engine.cpp 2015-04-23 14:53:52 +0000
> @@ -24,6 +24,8 @@
> #include <stdexcept>
> #include <unordered_map>
>
> +#include "time_based_update_policy.h"
> +
> namespace cul = com::ubuntu::location;
>
> const cul::SatelliteBasedPositioningState cul::Engine::Configuration::Defaults::satellite_based_positioning_state;
> @@ -33,7 +35,8 @@
> cul::Engine::Engine(const cul::ProviderSelectionPolicy::Ptr& provider_selection_policy,
> const cul::Settings::Ptr& settings)
> : provider_selection_policy(provider_selection_policy),
> - settings(settings)
> + settings(settings),
> + update_policy(std::make_shared<cul::TimeBasedUpdatePolicy>())
> {
> if (!provider_selection_policy) throw std::runtime_error
> {
> @@ -204,7 +207,7 @@
> // We should come up with a better heuristic here.
> auto cpr = provider->updates().position.connect([this](const cul::Update<cul::Position>& src)
> {
> - updates.reference_location = src;
> + updates.reference_location = update_policy->verify_update(src);
> });
>
> std::lock_guard<std::mutex> lg(guard);
>
> === modified file 'src/location_service/com/ubuntu/location/engine.h'
> --- src/location_service/com/ubuntu/location/engine.h 2015-01-14 13:41:54 +0000
> +++ src/location_service/com/ubuntu/location/engine.h 2015-04-23 14:53:52 +0000
> @@ -33,6 +33,8 @@
> #include <mutex>
> #include <set>
>
> +#include "update_policy.h"
> +
> namespace com
> {
> namespace ubuntu
> @@ -193,6 +195,7 @@
> std::map<Provider::Ptr, ProviderConnections> providers;
> ProviderSelectionPolicy::Ptr provider_selection_policy;
> Settings::Ptr settings;
> + UpdatePolicy::Ptr update_policy;
> };
>
> /** @brief Pretty prints the given status to the given stream. */
>
> === added file 'src/location_service/com/ubuntu/location/time_based_update_policy.cpp'
> --- src/location_service/com/ubuntu/location/time_based_update_policy.cpp 1970-01-01 00:00:00 +0000
> +++ src/location_service/com/ubuntu/location/time_based_update_policy.cpp 2015-04-23 14:53:52 +0000
> @@ -0,0 +1,120 @@
> +/*
> + * Copyright © 2015 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: Manuel de la Pena <manuel.delapena at canonical.com>
> + */
> +
> +#include "time_based_update_policy.h"
> +
> +namespace {
> + int DEFAULT_TIME_LIMIT = 2;
default_time_limit, to be consistent with the rest of the codebase. Also any reason not to have a public static std::chrono::minutes default_timeout() function in the class itself? With that, you can just default the argument and save a ctor.
> +}
> +namespace com
> +{
> +namespace ubuntu
> +{
> +namespace location
> +{
> +
> +TimeBasedUpdatePolicy::TimeBasedUpdatePolicy()
> + : TimeBasedUpdatePolicy(DEFAULT_TIME_LIMIT)
> +{
> +}
> +
> +TimeBasedUpdatePolicy::TimeBasedUpdatePolicy(int mins)
> + : limit(mins)
> +{
> +
> +}
> +const location::Update<location::Position>& TimeBasedUpdatePolicy::verify_update(const location::Update<location::Position>& update)
> +{
> + std::lock_guard<std::mutex> guard(position_update_mutex);
> + bool use_new_update;
> + if (is_significantly_newer(update))
> + {
> + use_new_update = true;
> + }
> + else if (is_significantly_older(update))
> + {
> + use_new_update = false;
> + }
> + else
> + {
> + use_new_update = last_position_update.value.accuracy.horizontal && update.value.accuracy.horizontal
> + && *last_position_update.value.accuracy.horizontal >= *update.value.accuracy.horizontal;
> + }
> +
> + if (use_new_update)
> + {
> + last_position_update = update;
> + return update;
> + }
> + else
> + {
> + return last_position_update;
> + }
> +}
> +
> +
> +const location::Update<location::Heading>& TimeBasedUpdatePolicy::verify_update(const location::Update<location::Heading>& update)
> +{
> + std::lock_guard<std::mutex> guard(heading_update_mutex);
> + bool use_new_update;
> + if (is_significantly_newer(update))
> + {
> + use_new_update = true;
> + }
> + else if (is_significantly_older(update))
> + {
> + use_new_update = false;
> + }
> + if (use_new_update)
> + {
> + last_heading_update = update;
> + return update;
> + }
> + else
> + {
> + return last_heading_update;
> + }
> +}
> +
> +const location::Update<location::Velocity>& TimeBasedUpdatePolicy::verify_update(const location::Update<location::Velocity>& update)
> +{
> + std::lock_guard<std::mutex> guard(velocity_update_mutex);
> + bool use_new_update;
> + if (is_significantly_newer(update))
> + {
> + use_new_update = true;
> + }
> + else if (is_significantly_older(update))
> + {
> + use_new_update = false;
> + }
> +
> + if (use_new_update)
> + {
> + last_velocity_update = update;
> + return update;
> + }
> + else
> + {
> + return last_velocity_update;
> + }
> +}
> +
> +}
> +}
> +}
> \ No newline at end of file
>
> === added file 'src/location_service/com/ubuntu/location/time_based_update_policy.h'
> --- src/location_service/com/ubuntu/location/time_based_update_policy.h 1970-01-01 00:00:00 +0000
> +++ src/location_service/com/ubuntu/location/time_based_update_policy.h 2015-04-23 14:53:52 +0000
> @@ -0,0 +1,82 @@
> +/*
> + * Copyright © 2015 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: Manuel de la Pena <manuel.delapena at canonical.com>
> + */
> +#ifndef LOCATION_SERVICE_UBUNTU_LOCATION_SERVICE_TIMES_BASED_UPDATE_POLICY_H_
> +#define LOCATION_SERVICE_UBUNTU_LOCATION_SERVICE_TIMES_BASED_UPDATE_POLICY_H_
> +
> +#include <chrono>
> +#include <mutex>
> +
> +#include "update_policy.h"
> +
> +namespace com
> +{
> +namespace ubuntu
> +{
> +namespace location
> +{
> +
> +// An interface that can be implemented to add heuristics on how update will be chosen. This class
> +// allows developers to inject different heuristics in the engine to perform the update selection
> +// so that the app developers can take advantage of it.
> +class TimeBasedUpdatePolicy : public UpdatePolicy {
> +
> + public:
> + TimeBasedUpdatePolicy();
> + TimeBasedUpdatePolicy(int mins);
Please use std::chrono::minutes as argument to the ctor.
> + TimeBasedUpdatePolicy(const TimeBasedUpdatePolicy&) = delete;
> + ~TimeBasedUpdatePolicy() = default;
> +
> + // Return if the given position update will be verifyed as the new position in the engine.
> + const location::Update<location::Position>& verify_update(const location::Update<location::Position>& update) override;
> + // Return if the given heading update will be verifyed as the new heading in the engine.
> + const location::Update<location::Heading>& verify_update(const location::Update<location::Heading>& update) override;
> + // Return if the given velocity update will be verifyed as the new velocity in the engine.
> + const location::Update<location::Velocity>& verify_update(const location::Update<location::Velocity>& update) override;
> +
> + protected:
> + // not private to simplify the testing but should be private
> + template <class T> bool is_significantly_newer(const location::Update<T> update)
Just move those functions to update.h, there are utility functions of general interest.
> + {
> + auto delta = update.when - last_position_update.when;
> + return delta > limit;
> + }
> +
> + template <class T> bool is_significantly_older(const location::Update<T> update)
> + {
> + auto delta = update.when - last_position_update.when;
> + return delta < (-1 * limit);
> + }
> +
> + location::Update<location::Position> last_position_update;
> + location::Update<location::Heading> last_heading_update;
> + location::Update<location::Velocity> last_velocity_update;
> +
> + private:
> + // callbacks can happen in diff threads, make sure multi-threading will work in this class
> + std::mutex position_update_mutex;
> + std::mutex heading_update_mutex;
> + std::mutex velocity_update_mutex;
> + // used to calculate the time accepted bracket
> + std::chrono::minutes limit;
> +};
> +
> +}
> +}
> +}
> +
> +#endif //LOCATION_SERVICE_UBUNTU_LOCATION_SERVICE_TIMES_BASED_UPDATE_POLICY_H_
>
> === added file 'src/location_service/com/ubuntu/location/update_policy.h'
> --- src/location_service/com/ubuntu/location/update_policy.h 1970-01-01 00:00:00 +0000
> +++ src/location_service/com/ubuntu/location/update_policy.h 2015-04-23 14:53:52 +0000
> @@ -0,0 +1,57 @@
> +/*
> + * Copyright © 2015 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: Manuel de la Pena <manuel.delapena at canonical.com>
> + */
> +#ifndef LOCATION_SERVICE_UBUNTU_LOCATION_SERVICE_UPDATE_POLICY_H_
> +#define LOCATION_SERVICE_UBUNTU_LOCATION_SERVICE_UPDATE_POLICY_H_
> +
> +#include <memory>
> +
> +#include <com/ubuntu/location/heading.h>
> +#include <com/ubuntu/location/position.h>
> +#include <com/ubuntu/location/update.h>
> +#include <com/ubuntu/location/velocity.h>
> +
> +namespace com
> +{
> +namespace ubuntu
> +{
> +namespace location
> +{
> +
> +// An interface that can be implemented to add heuristics on how update will be chosen. This class
> +// allows developers to inject different heuristics in the engine to perform the update selection
> +// so that the app developers can take advantage of it.
> +class UpdatePolicy {
> +
Spurious whitespace.
> +
> + public:
> + typedef std::shared_ptr<UpdatePolicy> Ptr;
> +
As this is meant as a base class, we need at the very least a virtual dtor (feel free to default it). In addition I would propose that you mark copy c'tor, move c'tor and the respective assignment operators as deleted, along with making the default c'tor protected.
> + // Return if the given position update will be verifyed as the new position in the engine.
> + virtual const location::Update<location::Position>& verify_update(const location::Update<location::Position>& update) = 0;
> + // Return if the given heading update will be verifyed as the new heading in the engine.
> + virtual const location::Update<location::Heading>& verify_update(const location::Update<location::Heading>& update) = 0;
> + // Return if the given velocity update will be verifyed as the new velocity in the engine.
> + virtual const location::Update<location::Velocity>& verify_update(const location::Update<location::Velocity>& update) = 0;
> +
> +};
> +}
> +}
> +}
> +
> +#endif // LOCATION_SERVICE_UBUNTU_LOCATION_SERVICE_UPDATE_POLICY_H_
> +
>
--
https://code.launchpad.net/~mandel/location-service/better-position-selection/+merge/257037
Your team Ubuntu Phablet Team is subscribed to branch lp:location-service.
More information about the Ubuntu-reviews
mailing list