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

applications: slm: try to continue even no DNS record was sent #19701

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

TaeZStkyoht
Copy link
Contributor

@TaeZStkyoht TaeZStkyoht commented Dec 23, 2024

We shouldn't expect 2 DNS records from ISP. Because it sends sometimes 1 and even 0. We must just try to continue to establish PPP connection.

Real world scenario:
ISP sends DNS records as:

  • Onomondo eSim in Germany in NB-IoT mode: 0 (command result: +CGCONTRDP: 0,,"onomondo","","")
  • Vodafone sim in Turkey in NB-IoT mode: 0 (command result: +CGCONTRDP: 0,,"internet","","")
  • Turkcell sim in Turkey in NB-IoT mode: 0 (command result: +CGCONTRDP: 0,,"nbiot","","")
  • JIO sim in India in NB-IoT mode: 1 (command result: +CGCONTRDP: 0,,"jionet","","","2405:0200:0800:0000:0000:0000:0000:0006","")

MOSH was already working fine with allowing like that I've implemented here for SLM. We were using MOSH in at_cmd_mode on our product. However we want to use SLM; because, SLM is an application while MOSH is a sample. And we want to also be able to access GPIO command to swtich sim/eSim from outside easily. SLM is capable of doing it while MOSH not.

If this change is really directly not ok for you, I can also make it configurable with KConfig. By default can be false and we can set it to true on our project. Nevertheless, MOSH works like that, why not SLM?

@TaeZStkyoht TaeZStkyoht requested review from a team as code owners December 23, 2024 09:15
@github-actions github-actions bot added the doc-required PR must not be merged without tech writer approval. label Dec 23, 2024
@NordicBuilder
Copy link
Contributor

Thank you for your contribution!
It seems you are not a member of the nrfconnect GitHub organization. External contributions are handled as follows:
Large contributions, affecting multiple subsystems for example, may be rejected if they are complex, may introduce regressions due to lack of test coverage, or if they are not consistent with the architecture of nRF Connect SDK.
PRs will be run in our continuous integration (CI) test system.
If CI passes, PRs will be tagged for review and merged on successful completion of review. You may be asked to make some modifications to your contribution during review.
If CI fails, PRs may be rejected or may be tagged for review and rework.
PRs that become outdated due to other changes in the repository may be rejected or rework requested.
External contributions will be prioritized for review based on the relevance to current development efforts in nRF Connect SDK. Bug fix PRs will be prioritized.
You may raise issues or ask for help from our Technical Support team by visiting https://devzone.nordicsemi.com/.

Note: This comment is automatically posted and updated by the Contribs GitHub Action.

@NordicBuilder NordicBuilder added the external External contribution label Dec 23, 2024
@TaeZStkyoht TaeZStkyoht force-pushed the fix_dns_records_expectation branch 2 times, most recently from 53d856f to bfb7fc6 Compare December 23, 2024 10:38
@TaeZStkyoht TaeZStkyoht changed the title applications: slm: try to continue even no DNS records was sent applications: slm: try to continue even no DNS record was sent Dec 23, 2024
Copy link
Contributor

@MarkusLassila MarkusLassila left a comment

Choose a reason for hiding this comment

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

Another good finding. The functions should be changed so that they do not fail in the scenarios where the number of the DNS addresses is unexpected.

applications/serial_lte_modem/src/slm_ppp.c Outdated Show resolved Hide resolved
lib/pdn/pdn.c Outdated Show resolved Hide resolved
lib/pdn/pdn.c Outdated Show resolved Hide resolved
@TaeZStkyoht TaeZStkyoht force-pushed the fix_dns_records_expectation branch 2 times, most recently from 2cd13a7 to d1fa9c0 Compare December 23, 2024 16:31
@@ -453,6 +453,7 @@ Modem libraries
* :ref:`pdn_readme` library:

* Added the :c:func:`pdn_dynamic_params_get_v6` function to get PDN parameters for IPv6-only.
* Changed logic DNS record expectation. Because ISP sends sometimes 1 and even 0.
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
* Changed logic DNS record expectation. Because ISP sends sometimes 1 and even 0.
* Changed logic DNS record expectation, because ISP sometimes sends 1 and even 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@TaeZStkyoht TaeZStkyoht force-pushed the fix_dns_records_expectation branch 2 times, most recently from 73ee8bd to 6159bb1 Compare January 3, 2025 14:34
include/modem/pdn.h Outdated Show resolved Hide resolved
include/modem/pdn.h Outdated Show resolved Hide resolved
@TaeZStkyoht TaeZStkyoht force-pushed the fix_dns_records_expectation branch 5 times, most recently from f69eac8 to 8dea03e Compare January 6, 2025 09:55
@eivindj-nordic eivindj-nordic requested a review from lemrey January 7, 2025 07:47
Copy link
Contributor

@eivindj-nordic eivindj-nordic left a comment

Choose a reason for hiding this comment

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

Some nits.
It would be nice to have some tests in https://github.com/nrfconnect/sdk-nrf/tree/main/tests/lib/pdn, though they can also be added in a follow up PR.
I'll trigger CI for this commit.

include/modem/pdn.h Outdated Show resolved Hide resolved
include/modem/pdn.h Outdated Show resolved Hide resolved
lib/pdn/pdn.c Outdated Show resolved Hide resolved
lib/pdn/pdn.c Outdated Show resolved Hide resolved
@eivindj-nordic eivindj-nordic added the CI-Requested Approves single commit for CI tests on Internal HW label Jan 7, 2025
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jan 7, 2025

CI Information

To view the history of this post, clich the 'edited' button above
Build number: 7

Inputs:

Sources:

sdk-nrf: PR head: 5af606c08731e46bbca1a60a3c41ea8775ac3a0b

