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

cellular: registration status change fix #8795

Merged
merged 3 commits into from Nov 30, 2018
Merged

cellular: registration status change fix #8795

merged 3 commits into from Nov 30, 2018

Conversation

ghost
Copy link

@ghost ghost commented Nov 19, 2018

Description

Network connection status callback called with NSAPI_EVENT_CONNECTION_STATUS_CHANGE, NSAPI_STATUS_DISCONNECTED, if network registration status changes from a registered status to a unregistered status. Tested with unit tests.

Pull request type

[ X ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@ghost
Copy link
Author

ghost commented Nov 19, 2018

@jarvte Please review

@0xc0170 0xc0170 requested a review from a team November 19, 2018 11:02
@0xc0170
Copy link
Contributor

0xc0170 commented Nov 19, 2018

Please review and fix travis failures

jarvte
jarvte suggested changes Nov 19, 2018
View reviewed changes
features/cellular/framework/AT/AT_CellularNetwork.cpp Outdated
_reg_params._status = reg_params._status;
data.status_data = reg_params._status;
_connection_status_cb((nsapi_event_t)CellularRegistrationStatusChanged, (intptr_t)&data);
Copy link
Contributor

Choose a reason for hiding this comment

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

we should also send the original CellularRegistrationStatusChanged callback before disconnected

features/cellular/framework/AT/AT_CellularNetwork.cpp Outdated
reg_params._status == SearchingNetwork ||
reg_params._status == RegistrationDenied ||
reg_params._status == Unknown) {
if (!(_reg_params._status == StatusNotAvailable ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, if we drop to AttachedEmergencyOnly, RegisteredCSFBNotPreferredHome or to RegisteredCSFBNotPreferredRoaming are we still registered? if we drop to RegisteredSMSOnlyHome or RegisteredSMSOnlyRoaming we are registered but we probably can't send data. @AriParkkila @mirelachirica what do you think? I guess there is no right answer here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it is ok if any other state than RegisteredHomeNetwork or RegisteredRoaming indicates NSAPI_STATUS_DISCONNECTED, since data connection is impossible in those states.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 19, 2018

Please review travis failure

@0xc0170 0xc0170 mentioned this pull request Nov 24, 2018
Merged
@0xc0170
Copy link
Contributor

0xc0170 commented Nov 24, 2018

Info: This PR has been re-bundled into a new rollup PR (#8862).

No further work is needed here, as once that PR is merged, this PR will also be closed and marked as merged.
If any more commits are made in this PR, this PR will remain open and have to go through CI on its own.

0xc0170
0xc0170 reviewed Nov 24, 2018
View reviewed changes
UNITTESTS/stubs/ATHandler_stub.cpp
@@ -43,23 +43,34 @@ bool ATHandler_stub::bool_value = false;
uint8_t ATHandler_stub::uint8_value = 0;
FileHandle_stub *ATHandler_stub::fh_value = NULL;
device_err_t ATHandler_stub::device_err_value;
Callback<void()> ATHandler_stub::callback = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this breaks unittests, see failure here https://mbed-os.mbedcloudtesting.com/blue/organizations/jenkins/mbed-os-ci_unittests/detail/mbed-os-ci_unittests/1295/pipeline (internal link)

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 26, 2018

CI started

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 26, 2018

CI restarted (broken pipeline)

@cmonr
Copy link
Contributor

cmonr commented Nov 26, 2018 •
edited
Loading

Stopped and pushed to 5.11.1 for now.

Prioritizing for 5.11.0-rc1 for CI.

@cmonr
Copy link
Contributor

cmonr commented Nov 29, 2018

CI restarted.

@mbed-ci
Copy link

mbed-ci commented Nov 29, 2018

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 3
Build artifacts
Build logs

@cmonr cmonr mentioned this pull request Nov 30, 2018
Closed
@0xc0170
Copy link
Contributor

0xc0170 commented Nov 30, 2018

Not related failure, see referenced PR above. We will restart once it is fixed

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 30, 2018

client test restarted

@0xc0170 0xc0170 merged commit b9927e5 into ARMmbed:master Nov 30, 2018
@ghost ghost deleted the cellular_connect_status_fix branch December 5, 2018 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
release-version: 5.11.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@0xc0170 @cmonr @mbed-ci @mirelachirica @jarvte