shill: Perform modem disconnect based on modem state.
The cellular state may not accurately reflect whether the underlying
modem is connecting/connected. If CellularCapabilityUniversal::Connect()
is called and while that call is still in progress, a call is made to
CellularCapabilityUniversal::StopModem(), the cellular state does not
indicate that the modem is connecting so StopModem() does not
attempt to disconnect. It's better to use the modem state to determine
if we need to disconnect the modem.
Also in this CL is a check to ignore stale modem Connected state
transitions. The stale state transition can occur when the modem is
asked to connect but is then quickly disabled. In this case, the
Connected state can show up after the modem has been disabled.
BUG=chromium-os:32241
TEST=Unit tests, cellular_Smoke, cellular_ModemControl
Change-Id: I7cc3316a287729529b097ca41c579b7ea6ecefdb
Reviewed-on: https://gerrit.chromium.org/gerrit/28441
Reviewed-by: Darin Petkov <petkov@chromium.org>
Tested-by: Thieu Le <thieule@chromium.org>
Commit-Ready: Thieu Le <thieule@chromium.org>
diff --git a/cellular.cc b/cellular.cc
index 889caf8..32c2ccd 100644
--- a/cellular.cc
+++ b/cellular.cc
@@ -217,6 +217,12 @@
return IsEnabledModemState(modem_state_);
}
+bool Cellular::IsModemRegistered() const {
+ return (modem_state_ == Cellular::kModemStateRegistered ||
+ modem_state_ == Cellular::kModemStateConnecting ||
+ modem_state_ == Cellular::kModemStateConnected);
+}
+
// static
bool Cellular::IsEnabledModemState(ModemState state) {
switch (state) {
@@ -549,9 +555,13 @@
capability_->SetUnregistered(new_state == kModemStateSearching);
HandleNewRegistrationState();
} else {
- SLOG(Cellular, 2) << __func__ << "Ignoring state change to Enabled";
+ SLOG(Cellular, 2) << __func__ << ": Ignoring state change to Enabled";
}
+ // Intentionally falls through.
case kModemStateRegistered:
+ // If the modem state changes from Connecting/Connected/Disconnecting
+ // to Registered/Enabled/Searching, then it's an indication that the
+ // modem has been disconnected or got disconnected by the network.
if (old_state == kModemStateConnected ||
old_state == kModemStateConnecting ||
old_state == kModemStateDisconnecting)
@@ -561,7 +571,10 @@
OnConnecting();
break;
case kModemStateConnected:
- OnConnected();
+ if (old_state == kModemStateConnecting)
+ OnConnected();
+ else
+ SLOG(Cellular, 2) << __func__ << ": Ignoring state change to Connected";
break;
default:
break;
diff --git a/cellular.h b/cellular.h
index babf933..ba59a76 100644
--- a/cellular.h
+++ b/cellular.h
@@ -145,6 +145,7 @@
void set_modem_state(ModemState state) { modem_state_ = state; }
ModemState modem_state() const { return modem_state_; }
bool IsUnderlyingDeviceEnabled() const;
+ bool IsModemRegistered() const;
static bool IsEnabledModemState(ModemState state);
mobile_provider_db *provider_db() const { return provider_db_; }
@@ -223,6 +224,8 @@
FRIEND_TEST(CellularTest, Disconnect);
FRIEND_TEST(CellularTest, ModemStateChangeDisable);
FRIEND_TEST(CellularTest, ModemStateChangeEnable);
+ FRIEND_TEST(CellularTest, ModemStateChangeStaleConnected);
+ FRIEND_TEST(CellularTest, ModemStateChangeValidConnected);
FRIEND_TEST(CellularTest, SetAllowRoaming);
FRIEND_TEST(CellularTest, StartModemCallback);
FRIEND_TEST(CellularTest, StartModemCallbackFail);
diff --git a/cellular_capability_universal.cc b/cellular_capability_universal.cc
index f3ff509..4c243e6 100644
--- a/cellular_capability_universal.cc
+++ b/cellular_capability_universal.cc
@@ -238,14 +238,12 @@
void CellularCapabilityUniversal::StopModem(Error *error,
const ResultCallback &callback) {
- SLOG(Cellular, 2) << __func__;
CHECK(!callback.is_null());
CHECK(error);
- Cellular::State state = cellular()->state();
- bool connected = (state == Cellular::kStateConnected ||
- state == Cellular::kStateLinked);
+ Cellular::ModemState state = cellular()->modem_state();
+ SLOG(Cellular, 2) << __func__ << "(" << state << ")";
- if (connected) {
+ if (cellular()->IsModemRegistered()) {
string all_bearers("/"); // "/" means all bearers. See Modemanager docs.
modem_simple_proxy_->Disconnect(
all_bearers,
diff --git a/cellular_capability_universal_unittest.cc b/cellular_capability_universal_unittest.cc
index 5960dad..a8eb207 100644
--- a/cellular_capability_universal_unittest.cc
+++ b/cellular_capability_universal_unittest.cc
@@ -332,7 +332,7 @@
Disconnect(::DBus::Path("/"), _, _,
CellularCapability::kTimeoutDefault))
.WillOnce(SaveArg<2>(&disconnect_callback));
- capability_->cellular()->state_ = Cellular::kStateConnected;
+ capability_->cellular()->modem_state_ = Cellular::kModemStateConnected;
capability_->StopModem(&error, callback);
EXPECT_TRUE(error.IsSuccess());
diff --git a/cellular_unittest.cc b/cellular_unittest.cc
index c276362..37b8e0f 100644
--- a/cellular_unittest.cc
+++ b/cellular_unittest.cc
@@ -683,6 +683,28 @@
EXPECT_FALSE(device_->enabled());
}
+TEST_F(CellularTest, ModemStateChangeStaleConnected) {
+ // Test to make sure that we ignore stale modem Connected state transitions.
+ // When a modem is asked to connect and before the connect completes, the
+ // modem is disabled, it may send a stale Connected state transition after
+ // it has been disabled.
+ device_->state_ = Cellular::kStateDisabled;
+ device_->OnModemStateChanged(Cellular::kModemStateEnabling,
+ Cellular::kModemStateConnected,
+ 0);
+ EXPECT_EQ(Cellular::kStateDisabled, device_->state());
+}
+
+TEST_F(CellularTest, ModemStateChangeValidConnected) {
+ device_->state_ = Cellular::kStateEnabled;
+ device_->service_ = new CellularService(
+ &control_interface_, &dispatcher_, &metrics_, &manager_, device_);
+ device_->OnModemStateChanged(Cellular::kModemStateConnecting,
+ Cellular::kModemStateConnected,
+ 0);
+ EXPECT_EQ(Cellular::kStateConnected, device_->state());
+}
+
TEST_F(CellularTest, StartModemCallback) {
EXPECT_CALL(*this, TestCallback(IsSuccess()));
EXPECT_EQ(device_->state_, Cellular::kStateDisabled);