more details

sdk-nrf:

PR head: 5af606c08731e46bbca1a60a3c41ea8775ac3a0b
merge base: 852ae666c4c15cea69f5ee33529d3220f66c070e
target head (main): 852ae666c4c15cea69f5ee33529d3220f66c070e
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 (5)
applications
│  ├── serial_lte_modem
│  │  ├── src
│  │  │  │ slm_ppp.c
doc
│  ├── nrf
│  │  ├── releases_and_maturity
│  │  │  ├── releases
│  │  │  │  │ release-notes-changelog.rst
include
│  ├── modem
│  │  │ pdn.h
lib
│  ├── pdn
│  │  │ pdn.c
samples
│  ├── cellular
│  │  ├── modem_shell
│  │  │  ├── src
│  │  │  │  ├── link
│  │  │  │  │  │ link_api.c

Outputs:

Toolchain

Version: b77d8c1312
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:b77d8c1312_912848a074

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

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister
    • sdk-nrf test count: 355
  • ✅ Integration tests
    • ✅ test-fw-nrfconnect-nrf-iot_serial_lte_modem
    • ✅ test-fw-nrfconnect-nrf-iot_samples
    • ✅ test-fw-nrfconnect-nrf-iot_lwm2m
    • ✅ test-fw-nrfconnect-nrf-iot_mosh
    • ⚠️ test-fw-nrfconnect-nrf-iot_cloud
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_positioning
    • test-fw-nrfconnect-nrf-iot_thingy91
    • test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
    • test-fw-nrfconnect-nrf_crypto
    • 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
    • test-secdom-samples-public

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

@github-actions github-actions bot removed the CI-Requested Approves single commit for CI tests on Internal HW label Jan 7, 2025
@TaeZStkyoht TaeZStkyoht force-pushed the fix_dns_records_expectation branch 2 times, most recently from 43275ff to b50e3ba Compare January 8, 2025 13:34
@tokangas tokangas added the CI-Requested Approves single commit for CI tests on Internal HW label Jan 8, 2025
@TaeZStkyoht TaeZStkyoht force-pushed the fix_dns_records_expectation branch from b50e3ba to 577cd31 Compare January 8, 2025 13:53
@github-actions github-actions bot removed the CI-Requested Approves single commit for CI tests on Internal HW label Jan 8, 2025
@TaeZStkyoht
Copy link
Contributor Author

@tokangas Sorry, I've rebased the branch right after you added CI-Requested tag and bot removed it.

@tokangas tokangas added the CI-Requested Approves single commit for CI tests on Internal HW label Jan 8, 2025
lib/pdn/pdn.c Show resolved Hide resolved
include/modem/pdn.h Outdated Show resolved Hide resolved
include/modem/pdn.h Show resolved Hide resolved
@TaeZStkyoht TaeZStkyoht force-pushed the fix_dns_records_expectation branch from 577cd31 to 5ef88bc Compare January 8, 2025 15:38
@github-actions github-actions bot removed the CI-Requested Approves single commit for CI tests on Internal HW label Jan 8, 2025
@MarkusLassila MarkusLassila added the CI-Requested Approves single commit for CI tests on Internal HW label Jan 9, 2025
@github-actions github-actions bot removed the CI-Requested Approves single commit for CI tests on Internal HW label Jan 9, 2025
@TaeZStkyoht TaeZStkyoht force-pushed the fix_dns_records_expectation branch from a660f32 to 8e86873 Compare January 9, 2025 09:37
@MarkusLassila MarkusLassila added the CI-Requested Approves single commit for CI tests on Internal HW label Jan 9, 2025
@TaeZStkyoht TaeZStkyoht force-pushed the fix_dns_records_expectation branch from 8e86873 to e009791 Compare January 9, 2025 09:54
@github-actions github-actions bot removed the CI-Requested Approves single commit for CI tests on Internal HW label Jan 9, 2025
- We shouldn't expect 2 DNS records from ISP.
Because it sends sometimes 1 and even 0.
It must try to continue like in MOSH.
- convert
link_api_pdp_context_dynamic_params_get
to
pdn_pdp_context_dynamic_params_get
and use it on both SLM and MOSH

Signed-off-by: Oguzhan Turk <[email protected]>
Co-authored-by: Eivind Jølsgard <[email protected]>
Co-authored-by: Pekka Niskanen <[email protected]>
Co-authored-by: Tommi Kangas <[email protected]>
Co-authored-by: Tommi Rantanen <[email protected]>
@TaeZStkyoht TaeZStkyoht force-pushed the fix_dns_records_expectation branch from e009791 to 5af606c Compare January 9, 2025 09:54
@eivindj-nordic eivindj-nordic added the CI-Requested Approves single commit for CI tests on Internal HW label Jan 9, 2025
Copy link
Contributor

@tokangas tokangas left a comment

Choose a reason for hiding this comment

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

@TaeZStkyoht Thanks for your contribution and patience with all requested changes! 😃

Copy link
Contributor

@trantanen trantanen left a comment

Choose a reason for hiding this comment

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

Great work and thanks for the patience @TaeZStkyoht 😄

@trantanen trantanen requested a review from peknis January 9, 2025 11:57
@tokangas
Copy link
Contributor

tokangas commented Jan 9, 2025

@jukkar or @rlubos, please have a look.

@peknis, your approval is also missing.

@rlubos rlubos merged commit ddfb6df into nrfconnect:main Jan 9, 2025
15 checks passed
@TaeZStkyoht TaeZStkyoht deleted the fix_dns_records_expectation branch January 9, 2025 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-Requested Approves single commit for CI tests on Internal HW doc-required PR must not be merged without tech writer approval. external External contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants