[Merge] lp:~thomas-voss/location-service/do_not_bail_out_on_invalid_technology into lp:location-service
Loïc Minier
loic.minier at ubuntu.com
Mon Oct 13 15:18:50 UTC 2014
Looks good except for typo; haven't test yet
Diff comments:
> === modified file 'src/location_service/com/ubuntu/location/connectivity/cached_radio_cell.cpp'
> --- src/location_service/com/ubuntu/location/connectivity/cached_radio_cell.cpp 2014-10-08 19:31:16 +0000
> +++ src/location_service/com/ubuntu/location/connectivity/cached_radio_cell.cpp 2014-10-13 15:12:57 +0000
> @@ -94,67 +94,27 @@
> roaming(false),
> radio_type(Type::gsm),
> modem(modem),
> - connections
> - {
> - modem.signals.property_changed->connect([this](const std::tuple<std::string, core::dbus::types::Variant>& tuple)
> - {
> - on_modem_property_changed(tuple);
> - }),
> - modem.network_registration.signals.property_changed->connect([this](const std::tuple<std::string, core::dbus::types::Variant>& tuple)
> - {
> - on_network_registration_property_changed(tuple);
> - })
> - },
> - detail{Gsm()}
> + detail{}
> {
> - auto technology =
> - modem.network_registration.get<
> - org::Ofono::Manager::Modem::NetworkRegistration::Technology
> - >();
> -
> - auto it = type_lut().find(technology);
> -
> - if (it == type_lut().end()) throw std::runtime_error
> - {
> - "Unknown technology for connected cell: " + technology
> - };
> -
> - if (it->second == com::ubuntu::location::connectivity::RadioCell::Type::unknown) throw std::runtime_error
> - {
> - "Unknown technology for connected cell: " + technology
> - };
> -
> - radio_type = it->second;
> -
> - auto lac =
> - modem.network_registration.get<
> - org::Ofono::Manager::Modem::NetworkRegistration::LocationAreaCode
> - >();
> -
> - int cell_id =
> - modem.network_registration.get<
> - org::Ofono::Manager::Modem::NetworkRegistration::CellId
> - >();
> -
> - auto strength =
> - modem.network_registration.get<
> - org::Ofono::Manager::Modem::NetworkRegistration::Strength
> - >();
> -
> - std::stringstream ssmcc
> - {
> - modem.network_registration.get<
> - org::Ofono::Manager::Modem::NetworkRegistration::MobileCountryCode
> - >()
> - };
> - int mcc{0}; ssmcc >> mcc;
> - std::stringstream ssmnc
> - {
> - modem.network_registration.get<
> - org::Ofono::Manager::Modem::NetworkRegistration::MobileNetworkCode
> - >()
> - };
> - int mnc{0}; ssmnc >> mnc;
> + auto status = query_status();
> + radio_type = query_technology();
> +
> + connections.network_registration_properties_changed = modem.network_registration.signals.property_changed->connect([this](const std::tuple<std::string, core::dbus::types::Variant>& tuple)
> + {
> + on_network_registration_property_changed(tuple);
> + });
> +
> + if (not is_registered_or_roaming(status))
> + return;
> +
> + if (radio_type == com::ubuntu::location::connectivity::RadioCell::Type::unknown)
> + return;
> +
> + auto lac = query_lac();
> + auto mcc = query_mnc();
Typo
> + auto mnc = query_mnc();
> + auto cell_id = query_cid();
> + auto strength = query_strength();
>
> switch(radio_type)
> {
> @@ -205,18 +165,15 @@
> break;
> }
>
> - roaming =
> - modem.network_registration.get<
> - org::Ofono::Manager::Modem::NetworkRegistration::Status
> - >() == org::Ofono::Manager::Modem::NetworkRegistration::Status::roaming;
> + roaming = status == org::Ofono::Manager::Modem::NetworkRegistration::Status::Value::roaming;
>
> execute_cell_change_heuristics_if_appropriate();
> }
>
> detail::CachedRadioCell::~CachedRadioCell()
> {
> - modem.signals.property_changed->disconnect(connections.modem_properties_changed);
> - modem.network_registration.signals.property_changed->disconnect(connections.network_registration_properties_changed);
> + // TODO(tvoss): Reenable this once we know why the modem cache gets flushed.
> + // modem.network_registration.signals.property_changed->disconnect(connections.network_registration_properties_changed);
> }
>
> const core::Property<bool>& detail::CachedRadioCell::is_roaming() const
> @@ -263,11 +220,6 @@
> return detail.lte;
> }
>
> -void detail::CachedRadioCell::on_modem_property_changed(const std::tuple<std::string, core::dbus::types::Variant>& tuple)
> -{
> - VLOG(10) << "Property on modem " << modem.object->path() << " changed: " << std::get<0>(tuple);
> -}
> -
> void detail::CachedRadioCell::on_network_registration_property_changed(const std::tuple<std::string, core::dbus::types::Variant>& tuple)
> {
> VLOG(10) << "Property changed on modem " << modem.object->path() << " for network registration: " << std::get<0>(tuple);
> @@ -367,6 +319,8 @@
> break;
> }
> default:
> + // We take the default path here, specifically for cases where
> + // we started out with Technology::unknown.
> break;
> };
>
> @@ -566,8 +520,168 @@
> cell_change_heuristics.valid = false;
> });
>
> - cell_change_heuristics.valid = true;
> - }
> + cell_change_heuristics.valid = is_cell_details_valid();
> + }
> +}
> +
> +// Queries the status from the Ofono NetworkRegistration.
> +org::Ofono::Manager::Modem::NetworkRegistration::Status::Value detail::CachedRadioCell::query_status()
> +{
> + auto status =
> + modem.network_registration.get
> + <
> + org::Ofono::Manager::Modem::NetworkRegistration::Status
> + >();
> +
> + if (status == org::Ofono::Manager::Modem::NetworkRegistration::Status::unregistered)
> + return org::Ofono::Manager::Modem::NetworkRegistration::Status::Value::unregistered;
> +
> + if (status == org::Ofono::Manager::Modem::NetworkRegistration::Status::registered)
> + return org::Ofono::Manager::Modem::NetworkRegistration::Status::Value::registered;
> +
> + if (status == org::Ofono::Manager::Modem::NetworkRegistration::Status::searching)
> + return org::Ofono::Manager::Modem::NetworkRegistration::Status::Value::searching;
> +
> + if (status == org::Ofono::Manager::Modem::NetworkRegistration::Status::denied)
> + return org::Ofono::Manager::Modem::NetworkRegistration::Status::Value::denied;
> +
> + if (status == org::Ofono::Manager::Modem::NetworkRegistration::Status::unknown)
> + return org::Ofono::Manager::Modem::NetworkRegistration::Status::Value::unknown;
> +
> + if (status == org::Ofono::Manager::Modem::NetworkRegistration::Status::unregistered)
> + return org::Ofono::Manager::Modem::NetworkRegistration::Status::Value::unregistered;
> +
> + if (status == org::Ofono::Manager::Modem::NetworkRegistration::Status::roaming)
> + return org::Ofono::Manager::Modem::NetworkRegistration::Status::Value::roaming;
> +
> + return org::Ofono::Manager::Modem::NetworkRegistration::Status::Value::unknown;
> +}
> +
> +// Queries the technology from the Ofono NetworkRegistration.
> +com::ubuntu::location::connectivity::RadioCell::Type detail::CachedRadioCell::query_technology()
> +{
> + auto technology =
> + modem.network_registration.get<
> + org::Ofono::Manager::Modem::NetworkRegistration::Technology
> + >();
> +
> + auto it = type_lut().find(technology);
> +
> + if (it == type_lut().end())
> + return com::ubuntu::location::connectivity::RadioCell::Type::unknown;
> +
> + return it->second;
> +}
> +
> +// Queries the cell id from the Ofono NetworkRegistration.
> +int detail::CachedRadioCell::query_cid()
> +{
> + return modem.network_registration.get
> + <
> + org::Ofono::Manager::Modem::NetworkRegistration::CellId
> + >();
> +}
> +
> +// Queries the location area code from the Ofono NetworkRegistration.
> +std::uint16_t detail::CachedRadioCell::query_lac()
> +{
> + return modem.network_registration.get
> + <
> + org::Ofono::Manager::Modem::NetworkRegistration::LocationAreaCode
> + >();
> +}
> +
> +// Queries the mobile network code from the Ofono NetworkRegistration.
> +int detail::CachedRadioCell::query_mnc()
> +{
> + std::stringstream ssmnc
> + {
> + modem.network_registration.get
> + <
> + org::Ofono::Manager::Modem::NetworkRegistration::MobileNetworkCode
> + >()
> + };
> + int mnc{0}; ssmnc >> mnc;
> + return mnc;
> +}
> +
> +// Queries the mobile country code from the Ofono NetworkRegistration.
> +int detail::CachedRadioCell::query_mcc()
> +{
> + std::stringstream ssmcc
> + {
> + modem.network_registration.get
> + <
> + org::Ofono::Manager::Modem::NetworkRegistration::MobileCountryCode
> + >()
> + };
> + int mcc{0}; ssmcc >> mcc;
> + return mcc;
> +}
> +
> +// Queries the signal strength from the Ofono NetworkRegistration.
> +std::int8_t detail::CachedRadioCell::query_strength()
> +{
> + return modem.network_registration.get
> + <
> + org::Ofono::Manager::Modem::NetworkRegistration::Strength
> + >();
> +}
> +
> +// Returns true iff status is either roaming or registered.
> +bool detail::CachedRadioCell::is_registered_or_roaming(org::Ofono::Manager::Modem::NetworkRegistration::Status::Value status)
> +{
> + return status == org::Ofono::Manager::Modem::NetworkRegistration::Status::Value::registered ||
> + status == org::Ofono::Manager::Modem::NetworkRegistration::Status::Value::roaming;
> +}
> +
> +// Returns true iff all the parts of the cell are populated with valid values.
> +bool detail::CachedRadioCell::is_cell_details_valid()
> +{
> + switch(radio_type)
> + {
> + case com::ubuntu::location::connectivity::RadioCell::Type::unknown:
> + return false;
> + case com::ubuntu::location::connectivity::RadioCell::Type::gsm:
> + return is_gsm_valid();
> + case com::ubuntu::location::connectivity::RadioCell::Type::umts:
> + return is_umts_valid();
> + case com::ubuntu::location::connectivity::RadioCell::Type::lte:
> + return is_lte_valid();
> + }
> +
> + return false;
> +}
> +
> +// Retuns true iff the GSM cell details are valid.
> +bool detail::CachedRadioCell::is_gsm_valid()
> +{
> + return gsm().mobile_country_code.is_valid() &&
> + gsm().mobile_network_code.is_valid() &&
> + gsm().location_area_code.is_valid() &&
> + gsm().id.is_valid() &&
> + gsm().strength.is_valid();
> +}
> +
> +// Returns true iff the Umts cell details are valid.
> +bool detail::CachedRadioCell::is_umts_valid()
> +{
> + return umts().mobile_country_code.is_valid() &&
> + umts().mobile_network_code.is_valid() &&
> + umts().location_area_code.is_valid() &&
> + umts().id.is_valid() &&
> + umts().strength.is_valid();
> +}
> +
> +// Returns true iff the Lte cell details are valid.
> +bool detail::CachedRadioCell::is_lte_valid()
> +{
> + return lte().mobile_country_code.is_valid() &&
> + lte().mobile_network_code.is_valid() &&
> + lte().tracking_area_code.is_valid() &&
> + lte().id.is_valid() &&
> + lte().physical_id.is_valid() &&
> + lte().strength.is_valid();
> }
>
> detail::CachedRadioCell::Detail::Detail() : none(detail::CachedRadioCell::None{})
>
> === modified file 'src/location_service/com/ubuntu/location/connectivity/cached_radio_cell.h'
> --- src/location_service/com/ubuntu/location/connectivity/cached_radio_cell.h 2014-10-08 19:31:16 +0000
> +++ src/location_service/com/ubuntu/location/connectivity/cached_radio_cell.h 2014-10-13 15:12:57 +0000
> @@ -64,9 +64,6 @@
> // Returns LTE-specific details or throws std::runtime_error if this is not an LTE radiocell.
> const com::ubuntu::location::connectivity::RadioCell::Lte& lte() const override;
>
> - // Invoked whenever a modem property changes remotely in ofono.
> - void on_modem_property_changed(const std::tuple<std::string, core::dbus::types::Variant>& tuple);
> -
> // Invoked whenever a property specific to a network registration changes remotely.
> void on_network_registration_property_changed(const std::tuple<std::string, core::dbus::types::Variant>& tuple);
>
> @@ -89,11 +86,35 @@
> // Property to indicate whether the current cell is
> // still valid according to the cell change heuristics.
> core::Property<bool> valid;
> - } cell_change_heuristics;
> -
> + } cell_change_heuristics;
> // Executes the cell change heuristics if precondition is met.
> void execute_cell_change_heuristics_if_appropriate();
>
> + // Queries the status from the Ofono NetworkRegistration.
> + org::Ofono::Manager::Modem::NetworkRegistration::Status::Value query_status();
> + // Queries the technology from the Ofono NetworkRegistration.
> + Type query_technology();
> + // Queries the cell id from the Ofono NetworkRegistration.
> + int query_cid();
> + // Queries the location area code from the Ofono NetworkRegistration.
> + std::uint16_t query_lac();
> + // Queries the mobile network code from the Ofono NetworkRegistration.
> + int query_mnc();
> + // Queries the mobile country code from the Ofono NetworkRegistration.
> + int query_mcc();
> + // Queries the signal strength from the Ofono NetworkRegistration.
> + std::int8_t query_strength();
> + // Returns true iff status is either roaming or registered.
> + bool is_registered_or_roaming(org::Ofono::Manager::Modem::NetworkRegistration::Status::Value status);
> + // Returns true iff all the parts of the cell are populated with valid values.
> + bool is_cell_details_valid();
> + // Retuns true iff the GSM cell details are valid.
> + bool is_gsm_valid();
> + // Returns true iff the Umts cell details are valid.
> + bool is_umts_valid();
> + // Returns true iff the Lte cell details are valid.
> + bool is_lte_valid();
> +
> core::Property<bool> roaming;
> core::Signal<> on_changed;
> Type radio_type;
> @@ -104,12 +125,6 @@
> {
> core::dbus::Signal
> <
> - org::Ofono::Manager::Modem::PropertyChanged,
> - org::Ofono::Manager::Modem::PropertyChanged::ArgumentType
> - >::SubscriptionToken modem_properties_changed;
> -
> - core::dbus::Signal
> - <
> org::Ofono::Manager::Modem::NetworkRegistration::PropertyChanged,
> org::Ofono::Manager::Modem::NetworkRegistration::PropertyChanged::ArgumentType
> >::SubscriptionToken network_registration_properties_changed;
>
> === modified file 'src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp'
> --- src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp 2014-10-09 13:22:04 +0000
> +++ src/location_service/com/ubuntu/location/connectivity/ofono_nm_connectivity_manager.cpp 2014-10-13 15:12:57 +0000
> @@ -229,6 +229,8 @@
>
> void connectivity::OfonoNmConnectivityManager::Private::on_modem_added(const core::dbus::types::ObjectPath& path)
> {
> + VLOG(1) << __PRETTY_FUNCTION__;
> +
> auto modem = modem_manager->modem_for_path(path);
>
> // We immediately make the modem known to the cache, specifically
> @@ -313,6 +315,8 @@
>
> void connectivity::OfonoNmConnectivityManager::Private::on_modem_removed(const core::dbus::types::ObjectPath& path)
> {
> + VLOG(1) << __PRETTY_FUNCTION__;
> +
> detail::CachedRadioCell::Ptr cell;
> {
> std::lock_guard<std::mutex> lg(cached.guard);
> @@ -371,33 +375,45 @@
> ul.unlock(); signals.connected_cell_removed(cell);
> } else if (not has_cell_for_modem and modem_has_network_registration)
> {
> - // A new network registration is coming in and we have to create
> - // a corresponding cell instance.
> - auto cell = std::make_shared<detail::CachedRadioCell>(itm->second, dispatcher.service);
> -
> - // We do not keep the cell alive.
> - std::weak_ptr<detail::CachedRadioCell> wp{cell};
> -
> - // We account for a cell becoming invalid and report it as report.
> - cell->is_valid().changed().connect([this, wp](bool valid)
> + dispatcher.service.post([this, path]()
> {
> - VLOG(10) << "Validity of cell changed: " << std::boolalpha << valid << std::endl;
> -
> - auto sp = wp.lock();
> -
> - if (not sp)
> + std::unique_lock<std::mutex> ul(cached.guard);
> +
> + auto itm = cached.modems.find(path);
> + if (itm == cached.modems.end())
> + {
> + VLOG(1) << "Could not find a modem for path " << path.as_string();
> return;
> -
> - if (valid)
> - signals.connected_cell_added(sp);
> - else
> - signals.connected_cell_removed(sp);
> + }
> +
> + // A new network registration is coming in and we have to create
> + // a corresponding cell instance.
> + auto cell = std::make_shared<detail::CachedRadioCell>(itm->second, dispatcher.service);
> +
> + // We do not keep the cell alive.
> + std::weak_ptr<detail::CachedRadioCell> wp{cell};
> +
> + // We account for a cell becoming invalid and report it as report.
> + cell->is_valid().changed().connect([this, wp](bool valid)
> + {
> + VLOG(10) << "Validity of cell changed: " << std::boolalpha << valid << std::endl;
> +
> + auto sp = wp.lock();
> +
> + if (not sp)
> + return;
> +
> + if (valid)
> + signals.connected_cell_added(sp);
> + else
> + signals.connected_cell_removed(sp);
> + });
> +
> + cached.cells.insert(std::make_pair(path,cell));
> + // Cache is up to date now and we announce the new cell to
> + // API customers, with the lock on the cache not being held.
> + ul.unlock(); signals.connected_cell_added(cell);
> });
> -
> - cached.cells.insert(std::make_pair(path,cell));
> - // Cache is up to date now and we announce the new cell to
> - // API customers, with the lock on the cache not being held.
> - ul.unlock(); signals.connected_cell_added(cell);
> }
> }
>
>
--
https://code.launchpad.net/~thomas-voss/location-service/do_not_bail_out_on_invalid_technology/+merge/238175
Your team Ubuntu Phablet Team is requested to review the proposed merge of lp:~thomas-voss/location-service/do_not_bail_out_on_invalid_technology into lp:location-service.
More information about the Ubuntu-reviews
mailing list