lib: lte_link_control: refactor into modules #17266

MirkoCovizzi · 2024-09-11T08:56:30Z

Divides the LTE Link Controller into modules.

Tasks:

Modularize: make the architecture modular (rather than monolithic) and easy to understand. Streamline the introduction of new AT commands/functionalities. I have a couple of AT commands I need to add so this is important
Identify potential configuration problems that might arise from lte_net_if stack, lte_lc API usage within NCS, or custom overlays

Benefits:

Improved structure
Improved efficiency because of the increased granularity

What has been changed compared to main:

Each AT command (or a subset of adjacent AT commands) sits in its own module that can be toggled with a Kconfig option
The work queue and the event handler list have been moved to a common folder and are shared between the modules

TODOs:

Add docstrings/comments to new functions
Update changelog
Update migration guide
Extend library documentation to clarify the need to enable Kconfigs for certain features
Add missing descriptions/prompts for new Kconfigs

NordicBuilder · 2024-09-11T08:57:34Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 109

Inputs:

Sources:

sdk-nrf: PR head: 0101c536961218edf748be9edf1b3a91570ddf84

more details

sdk-nrf:

PR head: 0101c536961218edf748be9edf1b3a91570ddf84
merge base: b2fbe4262015a1219b0e1fc7dde50bb1c7f80296
target head (main): 4b32dada658b7c84a5a7b2e1d59a6a0eefb35419
Diff

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (70)

applications
│  ├── asset_tracker_v2
│  │  ├── overlay-lwm2m.conf
│  │  │ prj.conf
doc
│  ├── nrf
│  │  ├── libraries
│  │  │  ├── modem
│  │  │  │  │ lte_lc.rst
│  │  ├── nrf.doxyfile.in
│  │  ├── releases_and_maturity
│  │  │  ├── migration
│  │  │  │  │ migration_guide_2.8.rst
│  │  │  ├── releases
│  │  │  │  │ release-notes-changelog.rst
include
│  ├── modem
│  │  │ lte_lc.h
lib
│  ├── location
│  │  │ Kconfig
│  ├── lte_link_control
│  │  ├── CMakeLists.txt
│  │  ├── Kconfig
│  │  ├── common
│  │  │  ├── CMakeLists.txt
│  │  │  ├── event_handler_list.c
│  │  │  ├── helpers.c
│  │  │  │ work_q.c
│  │  ├── include
│  │  │  ├── common
│  │  │  │  ├── event_handler_list.h
│  │  │  │  ├── helpers.h
│  │  │  │  │ work_q.h
│  │  │  ├── modules
│  │  │  │  ├── cereg.h
│  │  │  │  ├── cfun.h
│  │  │  │  ├── coneval.h
│  │  │  │  ├── cscon.h
│  │  │  │  ├── edrx.h
│  │  │  │  ├── mdmev.h
│  │  │  │  ├── ncellmeas.h
│  │  │  │  ├── periodicsearchconf.h
│  │  │  │  ├── psm.h
│  │  │  │  ├── rai.h
│  │  │  │  ├── redmob.h
│  │  │  │  ├── xfactoryreset.h
│  │  │  │  ├── xmodemsleep.h
│  │  │  │  ├── xsystemmode.h
│  │  │  │  │ xt3412.h
│  │  ├── lte_lc.c
│  │  ├── lte_lc_helpers.c
│  │  ├── lte_lc_helpers.h
│  │  ├── lte_lc_modem_hooks.c
│  │  ├── modules
│  │  │  ├── CMakeLists.txt
│  │  │  ├── Kconfig
│  │  │  ├── cereg.c
│  │  │  ├── cfun.c
│  │  │  ├── coneval.c
│  │  │  ├── cscon.c
│  │  │  ├── edrx.c
│  │  │  ├── mdmev.c
│  │  │  ├── ncellmeas.c
│  │  │  ├── periodicsearchconf.c
│  │  │  ├── psm.c
│  │  │  ├── rai.c
│  │  │  ├── redmob.c
│  │  │  ├── xfactoryreset.c
│  │  │  ├── xmodemsleep.c
│  │  │  ├── xsystemmode.c
│  │  │  │ xt3412.c
modules
│  ├── memfault-firmware-sdk
│  │  │ Kconfig
samples
│  ├── cellular
│  │  ├── battery
│  │  │  │ prj.conf
│  │  ├── gnss
│  │  │  ├── prj.conf
│  │  │  │ sample.yaml
│  │  ├── location
│  │  │  │ prj.conf
│  │  ├── lwm2m_client
│  │  │  ├── overlay-aggressive-psm.conf
│  │  │  │ prj.conf
│  │  ├── modem_shell
│  │  │  │ prj.conf
│  │  ├── nrf_cloud_multi_service
│  │  │  │ prj.conf
│  │  ├── nrf_cloud_rest_cell_location
│  │  │  │ prj.conf
│  │  ├── udp
│  │  │  │ prj.conf
subsys
│  ├── net
│  │  ├── lib
│  │  │  ├── lwm2m_client_utils
│  │  │  │  │ Kconfig
tests
│  ├── lib
│  │  ├── location
│  │  │  ├── Kconfig
│  │  │  │ prj.conf
│  │  ├── lte_lc_api
│  │  │  │ prj.conf
│  ├── subsys
│  │  ├── net
│  │  │  ├── lib
│  │  │  │  ├── lwm2m_client_utils
│  │  │  │  │  │ CMakeLists.txt
│  │  │  │  ├── lwm2m_fota_utils
│  │  │  │  │  │ CMakeLists.txt

