Merge lp:~thomas-voss/location-service/do_not_bail_out_on_invalid_technology into lp:location-service/trunk

[Thomas Voß]

Allow for creation of initially invalid radio cells.

Allow for creation of initially invalid radio cells.

[Loïc Minier (lool)]

Looks good except for typo; haven't test yet

[lp:~thomas-voss/location-service/do_not_bail_out_on_invalid_technology]

129. By Thomas Voß on 2014-10-13

Fix typo discovered in review.

[Loïc Minier (lool)]

LGTM

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:128
http://jenkins.qa.ubuntu.com/job/location-service-ci/313/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-utopic-amd64-ci/220
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-utopic-armhf-ci/220
        deb: http://jenkins.qa.ubuntu.com/job/location-service-utopic-armhf-ci/220/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-utopic-i386-ci/220

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/location-service-ci/313/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:129
http://jenkins.qa.ubuntu.com/job/location-service-ci/314/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-utopic-amd64-ci/221
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-utopic-armhf-ci/221
        deb: http://jenkins.qa.ubuntu.com/job/location-service-utopic-armhf-ci/221/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/location-service-utopic-i386-ci/221

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/location-service-ci/314/rebuild

review: Approve (continuous-integration)

=== 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:21:12 +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_mcc();
+    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:21:12 +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:21:12 +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);
     }
 }