Skip to content

Cellular: AT + QICSGP set up APN fix #12227

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 2 commits into from
Jan 15, 2020
Merged

Cellular: AT + QICSGP set up APN fix #12227

merged 2 commits into from
Jan 15, 2020

Conversation

AnttiKauppila
Copy link

@AnttiKauppila AnttiKauppila commented Jan 9, 2020 •
edited by 0xc0170
Loading

Earlier we called AT+QICSGP only if the username and password was set.
It seems that we must call it also to set up APN while in AT mode.
This commit fixes the issue + updated IPv4/v6 handling to be correct in the same call

Summary of changes

Earlier we called AT+QICSGP only if the username and password was set.
It seems that we must call it also to set up APN while in AT mode.
This commit fixes the issue + updated IPv4/v6 handling to be correct in the same call

Fixes IOTCELL-2384

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 

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

Reviewers

Fixed IOTCELL-2384
2fb167b 
Earlier we called AT+QICSGP only if the username and password was set.
It seems that we must call it also to set up APN while in AT mode.
This commit fixes the issue + updated IPv4/v6 handling to be correct in the same call
@AnttiKauppila AnttiKauppila requested review from a team and bentcooke January 9, 2020 15:26
@ciarmcom
Copy link
Member

ciarmcom commented Jan 9, 2020

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

@0Grit
Copy link

0Grit commented Jan 9, 2020

@linlingao is this related to the issue we found for ME910?

AriParkkila
AriParkkila suggested changes Jan 10, 2020
View reviewed changes
features/cellular/framework/targets/QUECTEL/BG96/QUECTEL_BG96_CellularContext.cpp
@@ -69,11 +69,20 @@ ControlPlane_netif *QUECTEL_BG96_CellularContext::get_cp_netif()

nsapi_error_t QUECTEL_BG96_CellularContext::do_user_authentication()
Copy link

Choose a reason for hiding this comment

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

AT_CellularContext::get_context still uses AT+CGDCONT so that should be overridden as well.

Copy link
Contributor

@mirelachirica mirelachirica Jan 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

AT_CellularContext::get_context is using AT+CGDCONT only for reading. No need for QICSGP. If you think set_new_context(), that has to be done still by CGDCONT.
QICSGP can only congfigure a context created by CGDCONT.

Copy link

Choose a reason for hiding this comment

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

QICSGP must be called to configure APN. If don't want to override get_context then should allow empty _pwd and _uname here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this allowing empty _pwd and _uname?

@0xc0170 0xc0170 changed the title Fixed IOTCELL-2384 Cellular: AT + QICSGP set up APN fix Jan 10, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Jan 10, 2020

@AnttiKauppila I updated the title. Please review and edit it if needed.

@AnttiKauppila
Copy link
Author

I guess title is fine, I updated the PDP type handling according to 1.1 spec

@0xc0170 0xc0170 requested a review from AriParkkila January 10, 2020 15:16
@0xc0170
Copy link
Contributor

0xc0170 commented Jan 10, 2020

Ci run meanwhile

@mbed-ci
Copy link

mbed-ci commented Jan 10, 2020

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

AriParkkila
AriParkkila reviewed Jan 13, 2020
View reviewed changes
features/cellular/framework/targets/QUECTEL/BG96/QUECTEL_BG96_CellularContext.cpp
@@ -69,11 +69,21 @@ ControlPlane_netif *QUECTEL_BG96_CellularContext::get_cp_netif()

nsapi_error_t QUECTEL_BG96_CellularContext::do_user_authentication()
{
uint8_t type = (uint8_t)_pdp_type;
if ((uint8_t)_pdp_type < 1) {
type = 1;
Copy link

Choose a reason for hiding this comment

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

This should probably default to IPv4v6?

@linlingao
Copy link
Contributor

@loverdeg-ep This issue doesn't seem to be related to what we saw on ME910. This issue fixes the case when username/password is not present. The ME910 issue has to do with passing username/password.

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 14, 2020

AriParkkila reviewed on behalf of ARMmbed/mbed-os-wan yesterday

Is this waiting for an answer? I marked this as ready for integration (once the release 6.0.0alpha1 is ready, it can be merged).

AriParkkila
AriParkkila approved these changes Jan 14, 2020
View reviewed changes
Copy link

@AriParkkila AriParkkila left a comment

Choose a reason for hiding this comment

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

Type should probably default to IPv4v6?

@0xc0170 0xc0170 added release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0 and removed ready for merge labels Jan 15, 2020
@0xc0170 0xc0170 merged commit c53f4d2 into ARMmbed:master Jan 15, 2020
@jeromecoutant jeromecoutant mentioned this pull request Feb 6, 2020
Closed
@AnttiKauppila AnttiKauppila deleted the BG96_fix branch February 6, 2020 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mirelachirica mirelachirica mirelachirica left review comments

@0xc0170 0xc0170 0xc0170 approved these changes

@AriParkkila AriParkkila AriParkkila approved these changes

@bentcooke bentcooke Awaiting requested review from bentcooke

Assignees
No one assigned
Labels
release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@AnttiKauppila @ciarmcom @0Grit @0xc0170 @mbed-ci @linlingao @AriParkkila @mirelachirica