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

application: serial_lte_modem: Automatic LTE connection #12849

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

junqingzou
Copy link
Contributor

@junqingzou junqingzou commented Oct 25, 2023

Move the lte auto-connect from SLM LwM2M client to full SLM scope.
Support lte auto-connnect by new Kconfig CONFIG_SLM_AUTO_CONNECT.
Default behavior is no automatic LTE connection, same as before.

When auto-connect is enabled, need to customize network-specific
PDN configuration in code. Default is Non-IP in NB-IoT for NIDD.

Impact on LwM2M client:
.Operation "auto_connect" is removed from LwM2M command.
.No more dynamic config of auto_connect (and auto_register).
.CONFIG_SLM_AUTO_CONNECT is enabled by default in overlay.

Change CONFIG_SLM_CUSTOMIZED to CONFIG_SLM_CUSTOMER_VERSION to
allow reporting customer version of SLM after customization.

Remove unused CONFIG_SLM_AT_MODE and CONFIG_SLM_SOCKET_RX_MAX.

@junqingzou junqingzou added the DNM label Oct 25, 2023
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 25, 2023
@junqingzou
Copy link
Contributor Author

junqingzou commented Oct 25, 2023

DNM: not for NCS v2.5.0, no review needed as of now. Doc changes come later

@NordicBuilder
Copy link
Contributor

NordicBuilder commented Oct 25, 2023

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite
test-fw-nrfconnect-nrf-iot_serial_lte_modem X

Detailed information of selected test modules

Note: This message is automatically posted and updated by the CI

@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

Copy link
Contributor

@tomi-font tomi-font left a comment

Choose a reason for hiding this comment

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

Request review once ready for review.

@github-actions github-actions bot added doc-required PR must not be merged without tech writer approval. and removed changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Nov 10, 2023
@junqingzou junqingzou added changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. and removed DNM labels Nov 10, 2023
@github-actions github-actions bot removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Nov 10, 2023
Copy link
Contributor

@tomi-font tomi-font left a comment

Choose a reason for hiding this comment

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

It seems that lte_auto_connect() is not quite ready.

applications/serial_lte_modem/Kconfig Outdated Show resolved Hide resolved
applications/serial_lte_modem/prj.conf Outdated Show resolved Hide resolved
applications/serial_lte_modem/Kconfig Outdated Show resolved Hide resolved
applications/serial_lte_modem/Kconfig Outdated Show resolved Hide resolved
applications/serial_lte_modem/doc/slm_description.rst Outdated Show resolved Hide resolved
applications/serial_lte_modem/prj.conf Outdated Show resolved Hide resolved
applications/serial_lte_modem/prj.conf Outdated Show resolved Hide resolved
applications/serial_lte_modem/src/main.c Outdated Show resolved Hide resolved
applications/serial_lte_modem/src/main.c Outdated Show resolved Hide resolved
@tomi-font
Copy link
Contributor

tomi-font commented Nov 10, 2023

Also, you are introducing different changes (auto-connect, custom version string) in a single commit whose title doesn't (and cannot) reflect that. I think it would be good to split it up.

@junqingzou
Copy link
Contributor Author

Also, you are introducing different changes (auto-connect, custom version string) in a single commit whose title doesn't (and cannot) reflect that. I think it would be good to split it up.

Customer version string has direct relationship with different default network configuration from this PR. The version string change actually should have been introduced before, as in the market SLM is not always used out of box. Customers DO customize SLM to meet their own need, e.g. integrating soft sim, etc, which means they need version control as well. The customer version string is also required for APP FOTA (to update SLM). For example, current work with LwM2M Carrier requires a version that could reflect the update of lib_lwm2m_carrier.

@junqingzou
Copy link
Contributor Author

CI seems to have failed due to TimeoutError: AT#XCARRIER="auto_connect","read" timed out waiting for OK

I can remove that part of the test case, so CI can pass. There is a question of whether this is used often enough without LWM2M that we should alter our current test cases to test the combination?

The auto-connect is not just for LwM2M over NIDD, but also for
.LwM2M over IP
.Non-LwM2M SLM client in LTE-M, NB-IoT even NIDD
Idea is to put all requried network configuiration in "slm_auto_connect.h".
What I tested (all my SIMs allow):
.LwM2M over NIDD
.Normal LTE-M requires APN and user authentication
.Normal NB-IoT requires APN and user authentication
.Normal LTE-M requires APN only

applications/serial_lte_modem/Kconfig Outdated Show resolved Hide resolved
applications/serial_lte_modem/doc/Generic_AT_commands.rst Outdated Show resolved Hide resolved
applications/serial_lte_modem/doc/Generic_AT_commands.rst Outdated Show resolved Hide resolved
applications/serial_lte_modem/src/slm_net_cfg.h Outdated Show resolved Hide resolved
applications/serial_lte_modem/src/slm_net_cfg.h Outdated Show resolved Hide resolved
applications/serial_lte_modem/src/main.c Outdated Show resolved Hide resolved
applications/serial_lte_modem/src/main.c Outdated Show resolved Hide resolved
applications/serial_lte_modem/src/slm_net_cfg.h Outdated Show resolved Hide resolved
applications/serial_lte_modem/prj.conf Outdated Show resolved Hide resolved
applications/serial_lte_modem/src/main.c Outdated Show resolved Hide resolved
applications/serial_lte_modem/src/main.c Outdated Show resolved Hide resolved
applications/serial_lte_modem/src/slm_auto_connect.h Outdated Show resolved Hide resolved
applications/serial_lte_modem/src/main.c Outdated Show resolved Hide resolved
applications/serial_lte_modem/src/main.c Outdated Show resolved Hide resolved
applications/serial_lte_modem/src/main.c Outdated Show resolved Hide resolved
Copy link
Contributor

