shill: cellular: Ensure that home_provider is set when modem enabled
Ensure that the home_provider is set when modem enabled. It is not
necessary to call either GetStatus or Register when a modem is
enabled, so delete the dead code.
Handle the registration state in Cellular::OnModemStarted because
registration state changes which occur before the device is enabled
are ignored.
Moved public methods to be protected methods and pushed them into
CellularCapabilityClassic since they are no longer used in
CellularCapabilityUniversal and do not need to be public.
Update the unit tests to ensure that home_provider_ is set when the
information from the SIM is updated.
BUG=none
TEST=unit tests, ran manually and connected without specifying an APN
Change-Id: Iadd400c99424e7dad858a264f58124c5636ff63f
Reviewed-on: https://gerrit.chromium.org/gerrit/21814
Tested-by: Jason Glasgow <jglasgow@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>
Commit-Ready: Jason Glasgow <jglasgow@chromium.org>
diff --git a/cellular.cc b/cellular.cc
index db6df6e..4b3d399 100644
--- a/cellular.cc
+++ b/cellular.cc
@@ -240,8 +240,12 @@
void Cellular::OnModemStarted(const EnabledStateChangedCallback &callback,
const Error &error) {
SLOG(Cellular, 2) << __func__ << ": " << GetStateString(state_);
- if (state_ == kStateDisabled)
+ if (state_ == kStateDisabled) {
SetState(kStateEnabled);
+ // Registration state updates may have been ignored while the
+ // modem was not yet marked enabled.
+ HandleNewRegistrationState();
+ }
callback.Run(error);
}
diff --git a/cellular_capability.h b/cellular_capability.h
index e7eca19..502ab95 100644
--- a/cellular_capability.h
+++ b/cellular_capability.h
@@ -81,8 +81,6 @@
// Invoked by the parent Cellular device when a new service is created.
virtual void OnServiceCreated() = 0;
- virtual void UpdateStatus(const DBusPropertiesMap &properties) = 0;
-
virtual void SetupConnectProperties(DBusPropertiesMap *properties) = 0;
// StartModem attempts to put the modem in a state in which it is
@@ -177,8 +175,6 @@
virtual bool AllowRoaming() = 0;
protected:
- virtual void GetRegistrationState() = 0;
-
// Releases all proxies held by the object. This is most useful
// during unit tests.
virtual void ReleaseProxies() = 0;
diff --git a/cellular_capability_cdma.h b/cellular_capability_cdma.h
index 599bc7c..3fb6edf 100644
--- a/cellular_capability_cdma.h
+++ b/cellular_capability_cdma.h
@@ -26,7 +26,6 @@
// Inherited from CellularCapability.
virtual void StartModem(Error *error, const ResultCallback &callback);
virtual void OnServiceCreated();
- virtual void UpdateStatus(const DBusPropertiesMap &properties);
virtual void SetupConnectProperties(DBusPropertiesMap *properties);
virtual void Activate(const std::string &carrier, Error *error,
const ResultCallback &callback);
@@ -49,6 +48,7 @@
protected:
virtual void InitProxies();
virtual void ReleaseProxies();
+ virtual void UpdateStatus(const DBusPropertiesMap &properties);
private:
friend class CellularCapabilityCDMATest;
diff --git a/cellular_capability_classic.h b/cellular_capability_classic.h
index 4e62020..664c9ba 100644
--- a/cellular_capability_classic.h
+++ b/cellular_capability_classic.h
@@ -88,6 +88,8 @@
const std::vector<std::string> &invalidated_properties);
protected:
+ virtual void GetRegistrationState() = 0;
+
// The following five methods are only ever called as
// callbacks (from the main loop), which is why they
// don't take an Error * argument.
@@ -101,6 +103,7 @@
void FinishDisable(const ResultCallback &callback);
virtual void InitProxies();
virtual void ReleaseProxies();
+ virtual void UpdateStatus(const DBusPropertiesMap &properties) = 0;
static void OnUnsupportedOperation(const char *operation, Error *error);
diff --git a/cellular_capability_gsm.h b/cellular_capability_gsm.h
index 455f164..bacc874 100644
--- a/cellular_capability_gsm.h
+++ b/cellular_capability_gsm.h
@@ -31,7 +31,6 @@
// Inherited from CellularCapability.
virtual void StartModem(Error *error, const ResultCallback &callback);
virtual void OnServiceCreated();
- virtual void UpdateStatus(const DBusPropertiesMap &properties);
virtual void SetupConnectProperties(DBusPropertiesMap *properties);
virtual void Connect(const DBusPropertiesMap &properties, Error *error,
const ResultCallback &callback);
@@ -79,6 +78,7 @@
protected:
virtual void InitProxies();
virtual void ReleaseProxies();
+ virtual void UpdateStatus(const DBusPropertiesMap &properties);
private:
friend class CellularTest;
@@ -100,6 +100,7 @@
FRIEND_TEST(CellularCapabilityGSMTest, RegisterOnNetwork);
FRIEND_TEST(CellularCapabilityGSMTest, Scan);
FRIEND_TEST(CellularCapabilityGSMTest, SetAccessTechnology);
+ FRIEND_TEST(CellularCapabilityGSMTest, UpdateStatus);
FRIEND_TEST(CellularCapabilityGSMTest, SetHomeProvider);
FRIEND_TEST(CellularCapabilityGSMTest, UpdateOperatorInfo);
FRIEND_TEST(CellularCapabilityGSMTest, GetRegistrationState);
diff --git a/cellular_capability_universal.cc b/cellular_capability_universal.cc
index 5b72c9a..454a4a0 100644
--- a/cellular_capability_universal.cc
+++ b/cellular_capability_universal.cc
@@ -205,37 +205,7 @@
// After modem is enabled, it should be possible to get properties
// TODO(jglasgow): handle errors from GetProperties
GetProperties();
-
- // Try to register
- Error local_error;
- modem_3gpp_proxy_->Register(
- selected_network_, &local_error,
- Bind(&CellularCapabilityUniversal::Start_RegisterCompleted,
- weak_ptr_factory_.GetWeakPtr(), callback),
- kTimeoutRegister);
- if (local_error.IsFailure()) {
- callback.Run(local_error);
- return;
- }
-}
-
-void CellularCapabilityUniversal::Start_RegisterCompleted(
- const ResultCallback &callback, const Error &error) {
- SLOG(Cellular, 2) << __func__ << ": " << error;
- if (error.IsSuccess()) {
- // Normally, running the callback is the last thing done in a method.
- // In this case, we do it first, because we want to make sure that
- // the device is marked as Enabled before the registration state is
- // handled. See comment in Cellular::HandleNewRegistrationState.
- callback.Run(error);
- // If registered, get the registration state and signal quality.
- GetRegistrationState();
- GetSignalQuality();
- } else {
- LOG(ERROR) << "registration failed: " << error;
- // Ignore registration errors, because that just means there is no signal.
- callback.Run(Error());
- }
+ callback.Run(error);
}
void CellularCapabilityUniversal::StopModem(Error *error,
@@ -338,13 +308,6 @@
UpdateServingOperator();
}
-void CellularCapabilityUniversal::UpdateStatus(
- const DBusPropertiesMap &properties) {
- if (ContainsKey(properties, kModemPropertyIMSI)) {
- SetHomeProvider();
- }
-}
-
// Create the list of APNs to try, in the following order:
// - last APN that resulted in a successful connection attempt on the
// current network (if any)
@@ -439,22 +402,6 @@
return requires_roaming || allow_roaming_property();
}
-void CellularCapabilityUniversal::GetRegistrationState() {
- SLOG(Cellular, 2) << __func__;
- string operator_code;
- string operator_name;
-
- const MMModem3gppRegistrationState state =
- static_cast<MMModem3gppRegistrationState>(
- modem_3gpp_proxy_->RegistrationState());
- if (state == MM_MODEM_3GPP_REGISTRATION_STATE_HOME ||
- state == MM_MODEM_3GPP_REGISTRATION_STATE_ROAMING) {
- operator_code = modem_3gpp_proxy_->OperatorCode();
- operator_name = modem_3gpp_proxy_->OperatorName();
- }
- On3GPPRegistrationChanged(state, operator_code, operator_name);
-}
-
void CellularCapabilityUniversal::GetProperties() {
SLOG(Cellular, 2) << __func__;
@@ -502,7 +449,11 @@
SLOG(Cellular, 2) << "GSM provider not found.";
return;
}
+
+ // Even if provider is the same as home_provider_, it is possible
+ // that the spn_ has changed. Run all the code below.
home_provider_ = provider;
+
Cellular::Operator oper;
if (provider->networks) {
oper.SetCode(provider->networks[0]);
@@ -965,10 +916,10 @@
if (sim_path.empty()) {
// Clear all data about the sim
- OnImsiChanged("");
+ imsi_ = "";
+ spn_ = "";
OnSimIdentifierChanged("");
OnOperatorIdChanged("");
- OnOperatorNameChanged("");
} else {
scoped_ptr<DBusPropertiesProxyInterface> properties_proxy(
proxy_factory()->CreateDBusPropertiesProxy(sim_path,
@@ -1160,20 +1111,25 @@
const vector<string> &/* invalidated_properties */) {
VLOG(2) << __func__;
string value;
- if (DBusProperties::GetString(props, MM_SIM_PROPERTY_IMSI, &value))
- OnImsiChanged(value);
+ bool must_update_home_provider = false;
if (DBusProperties::GetString(props, MM_SIM_PROPERTY_SIMIDENTIFIER, &value))
OnSimIdentifierChanged(value);
if (DBusProperties::GetString(props, MM_SIM_PROPERTY_OPERATORIDENTIFIER,
&value))
OnOperatorIdChanged(value);
- if (DBusProperties::GetString(props, MM_SIM_PROPERTY_OPERATORNAME, &value))
- OnOperatorNameChanged(value);
+ if (DBusProperties::GetString(props, MM_SIM_PROPERTY_OPERATORNAME, &value)) {
+ spn_ = value;
+ must_update_home_provider = true;
+ }
+ if (DBusProperties::GetString(props, MM_SIM_PROPERTY_IMSI, &value)) {
+ imsi_ = value;
+ must_update_home_provider = true;
+ }
// TODO(jglasgow): May eventually want to get SPDI, etc
-}
-void CellularCapabilityUniversal::OnImsiChanged(const string &imsi) {
- imsi_ = imsi;
+ if (must_update_home_provider)
+ SetHomeProvider();
+
}
void CellularCapabilityUniversal::OnSimIdentifierChanged(const string &id) {
@@ -1185,9 +1141,4 @@
operator_id_ = operator_id;
}
-void CellularCapabilityUniversal::OnOperatorNameChanged(
- const string &operator_name) {
- spn_ = operator_name;
-}
-
} // namespace shill
diff --git a/cellular_capability_universal.h b/cellular_capability_universal.h
index 3bc002e..16d489a 100644
--- a/cellular_capability_universal.h
+++ b/cellular_capability_universal.h
@@ -50,9 +50,7 @@
Error *error, const ResultCallback &callback);
virtual void OnServiceCreated();
- virtual void UpdateStatus(const DBusPropertiesMap &properties);
virtual void SetupConnectProperties(DBusPropertiesMap *properties);
- virtual void GetRegistrationState();
virtual void GetProperties();
virtual void Register(const ResultCallback &callback);
@@ -98,8 +96,6 @@
// Methods used in starting a modem
void Start_EnableModemCompleted(const ResultCallback &callback,
const Error &error);
- void Start_RegisterCompleted(const ResultCallback &callback,
- const Error &error);
// Methods used in stopping a modem
void Stop_DisconnectCompleted(const ResultCallback &callback,
diff --git a/cellular_capability_universal_unittest.cc b/cellular_capability_universal_unittest.cc
index fd63af4..a8dd456 100644
--- a/cellular_capability_universal_unittest.cc
+++ b/cellular_capability_universal_unittest.cc
@@ -94,6 +94,14 @@
capability_->proxy_factory_ = NULL;
}
+ void InitProviderDB() {
+ const char kTestMobileProviderDBPath[] = "provider_db_unittest.bfd";
+
+ provider_db_ = mobile_provider_open_db(kTestMobileProviderDBPath);
+ ASSERT_TRUE(provider_db_);
+ cellular_->provider_db_ = provider_db_;
+ }
+
void InvokeEnable(bool enable, Error *error,
const ResultCallback &callback, int timeout) {
callback.Run(Error());
@@ -174,6 +182,7 @@
TestProxyFactory proxy_factory_;
CellularCapabilityUniversal *capability_; // Owned by |cellular_|.
NiceMock<DeviceMockAdaptor> *device_adaptor_; // Owned by |cellular_|.
+ mobile_provider_db *provider_db_;
};
const char CellularCapabilityUniversalTest::kImei[] = "999911110000";
@@ -215,16 +224,6 @@
EXPECT_CALL(*properties_proxy_,
GetAll(MM_DBUS_INTERFACE_MODEM_MODEM3GPP))
.WillOnce(Return(modem3gpp_properties));
- EXPECT_CALL(*modem_3gpp_proxy_,
- Register(_, _, _, _))
- .WillOnce(Invoke(this, &CellularCapabilityUniversalTest::InvokeRegister));
- EXPECT_CALL(*modem_proxy_, SignalQuality()).WillOnce(Return(quality));
- EXPECT_CALL(*modem_3gpp_proxy_, RegistrationState())
- .WillOnce(Return(MM_MODEM_3GPP_REGISTRATION_STATE_HOME));
- EXPECT_CALL(*modem_3gpp_proxy_, OperatorName())
- .WillOnce(Return(operator_name));
- EXPECT_CALL(*modem_3gpp_proxy_, OperatorCode())
- .WillOnce(Return(operator_code));
// After setup we lose pointers to the proxies, so it is hard to set
// expectations.
@@ -321,7 +320,11 @@
// After setup we lose pointers to the proxies, so it is hard to set
// expectations.
SetUp();
+ InitProviderDB();
+ EXPECT_TRUE(cellular_->home_provider().GetName().empty());
+ EXPECT_TRUE(cellular_->home_provider().GetCountry().empty());
+ EXPECT_TRUE(cellular_->home_provider().GetCode().empty());
EXPECT_FALSE(capability_->sim_proxy_.get());
capability_->OnDBusPropertiesChanged(MM_DBUS_INTERFACE_MODEM,
modem_properties, vector<string>());
@@ -331,23 +334,35 @@
// Updating the SIM
DBusPropertiesMap new_properties;
- const char kNewImsi[] = "123123123";
+ const char kCountry[] = "us";
+ const char kCode[] = "310160";
+ const char kNewImsi[] = "310240123456789";
const char kSimIdentifier[] = "9999888";
- const char kOperatorIdentifier[] = "410";
- const char kOperatorName[] = "new operator";
+ const char kOperatorIdentifier[] = "310240";
+ const char kOperatorName[] = "Custom SPN";
new_properties[MM_SIM_PROPERTY_IMSI].writer().append_string(kNewImsi);
new_properties[MM_SIM_PROPERTY_SIMIDENTIFIER].writer().
append_string(kSimIdentifier);
new_properties[MM_SIM_PROPERTY_OPERATORIDENTIFIER].writer().
append_string(kOperatorIdentifier);
- new_properties[MM_SIM_PROPERTY_OPERATORNAME].writer().
- append_string(kOperatorName);
capability_->OnDBusPropertiesChanged(MM_DBUS_INTERFACE_SIM,
new_properties,
vector<string>());
EXPECT_EQ(kNewImsi, capability_->imsi_);
EXPECT_EQ(kSimIdentifier, capability_->sim_identifier_);
EXPECT_EQ(kOperatorIdentifier, capability_->operator_id_);
+ EXPECT_EQ("", capability_->spn_);
+ EXPECT_EQ("T-Mobile", cellular_->home_provider().GetName());
+ EXPECT_EQ(kCountry, cellular_->home_provider().GetCountry());
+ EXPECT_EQ(kCode, cellular_->home_provider().GetCode());
+ EXPECT_EQ(4, capability_->apn_list_.size());
+
+ new_properties[MM_SIM_PROPERTY_OPERATORNAME].writer().
+ append_string(kOperatorName);
+ capability_->OnDBusPropertiesChanged(MM_DBUS_INTERFACE_SIM,
+ new_properties,
+ vector<string>());
+ EXPECT_EQ(kOperatorName, cellular_->home_provider().GetName());
EXPECT_EQ(kOperatorName, capability_->spn_);
}