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.

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

Cellular: retry logic for CellularContext connect #10056

Merged
merged 1 commit into from Apr 1, 2019

Conversation

Projects
None yet
10 participants
@jarvte
Copy link
Contributor

commented Mar 12, 2019

Description

State machine has retry logic until device is attached to network.
After this CellularContext does the context activation e.g. connect.
There was no retry logic for context activation. Added logic to
CellularContext level so it's available for at and (upcoming) ril layers.

Pull request type

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

Reviewers

@AriParkkila

Release Notes

Add retry logic for CellularContext connect last phase: activating pdp context / open ppp channel.
Retry logic is the same as in CellularStateMachine.

@ciarmcom ciarmcom requested review from AriParkkila and ARMmbed/mbed-os-maintainers Mar 12, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Added retry logic for CellularContext connect last phase: activating pdp context / open ppp channel.

Is this still considered a fix rather than adding new functionality (new method to cellularcontext) as result it will fix as well.

features/cellular/framework/API/CellularContext.h Outdated
*/
void call_network_cb(nsapi_connection_status_t status);

virtual void do_connect() = 0;

This comment has been minimized.

Copy link
@AnttiKauppila

AnttiKauppila Mar 13, 2019

Contributor

This is an API break!

This comment has been minimized.

Copy link
@jarvte

jarvte Mar 13, 2019

Author Contributor

These are protected methods and already implemented in AT_CellularContext class (exepct do_connect_with_retry). This was done so that RIL and AT layers don't have copy-paste code. So is this kind of change also prohibited?

This comment has been minimized.

Copy link
@AnttiKauppila

AnttiKauppila Mar 13, 2019

Contributor

Pure virtual will cause code changes, while a function with a default implementation will not.

This comment has been minimized.

Copy link
@jarvte

jarvte Mar 13, 2019

Author Contributor

Ok, if I change this to not pure virtual but virtual it's accepted?

This comment has been minimized.

Copy link
@jarvte

jarvte Mar 13, 2019

Author Contributor

Agreed fix to virtual method with default implementation

This comment has been minimized.

Copy link
@jarvte

jarvte Mar 13, 2019

Author Contributor

pushed fix. Now there is a default implementation in place.

@AnttiKauppila
Copy link
Contributor

left a comment

Why a new API break introduced?

@jarvte

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

Why a new API break introduced?

see my response to your other comment

@SeppoTakalo
Copy link
Contributor

left a comment

These are internal APIs.
I have no objection from IP Core point of view. Needs Cellular tech-lead to approve.

@@ -106,6 +112,11 @@ void AT_CellularContext::enable_hup(bool enable)
}
}

void AT_CellularContext::do_connect_with_retry()
{
CellularContext::do_connect_with_retry();

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Mar 13, 2019

Contributor

Why do you override if you just directly call the base?

This comment has been minimized.

Copy link
@jarvte

jarvte Mar 13, 2019

Author Contributor

It's because of EventQueue. In non blocking mode we use EventQueue to call do_connect_with_retry. We can't call CellularContext::do_connect_with_retry() as it's protected so to avoid that problem we had to introduce this function.

@jarvte jarvte force-pushed the jarvte:context_act_ppp_retry_logic branch Mar 13, 2019

@jarvte

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

Added retry logic for CellularContext connect last phase: activating pdp context / open ppp channel.

Is this still considered a fix rather than adding new functionality (new method to cellularcontext) as result it will fix as well.

New method is a protected method but I'm fine with fix or functionality change. I really don't know which would suite better.

@jarvte jarvte force-pushed the jarvte:context_act_ppp_retry_logic branch to 8e58962 Mar 14, 2019

@jarvte

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

Force pushed doxygen updates.

@jarvte

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

ping

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

@ARMmbed/mbed-os-wan Please re-review.

@cmonr

cmonr approved these changes Mar 27, 2019

@jarvte jarvte force-pushed the jarvte:context_act_ppp_retry_logic branch from 8e58962 to b4b7128 Mar 29, 2019

@jarvte

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Force pushed rebase with master to fix conflicts.

Cellular: retry logic for CellularContext connect
State machine has retry logic until device is attached to network.
After this CellularContext does the context activation e.g. connect.
There was no retry logic for context activation. Added logic to
CellularContext level so it's available for at and (upcoming)ril layers.
@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Info: This PR has now been rebased.

If this was made in error, feel free to force-push your local branch to restore the PR.

@cmonr cmonr force-pushed the jarvte:context_act_ppp_retry_logic branch from b4b7128 to c6e5595 Mar 29, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

CI started

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

@jarvte Rebased your PR since a seperate PR fixed the astyle issues.

@cmonr cmonr added the needs: CI label Mar 29, 2019

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 29, 2019

Test run: FAILED

Summary: 2 of 13 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test
  • jenkins-ci/mbed-os-ci_exporter
@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

Failures seem to be CI K66 issue +license issue. restarting CI

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 31, 2019

Test run: FAILED

Summary: 1 of 13 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test
@jarvte

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

Still K66F failing, not related to this pr.

@alekla01

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

restarted jenkins-ci/mbed-os-ci_greentea-test

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 1, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 3
Build artifacts

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Apr 1, 2019

@cmonr cmonr merged commit cf4118f into ARMmbed:master Apr 1, 2019

28 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9080 cycles (-17 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Apr 1, 2019

@jarvte jarvte deleted the jarvte:context_act_ppp_retry_logic branch Apr 5, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

@jarvte Need help with this fix. Patching this causing multiple conflicts that are not that easy to resolve - too many changes on master landing to these files? Need a branch against release-candidate branch (already having 5.12.1rc ready, just waiting for this one to land).

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

Or will go to 5.12.2 with new PR targeting 5.12 branch

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

Moving to 5.12.2 (if wont be manually patched for 5.12.1).

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

Moving to 5.13.0-rc1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.