@tomi-font tomi-font left a comment

Choose a reason for hiding this comment

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

Just a few bits to address left from my perspective (including some unaddressed previous comment of mine).

applications/serial_lte_modem/doc/slm_description.rst Outdated Show resolved Hide resolved
applications/serial_lte_modem/Kconfig Outdated Show resolved Hide resolved
@junqingzou junqingzou force-pushed the pr_slm_auto_connect branch 2 times, most recently from d5e91cc to 0fd2d8b Compare November 15, 2023 01:21
Copy link
Contributor

@kacperradoszewski kacperradoszewski left a comment

Choose a reason for hiding this comment

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

Other than a few nits, looks good to me

int lte_preference; /* 0 ~ 4 */
/* Refer to AT command manual of +CGDCONT and +CGAUTH for PDN configuration */
bool pdp_config; /* PDP context definition required or not */
char *pdn_fam; /* PDP type: "Non-IP"; ""IP", "IPV6", "IPV4V6", "Non-IP" */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
char *pdn_fam; /* PDP type: "Non-IP"; ""IP", "IPV6", "IPV4V6", "Non-IP" */
char *pdn_fam; /* PDP type: "IP", "IPV6", "IPV4V6", "Non-IP" */

0, /* LTE preference */
/* Network-specific default PDN configured by +CGDCONT and +CGAUTH (refer to AT command manual) */
false, /* PDP context definition required or not */
"", /* PDP type: ""IP", "IPV6", "IPV4V6", "Non-IP" */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"", /* PDP type: ""IP", "IPV6", "IPV4V6", "Non-IP" */
"", /* PDP type: "IP", "IPV6", "IPV4V6", "Non-IP" */

* ``#XSLMVER`` AT command to report CONFIG_SLM_CUSTOMER_VERSION if it is defined

* Removed:
* The ``CONFIG_SLM_CUSTOMIZED`` Kconfig option due it to no longer being used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* The ``CONFIG_SLM_CUSTOMIZED`` Kconfig option due it to no longer being used.
* The ``CONFIG_SLM_CUSTOMIZED`` Kconfig option due to it to no longer being used.

Copy link
Contributor

Choose a reason for hiding this comment

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

That one's on me, and should actually be:

Suggested change
* The ``CONFIG_SLM_CUSTOMIZED`` Kconfig option due it to no longer being used.
* The ``CONFIG_SLM_CUSTOMIZED`` Kconfig option due to it no longer being used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I almost got it right 😄


* Updated:

* ``#XMQTTCON`` AT command to exclude MQTT client ID from the parameter list.
* ``#XSLMVER`` AT command to report CONFIG_SLM_CUSTOMER_VERSION if it is defined
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* ``#XSLMVER`` AT command to report CONFIG_SLM_CUSTOMER_VERSION if it is defined
* ``#XSLMVER`` AT command to report CONFIG_SLM_CUSTOMER_VERSION if it is defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

Comment on lines 30 to 32
#if defined(CONFIG_SLM_CARRIER)
#include "slm_at_carrier.h"
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#if defined(CONFIG_SLM_CARRIER)
#include "slm_at_carrier.h"
#endif

No longer needed I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes as LwM2M not VIP any more

0, /* LTE preference */
/* Network-specific default PDN configured by +CGDCONT and +CGAUTH (refer to AT command manual) */
true, /* PDP context definition required or not */
"Non-IP", /* PDP type: ""IP", "IPV6", "IPV4V6", "Non-IP" */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Non-IP", /* PDP type: ""IP", "IPV6", "IPV4V6", "Non-IP" */
"Non-IP", /* PDP type: "IP", "IPV6", "IPV4V6", "Non-IP" */

Move the lte auto-connect from SLM LwM2M client to full SLM scope.
Support lte auto-connnect by new Kconfig CONFIG_SLM_AUTO_CONNECT.
Default behavior is no automatic LTE connection, same as before.

When auto-connect is enabled, need to customize network-specific
PDN configuration in code. Default is Non-IP in NB-IoT for NIDD.

Impact on LwM2M client:
.Operation "auto_connect" is removed from LwM2M command.
.No more dynamic config of auto_connect (and auto_register).
.CONFIG_SLM_AUTO_CONNECT is enabled by default in overlay.

Change CONFIG_SLM_CUSTOMIZED to CONFIG_SLM_CUSTOMER_VERSION to
allow reporting customer version of SLM after customization.

Remove unused CONFIG_SLM_AT_MODE and CONFIG_SLM_SOCKET_RX_MAX.

Signed-off-by: Jun Qing Zou <[email protected]>
@junqingzou
Copy link
Contributor Author

Thanks for all the comments.
I'm asking for merging now.

@nordicjm nordicjm merged commit 710acdb into nrfconnect:main Nov 15, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-required PR must not be merged without tech writer approval.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants