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

Add exception case for IPV6 only mode of BG96. #10473

Merged
merged 1 commit into from May 23, 2019

Conversation

Projects
None yet
8 participants
@DanielDmlee
Copy link
Contributor

commented Apr 25, 2019

Need be IPV6 address when use IPV6 only mode.

Signed-off-by: Daniel Lee daniel.lee2@arm.com

Description

Need be IPV6 address when using IPV6 only mode. Therefore added IPV6 address instead of IPV4 address when using UDP socket for getting DNS.

Tested via SK Telecom in Korea.

Pull request type

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

Reviewers

Release Notes

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Apr 25, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

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

0, // PROPERTY_IPV6_STACK
#endif
0, // PROPERTY_IPV4V6_STACK

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Apr 25, 2019

Contributor

Looking at the Quectel BG96 data sheet, PDP type can also be “IPV4V6”. So that property should be turned on as well.

@@ -42,8 +42,16 @@ static const intptr_t cellular_properties[AT_CellularBase::PROPERTY_MAX] = {
1, // AT_CSMP
1, // AT_CMGF
1, // AT_CSDH
#if MBED_CONF_LWIP_IPV4_ENABLED

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Apr 25, 2019

Contributor

Adding LWIP specific compile time flags for generic driver is not suitable. Same constructor is used whether you use LWIP with PPP or drive the modem in AT mode.

@@ -183,13 +183,17 @@ nsapi_error_t QUECTEL_BG96_CellularStack::create_socket_impl(CellularSocket *soc
int request_connect_id = socket->id;
int remote_port = 0;
int err = -1;

#if MBED_CONF_LWIP_IPV6_ENABLED & !MBED_CONF_LWIP_IPV4_ENABLED

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Apr 25, 2019

Contributor

Same thing here. This class is not used when LWIP-PPP is in use.

@hasnainvirk

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

@DanielDmlee On a general note, I can see what your intention is. The property setup is obviously not correct as the modem supports both IPv4 and IPv6 and simultaneous IPv4-IPv6 as well. So, yeah your PR is good to turn those features on.
Looking at AT_CellularContext::set_new_context(...), I can see that the function defaults to IPv4. And then there is a priority order, Non-IP, Both IPv4 and IPv6, IPv6 and finally IPv4. I don't think this is the right approach. The rationale is evident from your attempt. Modem can support all properties, I mean non-IP and any flavour of IP. We shouldn't be prejudicing here.
A better static solution would be to add framework level config options (not dependent upon LWIP) that set which PDP type to pick based on both the option and cellular properties and make a decision at compile time. If the config option and the cellular properties do not match, compiler throws error. Ideal solution would be pick up the required PDP type at run time.

@DanielDmlee

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

The button was pressed incorrectly. @hasnainvirk Thanks for the comments, I'll think about your comment.

@DanielDmlee DanielDmlee reopened this Apr 29, 2019

@0xc0170 0xc0170 added needs: work and removed needs: review labels Apr 29, 2019

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 29, 2019

Test run: SUCCESS

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

@DanielDmlee DanielDmlee force-pushed the DanielDmlee:BG96_IPV6_only branch from 49e4075 to 78a6c13 Apr 30, 2019

@DanielDmlee

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@DanielDmlee On a general note, I can see what your intention is. The property setup is obviously not correct as the modem supports both IPv4 and IPv6 and simultaneous IPv4-IPv6 as well. So, yeah your PR is good to turn those features on.
Looking at AT_CellularContext::set_new_context(...), I can see that the function defaults to IPv4. And then there is a priority order, Non-IP, Both IPv4 and IPv6, IPv6 and finally IPv4. I don't think this is the right approach. The rationale is evident from your attempt. Modem can support all properties, I mean non-IP and any flavour of IP. We shouldn't be prejudicing here.
A better static solution would be to add framework level config options (not dependent upon LWIP) that set which PDP type to pick based on both the option and cellular properties and make a decision at compile time. If the config option and the cellular properties do not match, compiler throws error. Ideal solution would be pick up the required PDP type at run time.

@hasnainvirk You're right. I thought easily. I modified the code following your guide. Could you review code again? Below is tested log.

[MAIN], plmn: NULL
Establishing connection
[00000001ms][INFO][CELL]: New CellularContext  (0x20003ce0)
[00000001ms][INFO][CELL]: CellularContext plmn NULL
[00000002ms][INFO][CELL]: CellularContext connect
[00000006ms][INFO][CELL]: Start connecting (timeout 1 s)
[00000016ms][DBG ][CELL]: Init => Device ready
[00000018ms][DBG ][CELL]: AT flush
[00000030ms][INFO][CELL]: Modem ready
[00000030ms][DBG ][CELL]: callback: 4096, err: 0, data: -1
[00000033ms][DBG ][CELL]: Device ready => SIM pin
[00000043ms][INFO][CELL]: RSSI -81 dBm
[00000043ms][INFO][CELL]: Setup SIM (timeout 1 s)
[00000049ms][DBG ][CELL]: AT flush
[00000057ms][INFO][CELL]: SIM is ready
[00000058ms][DBG ][CELL]: callback: 4097, err: 0, data: 0
[00000077ms][DBG ][CELL]: Found active context
[00000081ms][DBG ][CELL]: Active context found.
[00000085ms][DBG ][CELL]: Cellular already attached.
[00000088ms][DBG ][CELL]: SIM pin => Registering network
[00000092ms][INFO][CELL]: RSSI -81 dBm
[00000092ms][INFO][CELL]: Network registration (timeout 180 s)
[00000104ms][DBG ][CELL]: +CEREG: RegisteredHomeNetwork, LAC 10035, cell 233771, CATM1
[00000108ms][DBG ][CELL]: callback: 4098, err: 0, data: 1
[00000114ms][INFO][CELL]: Registering network => Attaching network
[00000125ms][INFO][CELL]: RSSI -81 dBm
[00000125ms][INFO][CELL]: Attaching network (timeout 60 s)
[00000136ms][DBG ][CELL]: callback: 4102, err: 0, data: 1
[00000204ms][INFO][CELL]: Found PDP context 7
[00000208ms][DBG ][CELL]: Found active context

Connection Established.
[00000212ms][INFO][CELL]: Socket 0 open
[00000222ms][INFO][CELL]: Socket 1 open
[00000276ms][INFO][CELL]: Socket 1 sent 43 bytes to 8.8.8.8 port 53
[00000454ms][DBG ][CELL]: AT OoB readable 1, len 0
[00000457ms][DBG ][CELL]: AT OoB done
[00000470ms][INFO][CELL]: Socket 1 recv 71 bytes from 64:ff9b::808:808 port 53
[00000475ms][INFO][CELL]: Socket 1 closed
TCP: connected with echo.mbedcloudtesting.com server
[00000855ms][INFO][CELL]: Socket 0 sent 4 bytes to 2a05:d018:21f:3800:3164:2a5c:75b3:970b port 7
TCP: Sent 4 Bytes to echo.mbedcloudtesting.com
[00001230ms][DBG ][CELL]: AT OoB readable 1, len 0
[00001232ms][DBG ][CELL]: AT OoB done
[00001237ms][INFO][CELL]: Socket 0 recv 4 bytes
[00011242ms][INFO][CELL]: Socket 0 closed
Received from echo server 4 Bytes
[00011242ms][INFO][CELL]: CellularContext disconnect()
[00011243ms][DBG ][CELL]: callback: 0, ptr: 2
[00011243ms][INFO][CELL]: cb: CellularContext disconnected


Success. Exiting

@adbridge adbridge added needs: review and removed needs: work labels May 1, 2019

@adbridge

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

@hasnainvirk Could you re-review please?

@AnttiKauppila

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

@AriParkkila @mirelachirica Could you review this?

@@ -218,6 +216,39 @@ nsapi_error_t QUECTEL_BG96_CellularStack::create_socket_impl(CellularSocket *soc

handle_open_socket_response(modem_connect_id, err);
}
if (_stack_type == IPV6_STACK || _stack_type == IPV4V6_STACK) {

This comment has been minimized.

Copy link
@mirelachirica

mirelachirica May 17, 2019

Contributor

Could you add this check only when IP address is feed to AT command and not copy the whole block?
Around lines 192, 211: _at.write_string("127.0.0.1")?

This comment has been minimized.

Copy link
@DanielDmlee

DanielDmlee May 21, 2019

Author Contributor

Hi @mirelachirica, Thank you for comment. What is meaning 'when IP address is feed to AT command and not copy the whole block?' Could you explain more detail?

This comment has been minimized.

Copy link
@mirelachirica

mirelachirica May 21, 2019

Contributor

I mean to add checks for lines 192 and 211. Something like:

if (_stack_type == IPV4_STACK) {
_at.write_string("127.0.0.1");
} else if (_stack_type == IPV6_STACK || _stack_type == IPV4V6_STACK) {
_at.write_string("0:0:0:0:0:0:0:1");
}

This way you do not need to copy the whole block of code starting with QIOPEN, lines 188-218 to lines: 220-251.

This comment has been minimized.

Copy link
@DanielDmlee

DanielDmlee May 21, 2019

Author Contributor

Oh, that's a good way. Why did not I think about it? I'll check again, thanks.!

@mirelachirica

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

What about setting following properties:
0, // PROPERTY_IPV6_STACK
0, // PROPERTY_IPV4V6_STACK
in BG96's cellular_properties array? They are not part of the changes anymore.

@DanielDmlee

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

What about setting following properties:
0, // PROPERTY_IPV6_STACK
0, // PROPERTY_IPV4V6_STACK
in BG96's cellular_properties array? They are not part of the changes anymore.

Hi @mirelachirica, Actually, I didn't care about this setting when I communication tested via BG96. Because when I set the IPV6 address with AT + QIOPEN =, the communication was a success.

@mirelachirica

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

Hi @mirelachirica, Actually, I didn't care about this setting when I communication tested via BG96. Because when I set the IPV6 address with AT + QIOPEN =, the communication was a success.

In this case we can leave the property settings out of this PR scope.

Add a check step of IPv6 network to BG96
Need to check IPv6 address when use IPv6 network via BG96.

Signed-off-by: Daniel Lee <daniel.lee2@arm.com>

@DanielDmlee DanielDmlee force-pushed the DanielDmlee:BG96_IPV6_only branch from 71d2970 to 26c3bcf May 22, 2019

@DanielDmlee

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

Hi @mirelachirica updated the code. Could you review again?

@0xc0170 0xc0170 requested a review from hasnainvirk May 23, 2019

@0xc0170 0xc0170 added needs: CI and removed needs: review labels May 23, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 23, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented May 23, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels May 23, 2019

@0xc0170 0xc0170 merged commit 51b835b into ARMmbed:master May 23, 2019

26 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-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage Success
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8679 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
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.