Outputs:

Toolchain

Version: 3dd8985b56
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:3dd8985b56_912848a074

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister
- sdk-nrf test count: 314
✅ Integration tests
- ✅ test-fw-nrfconnect-nrf-iot_cloud
- ✅ test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
- ✅ test-fw-nrfconnect-nrf-iot_samples
- ✅ test-fw-nrfconnect-nrf-iot_lwm2m
- ✅ test-fw-nrfconnect-nrf-iot_thingy91
- ✅ test-fw-nrfconnect-nrf-iot_mosh
- ✅ test-fw-nrfconnect-nrf-iot_positioning
- ⚠️ test-fw-nrfconnect-fw-update

Disabled integration tests

- desktop52_verification
- doc-internal
- test_ble_nrf_config
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-ble_samples
- test-fw-nrfconnect-boot
- test-fw-nrfconnect-chip
- test-fw-nrfconnect-fem
- test-fw-nrfconnect-nfc
- test-fw-nrfconnect-nrf-iot_libmodem-nrf
- test-fw-nrfconnect-nrf-iot_nrf_provisioning
- test-fw-nrfconnect-nrf-iot_serial_lte_modem
- test-fw-nrfconnect-nrf_crypto
- test-fw-nrfconnect-proprietary_esb
- test-fw-nrfconnect-ps
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-rs
- test-fw-nrfconnect-tfm
- test-fw-nrfconnect-thread
- test-fw-nrfconnect-zigbee
- test-low-level
- test-sdk-audio
- test-sdk-dfu
- test-sdk-find-my
- test-sdk-mcuboot
- test-sdk-pmic-samples
- test-sdk-sidewalk
- test-sdk-wifi

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

NordicBuilder · 2024-09-11T09:22:08Z

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.

lib/lte_link_control/modules/xsystemmode/Kconfig

tejlmand

looking much better.

Some questions to reply, and depending on answer / followup jira issues, then I believe it can get a +1.

tejlmand · 2024-10-22T10:54:27Z

tests/subsys/net/lib/lwm2m_client_utils/CMakeLists.txt

+  -DCONFIG_LTE_LC_EDRX_MODULE=1
+  -DCONFIG_LTE_LC_NEIGHBOR_CELL_MEAS_MODULE=1
+  -DCONFIG_LTE_LC_PSM_MODULE=1
+  -DCONFIG_LTE_LC_TAU_PRE_WARNING_MODULE=1


placing a comment to remember why this has been accepted.

This construct is not optimal. This is something to be addressed in future, see: #17266 (comment)

