Skip to content

Cellular: Fixed network connect/disconnect #8192

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
Closed
wants to merge 4 commits into from
Closed

Cellular: Fixed network connect/disconnect #8192

wants to merge 4 commits into from

Conversation

@AriParkkila
Copy link

@AriParkkila AriParkkila commented Sep 20, 2018 •
edited
Loading

Description

Fixed cellular network connect and disconnect to handle PDP context more gracefully.

Commit 1: Fix Greentea

  • UDP test to call network.disconnect().

Commit 2: Fix ATHandler

  • To have mutex locks when modifying URC handlers
  • Add sync() AT stream that is needed for PPP disconnect

Commit 3: Add to Cellular network

  • Update network registration to have also deregistration
  • Add a new function to get active cellular context
  • Deactivate PDP context only if it was activated by us (also in PPP mode)

Commit 4: Change cellular statemachine

  • During registration check if PDP context is already useful and continue to attach

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[x] Functionality change
[ ] Breaking change

@AriParkkila
Copy link
Author

AriParkkila commented Sep 20, 2018

@mirelachirica @jarvte please review

@0xc0170 0xc0170 requested a review from a team September 20, 2018 09:28
jarvte
jarvte suggested changes Sep 20, 2018
View reviewed changes
features/cellular/framework/AT/AT_CellularNetwork.cpp Outdated
Copy link
Contributor

@jarvte jarvte Sep 20, 2018

Choose a reason for hiding this comment

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

You don't know if it's activated until you check the error from athandler

features/cellular/framework/AT/AT_CellularNetwork.cpp Outdated
Copy link
Contributor

@jarvte jarvte Sep 20, 2018

Choose a reason for hiding this comment

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

even if there is an error in ppp disconnect we should take control of filehandle to athandler and deactivate context.
Reasoning: ppp disconnect in any case releases filehandle

features/cellular/framework/AT/AT_CellularNetwork.cpp Outdated
Copy link
Contributor

@jarvte jarvte Sep 20, 2018

Choose a reason for hiding this comment

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

only deactive ... :)

features/cellular/framework/AT/AT_CellularNetwork.cpp Outdated
Copy link
Contributor

@jarvte jarvte Sep 20, 2018

Choose a reason for hiding this comment

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

not in line with disconnect methods where we call NSAPI_STATUS_DISCONNECTED only in case of success

features/cellular/framework/AT/AT_CellularNetwork.cpp Outdated
Copy link
Contributor

@jarvte jarvte Sep 20, 2018

Choose a reason for hiding this comment

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

Should we return nsapi_error_t e.g. : return _is_context_active ? NSAPI_ERROR_OK : NSAPI_ERROR_NO_CONNECTION;
and give cid as reference?

@AriParkkila
Copy link
Author

AriParkkila commented Sep 20, 2018

@jarvte please re-review

mirelachirica
mirelachirica reviewed Sep 21, 2018
View reviewed changes
features/cellular/framework/AT/AT_CellularNetwork.cpp Outdated
Copy link
Contributor

@mirelachirica mirelachirica Sep 21, 2018

Choose a reason for hiding this comment

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

This deregisters from network. Better be done in registration routine not in this detach from PS domain. Set_registration could have true/false param for register/deregister.

Copy link
Contributor

@0xc0170 0xc0170 Oct 1, 2018

Choose a reason for hiding this comment

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

@AriParkkila Any update required for this PR?

Copy link
Author

@AriParkkila AriParkkila Oct 1, 2018

Choose a reason for hiding this comment

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

@mirelachirica comment must still be handled, I will try to fix that soon.

@AriParkkila
Copy link
Author

AriParkkila commented Oct 2, 2018

@mirelachirica please review

@mirelachirica
Copy link
Contributor

mirelachirica commented Oct 3, 2018

@AriParkkila Can every bullet in the description, or at least some, be own commit?

@AriParkkila
Copy link
Author

AriParkkila commented Oct 4, 2018

@mirelachirica please review

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 5, 2018

Please review unittest failures

@AriParkkila
Copy link
Author

AriParkkila commented Oct 5, 2018

UnitTests can fixed after #8315 is merged.

@AriParkkila
Copy link
Author

AriParkkila commented Oct 15, 2018

The code base will change and these changes are coming in another PR.

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

+2 more reviewers

@mirelachirica mirelachirica mirelachirica approved these changes

@jarvte jarvte jarvte approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@AriParkkila @mirelachirica @0xc0170 @jarvte @studavekar