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

Fix #13490 Cellular disconnect does not deactivate context #13610

Merged
merged 3 commits into from
Oct 13, 2020

Conversation

boraozgen
Copy link
Contributor

Summary of changes

Fixes #13490, where the PDP context does not get deactivated in some cases when CellularInterface::disconnect() is called. Further description is in the issue.

Impact of changes

Migration actions required

Documentation

None


Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[X] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers


@mergify mergify bot added the needs: work label Sep 15, 2020
@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Sep 15, 2020
@ciarmcom
Copy link
Member

@boraozgen, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

@ciarmcom ciarmcom requested a review from a team September 15, 2020 08:00
@0xc0170 0xc0170 requested a review from a team September 21, 2020 12:54
}

deactivate_context();
Copy link

@AriParkkila AriParkkila Sep 22, 2020

Choose a reason for hiding this comment

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

The comment above For EPS, if an attempt is made to disconnect the last PDN connection, then the MT responds with ERROR... comes from 3GPP TS 27.007.

Unlike EPS, NB-IoT can be without PDN so could be fixed above like if (_is_context_active && (rat < CellularNetwork::RAT_E_UTRAN || rat == CellularNetwork::RAT_NB1 || active_contexts_count > 1)) {.

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 this. Does this also apply to CAT-M1?

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 23, 2020

Thanks @AriParkkila for review

@mergify mergify bot added needs: CI and removed needs: review labels Sep 30, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Oct 13, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Oct 13, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-ARM ✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest ✔️

@0xc0170 0xc0170 merged commit 0548981 into ARMmbed:master Oct 13, 2020
@mergify mergify bot removed the ready for merge label Oct 13, 2020
@mbedmain mbedmain added release-version: 6.4.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Oct 20, 2020
@ocecelja
Copy link

Added this. Does this also apply to CAT-M1?

Good question. According to changes it doesn't. But I'm pretty sure it should because CAT M1 and NB1 have similar working concepts from fw point of view, both needing contexts.

So maybe instead of:
if (_is_context_active && (rat < CellularNetwork::RAT_E_UTRAN || rat == CellularNetwork::RAT_NB1 || active_contexts_count > 1)) {
use:
if (_is_context_active && (rat < CellularNetwork::RAT_UNKNOWN || active_contexts_count > 1)) {

@boraozgen
Copy link
Contributor Author

All RATs satisfy the condition rat < CellularNetwork::RAT_UNKNOWN. What do you mean by that?

@ocecelja
Copy link

ocecelja commented Oct 28, 2020

As do for rat < CellularNetwork::RAT_E_UTRAN || rat == CellularNetwork::RAT_NB1 except CellularNetwork::RAT_CATM1. What I propose is to include RAT_CATM1.

Question is why did you left out RAT_CATM1?

@boraozgen
Copy link
Contributor Author

I am not 100% sure but I think this issue was not affecting CAT-M1. I will try to test CAT-M1 with this fix and comment here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cellular: disconnect does not deactivate context (for some cases)
7 participants