tejlmand · 2024-10-22T12:35:26Z

subsys/net/lib/lwm2m_client_utils/Kconfig

+	imply LTE_LC_EDRX_MODULE
+	imply LTE_LC_NEIGHBOR_CELL_MEAS_MODULE
+	imply LTE_LC_PSM_MODULE
+	imply LTE_LC_TAU_PRE_WARNING_MODULE


as raised else-where in comments, then I'm a bit unsure if lwm2m actually depends on lte lc modules or not.

And if lwm2m works just as fine with lte lc modules as without, then why the imply ?
I do notice that changing this to a depends on seems to open pandoras box of circular dependencies (including select and imply deps), so far seem to be originating at a bad select FLASH somewhere (still investigating).

But I would like some more reasoning regarding those imply, than just, this makes it work.

@tejlmand lwm2m_client_utils depends on the link controller, but it doesn't show. This is happening because, as far as I understand, the way it is used indirectly brings in the link controller from other components. This is a design flaw because each component should export a contract of its dependencies (in this case via Kconfig), at minimum to document them. This should be addressed, but it requires more work because its tests seem to depend on this design.

accepted. To be addressed later.
Please create internal ticket.

tejlmand

approved. see conditions in comments.

MirkoCovizzi · 2024-10-23T08:09:59Z

looking much better.

Some questions to reply, and depending on answer / followup jira issues, then I believe it can get a +1.

NCSDK-29864

trantanen

Good ~12kB flash improvement for those using just the core features.
And nice code restructuring for maintenance.

Divides the LTE Link Controller into modules. Signed-off-by: Mirko Covizzi <[email protected]> Co-authored-by: divya pillai <[email protected]>

Removes linking to removed functions in the LTE link controller. Signed-off-by: Mirko Covizzi <[email protected]>

Use the proper macro instead of a hardcoded string. Signed-off-by: Mirko Covizzi <[email protected]>

MirkoCovizzi self-assigned this Sep 11, 2024

MirkoCovizzi requested a review from eivindj-nordic September 11, 2024 08:56

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Sep 11, 2024

MirkoCovizzi requested a review from anhmolt September 11, 2024 08:56

trantanen reviewed Sep 12, 2024

View reviewed changes

lib/lte_link_control/modules/xsystemmode/Kconfig Outdated Show resolved Hide resolved

MirkoCovizzi force-pushed the llc-refactor branch 4 times, most recently from 00fe095 to 62d626e Compare September 13, 2024 14:09

MirkoCovizzi changed the title ~~[WIP] lib: lte_link_control: divide into modules~~ [WIP] lib: lte_link_control: refactor into modules Sep 13, 2024

MirkoCovizzi force-pushed the llc-refactor branch 4 times, most recently from ab799b5 to 1a17a8d Compare September 16, 2024 08:29

MirkoCovizzi marked this pull request as ready for review September 16, 2024 08:33

MirkoCovizzi requested review from jhirsi and tokangas as code owners September 16, 2024 08:33

MirkoCovizzi force-pushed the llc-refactor branch from 1a17a8d to 7e13fb9 Compare September 16, 2024 08:57

MirkoCovizzi requested review from plskeggs, jayteemo, glarsennordic, coderbyheart and a team as code owners September 16, 2024 08:57

MirkoCovizzi requested review from jhn-nordic and simensrostad and removed request for a team September 16, 2024 08:57

MirkoCovizzi marked this pull request as draft September 16, 2024 10:20

MirkoCovizzi force-pushed the llc-refactor branch from 7e13fb9 to 0df22f3 Compare September 16, 2024 10:20

MirkoCovizzi force-pushed the llc-refactor branch from 8ecb98e to 3473990 Compare October 21, 2024 08:01

MirkoCovizzi requested a review from trantanen October 21, 2024 08:03

MirkoCovizzi force-pushed the llc-refactor branch 2 times, most recently from be81159 to 0de4670 Compare October 21, 2024 08:38

MirkoCovizzi added DNM and removed DNM labels Oct 21, 2024

