[Merge] lp:~ssweeny/location-service/fusion-provider.15.04 into lp:location-service/15.04
Thomas Voß
thomas.voss at canonical.com
Wed Mar 9 16:14:18 UTC 2016
Review: Needs Fixing
Minor remaining niggles inline.
Diff comments:
> === added file 'include/location_service/com/ubuntu/location/fusion_provider.h'
> --- include/location_service/com/ubuntu/location/fusion_provider.h 1970-01-01 00:00:00 +0000
> +++ include/location_service/com/ubuntu/location/fusion_provider.h 2016-03-08 22:20:34 +0000
> @@ -0,0 +1,60 @@
> +/*
> + * Copyright © 2016 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: Scott Sweeny <scott.sweeny at canonical.com
> + */
> +#ifndef LOCATION_SERVICE_COM_UBUNTU_LOCATION_FUSION_PROVIDER_H
> +#define LOCATION_SERVICE_COM_UBUNTU_LOCATION_FUSION_PROVIDER_H
> +
> +#include <com/ubuntu/location/provider.h>
> +#include <com/ubuntu/location/provider_selection_policy.h>
> +#include <com/ubuntu/location/update_selector.h>
> +
> +namespace com
> +{
> +namespace ubuntu
> +{
> +namespace location
> +{
> +class FusionProvider : public Provider
> +{
> +public:
> + typedef std::shared_ptr<FusionProvider> Ptr;
> +
> + FusionProvider(const std::set<Provider::Ptr>& providers, const UpdateSelector::Ptr& update_selector);
> + ~FusionProvider() = default;
No need for this, it is automatically generated by the compiler.
> +
> + bool matches_criteria(const Criteria &criteria) override;
> + void on_wifi_and_cell_reporting_state_changed(location::WifiAndCellIdReportingState state) override;
> + void on_reference_location_updated(const Update<Position>& position) override;
> + void on_reference_velocity_updated(const Update<Velocity>& velocity) override;
> + void on_reference_heading_updated(const Update<Heading>& heading) override;
> + void start_position_updates() override;
> + void stop_position_updates() override;
> + void start_heading_updates() override;
> + void stop_heading_updates() override;
> + void start_velocity_updates() override;
> + void stop_velocity_updates() override;
> +
> +private:
> + Optional<Update<Position>> last_position;
> + std::set<Provider::Ptr> providers;
> + std::vector<core::ScopedConnection> connections;
> +};
> +}
> +}
> +}
> +
> +#endif // LOCATION_SERVICE_COM_UBUNTU_LOCATION_FUSION_PROVIDER_H
>
> === added file 'include/location_service/com/ubuntu/location/newer_or_more_accurate_update_selector.h'
> --- include/location_service/com/ubuntu/location/newer_or_more_accurate_update_selector.h 1970-01-01 00:00:00 +0000
> +++ include/location_service/com/ubuntu/location/newer_or_more_accurate_update_selector.h 2016-03-08 22:20:34 +0000
> @@ -0,0 +1,60 @@
> +/*
> + * Copyright © 2016 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: Scott Sweeny <scott.sweeny at canonical.com
> + */
> +#ifndef LOCATION_SERVICE_COM_UBUNTU_LOCATIONNEWER_OR_MORE_ACCURATE_UPDATE_SELECTOR_H
> +#define LOCATION_SERVICE_COM_UBUNTU_LOCATIONNEWER_OR_MORE_ACCURATE_UPDATE_SELECTOR_H
> +
> +#include <com/ubuntu/location/update_selector.h>
> +
> +namespace com
> +{
> +namespace ubuntu
> +{
> +namespace location
> +{
> +class NewerOrMoreAccurateUpdateSelector : public UpdateSelector
> +{
> +public:
> + typedef std::shared_ptr<NewerOrMoreAccurateUpdateSelector> Ptr;
> +
> + const Update<Position>& select(const Update<Position>& older,
> + const Update<Position>& newer) override
> + {
> + // Basically copied this value from the Android fusion provider
> + const std::chrono::seconds cutoff(11);
Can very well be static.
> +
> + // If the new position is newer by a significant margin then just use it
> + if (newer.when > older.when + cutoff) {
> + return newer;
> + }
> +
> + // Choose the position with the smaller accuracy circle if both have them
> + if (!older.value.accuracy.horizontal)
> + return newer;
> + if (!newer.value.accuracy.horizontal)
> + return older;
> + if (newer.value.accuracy.horizontal < older.value.accuracy.horizontal)
> + return newer;
> + else
> + return older;
> + }
> +};
> +}
> +}
> +}
> +#endif // LOCATION_SERVICE_COM_UBUNTU_LOCATIONNEWER_OR_MORE_ACCURATE_UPDATE_SELECTOR_H
> +
>
> === added file 'include/location_service/com/ubuntu/location/update_selector.h'
> --- include/location_service/com/ubuntu/location/update_selector.h 1970-01-01 00:00:00 +0000
> +++ include/location_service/com/ubuntu/location/update_selector.h 2016-03-08 22:20:34 +0000
> @@ -0,0 +1,42 @@
> +/*
> + * Copyright © 2016 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: Scott Sweeny <scott.sweeny at canonical.com
> + */
> +#ifndef LOCATION_SERVICE_COM_UBUNTU_LOCATION_UPDATE_SELECTOR_H
> +#define LOCATION_SERVICE_COM_UBUNTU_LOCATION_UPDATE_SELECTOR_H
> +
> +#include <com/ubuntu/location/update.h>
> +#include <com/ubuntu/location/position.h>
> +
> +namespace com
> +{
> +namespace ubuntu
> +{
> +namespace location
> +{
> +class UpdateSelector
Please add a virtual dtor, and mark copy/move dtor/assigment operators as deleted. Add a default protected ctor.
> +{
> +public:
> + typedef std::shared_ptr<UpdateSelector> Ptr;
> +
> + const virtual Update<Position>& select(const Update<Position>& older,
Should return a copy to avoid returning a reference to a temporary value.
> + const Update<Position>& newer) = 0;
> +};
> +}
> +}
> +}
> +#endif // LOCATION_SERVICE_COM_UBUNTU_LOCATION_UPDATE_SELECTOR_H
> +
>
> === added file 'src/location_service/com/ubuntu/location/fusion_provider.cpp'
> --- src/location_service/com/ubuntu/location/fusion_provider.cpp 1970-01-01 00:00:00 +0000
> +++ src/location_service/com/ubuntu/location/fusion_provider.cpp 2016-03-08 22:20:34 +0000
> @@ -0,0 +1,138 @@
> +/*
> + * Copyright © 2016 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: Scott Sweeny <scott.sweeny at canonical.com
> + */
> + #include <com/ubuntu/location/fusion_provider.h>
> + #include <com/ubuntu/location/update.h>
> +
> +namespace cu = com::ubuntu;
> +namespace cul = com::ubuntu::location;
> +
> +cul::Provider::Features all_features()
> +{
> + return cul::Provider::Features::position |
> + cul::Provider::Features::heading |
> + cul::Provider::Features::velocity;
> +}
> +
> +cul::Provider::Requirements all_requirements()
> +{
> + return cul::Provider::Requirements::cell_network |
> + cul::Provider::Requirements::data_network |
> + cul::Provider::Requirements::monetary_spending |
> + cul::Provider::Requirements::satellites;
> +}
> +
> +
> +cul::FusionProvider::FusionProvider(const std::set<location::Provider::Ptr>& providers, const UpdateSelector::Ptr& update_selector)
> + : Provider{all_features(), all_requirements()},
> + providers{providers}
> +{
> +
> + for (auto provider : providers)
> + {
> + connections.push_back(provider->updates().position.connect(
> + [this, update_selector](const cul::Update<cul::Position>& u)
> + {
> + // if this is the first update, use it
> + if (!last_position) {
> + mutable_updates().position(u);
you can nicely fold both lines together by doing mutable_updates().position(last_position = u); operator= returns a reference to the new value.
> + last_position = u;
> + } else {
> + *last_position = update_selector->select(*last_position, u);
Same trick applies here, but you want to guard the call to update_selector->select with a try {...} catch (const std::exception& e) {...}. In the catch, logging a warning with LOG(WARNING) << "..."; should be good enough for now.
> + mutable_updates().position(*last_position);
> + }
> + }));
> + connections.push_back(provider->updates().heading.connect(
> + [this](const cul::Update<cul::Heading>& u)
> + {
> + mutable_updates().heading(u);
> + }));
> + connections.push_back(provider->updates().velocity.connect(
> + [this](const cul::Update<cul::Velocity>& u)
> + {
> + mutable_updates().velocity(u);
> + }));
> + }
> +
> +}
> +
> +// We always match :)
> +bool cul::FusionProvider::matches_criteria(const cul::Criteria&)
> +{
> + return true;
> +}
> +
> +// We forward all events to the other providers.
> +void cul::FusionProvider::on_wifi_and_cell_reporting_state_changed(cul::WifiAndCellIdReportingState state)
> +{
> + for (auto provider : providers)
> + provider->on_wifi_and_cell_reporting_state_changed(state);
> +}
> +
> +void cul::FusionProvider::on_reference_location_updated(const cul::Update<cul::Position>& position)
> +{
> + for (auto provider : providers)
> + provider->on_reference_location_updated(position);
> +}
> +
> +void cul::FusionProvider::on_reference_velocity_updated(const cul::Update<cul::Velocity>& velocity)
> +{
> + for (auto provider : providers)
> + provider->on_reference_velocity_updated(velocity);
> +}
> +
> +void cul::FusionProvider::on_reference_heading_updated(const cul::Update<cul::Heading>& heading)
> +{
> + for (auto provider : providers)
> + provider->on_reference_heading_updated(heading);
> +}
> +
> +void cul::FusionProvider::start_position_updates()
> +{
> + for (auto provider : providers)
> + provider->state_controller()->start_position_updates();
> +}
> +
> +void cul::FusionProvider::stop_position_updates()
> +{
> + for (auto provider : providers)
> + provider->state_controller()->stop_position_updates();
> +}
> +
> +void cul::FusionProvider::start_heading_updates()
> +{
> + for (auto provider : providers)
> + provider->state_controller()->start_heading_updates();
> +}
> +
> +void cul::FusionProvider::stop_heading_updates()
> +{
> + for (auto provider : providers)
> + provider->state_controller()->stop_heading_updates();
> +}
> +
> +void cul::FusionProvider::start_velocity_updates()
> +{
> + for (auto provider : providers)
> + provider->state_controller()->start_velocity_updates();
> +}
> +
> +void cul::FusionProvider::stop_velocity_updates()
> +{
> + for (auto provider : providers)
> + provider->state_controller()->stop_velocity_updates();
> +}
--
https://code.launchpad.net/~ssweeny/location-service/fusion-provider.15.04/+merge/287242
Your team Ubuntu Phablet Team is subscribed to branch lp:location-service/15.04.
More information about the Ubuntu-reviews
mailing list