Skip to content

Cellular: more gracefully disconnect. #8772

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
Merged
merged 1 commit into from
Nov 25, 2018
Merged

Cellular: more gracefully disconnect. #8772

merged 1 commit into from
Nov 25, 2018

Conversation

jarvte
Copy link
Contributor

@jarvte jarvte commented Nov 16, 2018

Description

  • disconnect now will also detach from the network on disconnect().
  • disconnect will deactivate PDP Context in PPP mode also
  • NSAPI_STATUS_DISCONNECTED is always called to callback function in disconnect

@mirelachirica @AriParkkila please review

Pull request type

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

@0xc0170 0xc0170 requested a review from a team November 16, 2018 13:28
@jarvte jarvte force-pushed the gracefully_disconnect branch from 320f41c to 168b9ca Compare November 19, 2018 10:07
@jarvte
Copy link
Contributor Author

jarvte commented Nov 19, 2018

rebased as there was checkout failure in travis

@jarvte jarvte force-pushed the gracefully_disconnect branch from 168b9ca to e0290cd Compare November 19, 2018 10:50
AriParkkila
AriParkkila suggested changes Nov 19, 2018
View reviewed changes
features/cellular/framework/AT/AT_CellularContext.cpp
}

_is_connected = false;
call_network_cb(NSAPI_STATUS_DISCONNECTED);
Copy link

Choose a reason for hiding this comment

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

I guess disconnected event should be handled by +CGEV, when really disconnected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reasons for sending NSAPI_STATUS_DISCONNECTED here:

  • +CGEV is unreliable
  • synchronous disconnect call in PPP mode will also send NSAPI_STATUS_DISCONNECTED before returning so it's on line with PPP now.

features/cellular/framework/AT/AT_CellularContext.cpp
_at.cmd_stop_read_resp();
}
_at.clear_error();
_at.cmd_start("AT+CGATT=0");
Copy link

Choose a reason for hiding this comment

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

I guess CellularNetwork::detach should be updated to detach from network.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added register to CellularNetwork::detach

features/cellular/framework/AT/AT_CellularContext.cpp Outdated
_at.unlock();
#endif // NSAPI_PPP_AVAILABLE
_at.lock();

// deactivate a context only if we have activated
if (_is_context_activated) {
// CGACT and CGATT commands might take up to 3 minutes to respond.
_at.set_at_timeout(180 * 1000, false);
Copy link

Choose a reason for hiding this comment

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

false is default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, removed extra false

features/cellular/framework/AT/ATHandler.cpp Outdated
@@ -150,7 +148,10 @@ FileHandle *ATHandler::get_file_handle()

void ATHandler::set_file_handle(FileHandle *fh)
{
_fh_sigio_set = false;
Copy link

Choose a reason for hiding this comment

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

Why to protect setting fileHandle, maybe this flag could be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, removed.

features/cellular/framework/AT/AT_CellularContext.cpp
@@ -659,17 +665,27 @@ nsapi_error_t AT_CellularContext::disconnect()
// 3GPP TS 27.007:
// For EPS, if an attempt is made to disconnect the last PDN connection, then the MT responds with ERROR
if (_is_context_active && (rat < CellularNetwork::RAT_E_UTRAN || active_contexts_count > 1)) {
_at.clear_error();
Copy link

Choose a reason for hiding this comment

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

Return value is actually meaningless due to clear_error calls?

Copy link
Contributor Author

@jarvte jarvte Nov 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

almost, CGACT can still fail. CGACT is more powerful than commands above. So we try to do thing properly but the last CGACT will finally detach from the network and then it should not matter that the above commands have failed.

features/cellular/framework/AT/ATHandler.cpp Outdated
@@ -1207,3 +1208,26 @@ void ATHandler::debug_print(char *p, int len)
}
#endif // MBED_CONF_CELLULAR_DEBUG_AT
}

bool ATHandler::sync()
Copy link

Choose a reason for hiding this comment

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

Could take time as parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added parameter

@jarvte jarvte force-pushed the gracefully_disconnect branch from e0290cd to a655a53 Compare November 19, 2018 14:02
@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 merged commit a655a53 into ARMmbed:master Nov 25, 2018
@0xc0170 0xc0170 removed the needs: CI label Nov 25, 2018
@cmonr cmonr removed the rollup PR label Nov 26, 2018
@jarvte jarvte deleted the gracefully_disconnect branch December 3, 2018 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@AriParkkila AriParkkila AriParkkila approved these changes

@mirelachirica mirelachirica mirelachirica approved these changes

Assignees
No one assigned
Labels
release-version: 5.11.0-rc1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jarvte @0xc0170 @AriParkkila @mirelachirica @cmonr