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.

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

Cellular: Fix BG96 AT driver for IPv6 #11246

Merged
merged 1 commit into from
Aug 22, 2019
Merged

Cellular: Fix BG96 AT driver for IPv6 #11246

merged 1 commit into from
Aug 22, 2019

Conversation

AriParkkila
Copy link

Description

Fix BG96 AT driver to support IPv6 addressing.

Pull request type

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

Reviewers

@mirelachirica

Release Notes

@ciarmcom ciarmcom requested review from mirelachirica and a team August 16, 2019 13:00
@ciarmcom
Copy link
Member

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

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 19, 2019

CI started

mirelachirica
mirelachirica reviewed Aug 19, 2019
View reviewed changes
features/cellular/framework/targets/QUECTEL/BG96/QUECTEL_BG96.cpp Outdated
@@ -57,8 +57,8 @@ static const intptr_t cellular_properties[AT_CellularBase::PROPERTY_MAX] = {
1, // AT_CMGF
1, // AT_CSDH
1, // PROPERTY_IPV4_STACK
0, // PROPERTY_IPV6_STACK
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is causing DNS resolving problems due to the way _stack_type is currently used when BG96 socket is created.
Modem might get only IPv4 address eventhough the PDP context is IPV4V6 and because stack type is set as PDP type => _stack_type set as IPV4V6 => IP_address for the socket opening set to "0:0:0:0:0:0:0:1" => fails to send DNS requests.

_stack_type requires to be set according to the IP address(es) modem is assigned by network. IPV4 for only IPV4 address, IPV6 for only IPV6 address, IPV4V6 when both IPV4 and IPV6 address are assigned by network.

Some kind of fix is needed before we cand do these stack PROPERTY changes since they are breaking the DNS resolving?

Copy link
Contributor

Choose a reason for hiding this comment

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

If nothing else changes, at least (_stack_type == IPV6_STACK) ? "0:0:0:0:0:0:0:1":"127.0.0.1" might be better check than
(_stack_type == IPV4_STACK) ? "127.0.0.1" : "0:0:0:0:0:0:0:1".

Copy link
Contributor

Choose a reason for hiding this comment

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

Anything to address here? I set this to needs: review just in case

Copy link
Contributor

Choose a reason for hiding this comment

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

Once answered (all good?), we can merge

Copy link
Author

Choose a reason for hiding this comment

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

@mirelachirica Please re-review, no more property flag changes in this PR.

@mbed-ci
Copy link

mbed-ci commented Aug 19, 2019

Test run: SUCCESS

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

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 21, 2019

CI started

@adbridge
Copy link
Contributor

restarted

@mbed-ci
Copy link

mbed-ci commented Aug 21, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 merged commit 88dcf27 into ARMmbed:master Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kivaisan kivaisan kivaisan left review comments

@0xc0170 0xc0170 0xc0170 approved these changes

@mirelachirica mirelachirica mirelachirica approved these changes

@AnttiKauppila AnttiKauppila AnttiKauppila approved these changes

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

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