Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ublox] Do not run AT+COPS=0 when performing warm bootup if already registering/registered #2139

Merged
merged 4 commits into from
Jun 29, 2020
Merged

[ublox] Do not run AT+COPS=0 when performing warm bootup if already registering/registered #2139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4030584
[cellular] when resuming cellular connection do not run COPS=0 again …
avtolstoy Jun 18, 2020
9864d2e
[ncp] treat failure to open the data channel as critical error
avtolstoy Jun 18, 2020
7092329
[quectel] undo conditional cops
avtolstoy Jun 18, 2020
0c38666
[cellular] enable packet domain error reporting
avtolstoy Jun 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions hal/network/ncp_client/quectel/quectel_ncp_client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,9 @@ const auto ICCID_MAX_LENGTH = 20;
using LacType = decltype(CellularGlobalIdentity::location_area_code);
using CidType = decltype(CellularGlobalIdentity::cell_id);

const int QUECTEL_DEFAULT_CID = 1;
const char QUECTEL_DEFAULT_PDP_TYPE[] = "IP";

} // anonymous

QuectelNcpClient::QuectelNcpClient() {
Expand Down Expand Up @@ -983,6 +986,9 @@ int QuectelNcpClient::initReady(ModemState state) {
// just in case
CHECK_PARSER(parser_.execCommand("AT+QCFG=\"cmux/urcport\",1"));

// Enable packet domain error reporting
CHECK_PARSER_OK(parser_.execCommand("AT+CGEREP=1,0"));

return SYSTEM_ERROR_NONE;
}

Expand Down Expand Up @@ -1106,7 +1112,8 @@ int QuectelNcpClient::configureApn(const CellularNetworkConfig& conf) {
netConf_ = networkConfigForImsi(buf, strlen(buf));
}
// FIXME: for now IPv4 context only
auto resp = parser_.sendCommand("AT+CGDCONT=1,\"IP\",\"%s\"",
auto resp = parser_.sendCommand("AT+CGDCONT=%d,\"%s\",\"%s\"",
QUECTEL_DEFAULT_CID, QUECTEL_DEFAULT_PDP_TYPE,
netConf_.hasApn() ? netConf_.apn() : "");
const int r = CHECK_PARSER(resp.readResult());
CHECK_TRUE(r == AtResponse::OK, SYSTEM_ERROR_UNKNOWN);
Expand Down Expand Up @@ -1199,9 +1206,10 @@ void QuectelNcpClient::connectionState(NcpConnectionState state) {
return SYSTEM_ERROR_NONE;
}, this);
if (r) {
LOG(ERROR, "Failed to open data channel");
ready_ = false;
connState_ = NcpConnectionState::DISCONNECTED;
}
muxer_.resumeChannel(QUECTEL_NCP_PPP_CHANNEL);
}

const auto handler = conf_.eventHandler();
Expand Down
28 changes: 22 additions & 6 deletions hal/network/ncp_client/sara/sara_ncp_client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,9 @@ using CidType = decltype(CellularGlobalIdentity::cell_id);
const size_t UBLOX_NCP_R4_BYTES_PER_WINDOW_THRESHOLD = 512;
const system_tick_t UBLOX_NCP_R4_WINDOW_SIZE_MS = 50;

const int UBLOX_DEFAULT_CID = 1;
const char UBLOX_DEFAULT_PDP_TYPE[] = "IP";


} // anonymous

Expand Down Expand Up @@ -1078,6 +1081,9 @@ int SaraNcpClient::initReady(ModemState state) {
// (allows the capture of `mcc` and `mnc`)
int r = CHECK_PARSER(parser_.execCommand("AT+COPS=3,2"));

// Enable packet domain error reporting
CHECK_PARSER_OK(parser_.execCommand("AT+CGEREP=1,0"));

if (state != ModemState::MuxerAtChannel) {
if (conf_.ncpIdentifier() != PLATFORM_NCP_SARA_R410) {
// Change the baudrate to 921600
Expand Down Expand Up @@ -1347,9 +1353,9 @@ int SaraNcpClient::configureApn(const CellularNetworkConfig& conf) {
CHECK_TRUE(r == AtResponse::OK, SYSTEM_ERROR_AT_NOT_OK);
netConf_ = networkConfigForImsi(buf, strlen(buf));
}
// FIXME: for now IPv4 context only
CHECK_PARSER_OK(parser_.execCommand("AT+CGDCONT?"));
auto resp = parser_.sendCommand("AT+CGDCONT=1,\"IP\",\"%s%s\"",

auto resp = parser_.sendCommand("AT+CGDCONT=%d,\"%s\",\"%s%s\"",
UBLOX_DEFAULT_CID, UBLOX_DEFAULT_PDP_TYPE,
(netConf_.hasUser() && netConf_.hasPassword()) ? "CHAP:" : "",
netConf_.hasApn() ? netConf_.apn() : "");
const int r = CHECK_PARSER(resp.readResult());
Expand Down Expand Up @@ -1377,11 +1383,20 @@ int SaraNcpClient::registerNet() {
connectionState(NcpConnectionState::CONNECTING);
registeredTime_ = 0;

CHECK_PARSER_OK(parser_.execCommand("AT+COPS?"));
auto resp = parser_.sendCommand("AT+COPS?");
int copsState = -1;
r = CHECK_PARSER(resp.scanf("+COPS: %d", &copsState));
CHECK_TRUE(r == 1, SYSTEM_ERROR_AT_RESPONSE_UNEXPECTED);
r = CHECK_PARSER(resp.readResult());
CHECK_TRUE(r == AtResponse::OK, SYSTEM_ERROR_AT_NOT_OK);

// NOTE: up to 3 mins (FIXME: there seems to be a bug where this timeout of 3 minutes
// is not being respected by u-blox modems. Setting to 5 for now.)
r = CHECK_PARSER(parser_.execCommand(5 * 60 * 1000, "AT+COPS=0,2"));
if (copsState != 0) {
// If the set command with <mode>=0 is issued, a further set
// command with <mode>=0 is managed as a user reselection
r = CHECK_PARSER(parser_.execCommand(5 * 60 * 1000, "AT+COPS=0,2"));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One quick thought on this: As written in the attached AT manual, this will trigger a re-selection which we want to avoid for faster bootup times (which is all good). If we have to deal with roaming locations though, is it worth having an AT+COPS=0 setting to in fact trigger a re-selection because otherwise what if it doesn't attach to network as expected. Am I making sense? Should we have a check that tells when to set AT+COPS=0 and when not to set it? I know we have 10 min timeout, but even after 10min when we hit this line, looks like we won't do another AT+COPS=0 in case which we may not register on a given network with edge case roaming scenarios and such?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But after 10 mins and a modem reset the modem on its own will start a new registration process (auto-COPS), right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. good point. That's also right. It should. (Reset is supposed to be more powerful than anything else.)

// Ignore response code here
// CHECK_TRUE(r == AtResponse::OK, SYSTEM_ERROR_AT_NOT_OK);

Expand Down Expand Up @@ -1447,9 +1462,10 @@ void SaraNcpClient::connectionState(NcpConnectionState state) {
return SYSTEM_ERROR_NONE;
}, this);
if (r) {
LOG(ERROR, "Failed to open data channel");
ready_ = false;
connState_ = NcpConnectionState::DISCONNECTED;
}
muxer_.resumeChannel(UBLOX_NCP_PPP_CHANNEL);
}

const auto handler = conf_.eventHandler();
Expand Down