MirkoCovizzi force-pushed the llc-refactor branch 2 times, most recently from 7c59e1d to 2415638 Compare October 22, 2024 12:25

tejlmand reviewed Oct 22, 2024

View reviewed changes

MirkoCovizzi force-pushed the llc-refactor branch 3 times, most recently from fe06404 to 1df0844 Compare October 22, 2024 14:27

tejlmand approved these changes Oct 23, 2024

View reviewed changes

MirkoCovizzi force-pushed the llc-refactor branch from 1df0844 to e27eba2 Compare October 23, 2024 08:09

MirkoCovizzi removed the DNM label Oct 23, 2024

MirkoCovizzi force-pushed the llc-refactor branch from e27eba2 to a43c8f3 Compare October 23, 2024 08:20

trantanen approved these changes Oct 23, 2024

View reviewed changes

MirkoCovizzi added the CI-all-test Run All integration tests label Oct 23, 2024

MirkoCovizzi force-pushed the llc-refactor branch 2 times, most recently from 16d92c4 to a8080e6 Compare October 23, 2024 11:01

MirkoCovizzi removed the CI-all-test Run All integration tests label Oct 23, 2024

MirkoCovizzi and others added 2 commits October 23, 2024 15:42

lib: lte_link_control: refactor into modules

4989329

Divides the LTE Link Controller into modules. Signed-off-by: Mirko Covizzi <[email protected]> Co-authored-by: divya pillai <[email protected]>

doc: nrf: releases: remove linking to removed functions

088d61c

Removes linking to removed functions in the LTE link controller. Signed-off-by: Mirko Covizzi <[email protected]>

MirkoCovizzi force-pushed the llc-refactor branch from a8080e6 to f1a3c74 Compare October 23, 2024 13:42

lib: lte_link_control: cereg: use macro instead of hardcoded

0101c53

Use the proper macro instead of a hardcoded string. Signed-off-by: Mirko Covizzi <[email protected]>

MirkoCovizzi force-pushed the llc-refactor branch from f1a3c74 to 0101c53 Compare October 23, 2024 14:39

rlubos merged commit cbf1d39 into nrfconnect:main Oct 23, 2024
13 checks passed

simensrostad mentioned this pull request Oct 23, 2024

wip: projectwide: update sdk-nrf hello-nrfcloud/firmware#424

Merged

MirkoCovizzi mentioned this pull request Oct 24, 2024

lib: lte_link_control: modules: remove Kconfig #18153

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: lte_link_control: refactor into modules #17266

lib: lte_link_control: refactor into modules #17266

MirkoCovizzi commented Sep 11, 2024 •

edited

Loading

NordicBuilder commented Sep 11, 2024 •

edited

Loading

NordicBuilder commented Sep 11, 2024

tejlmand left a comment

tejlmand Oct 22, 2024

tejlmand Oct 22, 2024

MirkoCovizzi Oct 22, 2024 •

edited

Loading

tejlmand Oct 23, 2024

tejlmand left a comment

MirkoCovizzi commented Oct 23, 2024

trantanen left a comment

lib: lte_link_control: refactor into modules #17266

lib: lte_link_control: refactor into modules #17266

Conversation

MirkoCovizzi commented Sep 11, 2024 • edited Loading

NordicBuilder commented Sep 11, 2024 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

NordicBuilder commented Sep 11, 2024

tejlmand left a comment

Choose a reason for hiding this comment

tejlmand Oct 22, 2024

Choose a reason for hiding this comment

tejlmand Oct 22, 2024

Choose a reason for hiding this comment

MirkoCovizzi Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

tejlmand Oct 23, 2024

Choose a reason for hiding this comment

tejlmand left a comment

Choose a reason for hiding this comment

MirkoCovizzi commented Oct 23, 2024

trantanen left a comment

Choose a reason for hiding this comment

MirkoCovizzi commented Sep 11, 2024 •

edited

Loading

NordicBuilder commented Sep 11, 2024 •

edited

Loading

MirkoCovizzi Oct 22, 2024 •

edited

Loading