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

applications: serial_lte_modem: Fix socket options. #12527

Merged
merged 1 commit into from
Oct 10, 2023
Merged

applications: serial_lte_modem: Fix socket options. #12527

merged 1 commit into from
Oct 10, 2023

Conversation

MarkusLassila
Copy link
Contributor

@MarkusLassila MarkusLassila commented Oct 4, 2023 •
edited
Loading

Numbering has changed for: TLS_DTLS_CID, TLS_DTLS_CID_STATUS and TLS_DTLS_HANDSHAKE_TIMEO.

Edit: If these keep on changing, we should consider string names for options.

Edit edit: SO_BINDTODEVICE has been replaced with SO_BINDTOPDN.

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

NordicBuilder commented Oct 4, 2023 •
edited
Loading

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

tomi-font
tomi-font approved these changes Oct 4, 2023
View reviewed changes
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.

Yeah. Using hard-coded numbers is really fragile. I hope users use the macros. Maybe we should even remove such numbers from the documentation... (Doesn't change the current state of tests though.)

@MarkusLassila
Copy link
Contributor Author

MarkusLassila commented Oct 4, 2023 •
edited
Loading

Yeah. Using hard-coded numbers is really fragile. I hope users use the macros. Maybe we should even remove such numbers from the documentation... (Doesn't change the current state of tests though.)

The numbers are how the options are currently selected. To remove them from documentation, would complicate the life of the user quite a bit (user of SLM is not necessary a coder). However, we should consider using the names instead of numbers.

@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.

@tomi-font
Copy link
Contributor

tomi-font commented Oct 4, 2023 •
edited
Loading

The numbers are how the options are currently selected. To remove them from documentation, would complicate the life of the user quite a bit (user of SLM is not necessary a coder). However, we should consider using the names instead of numbers.

Yeah, removing the numbers would force using or needing to look up the macros, which could be annoying if not doing it via (C) code. Using the name would be the one way to make this better.

@MarkusLassila MarkusLassila force-pushed the slm-fix-socket-option-numbering branch from 97053bb to e5e378f Compare October 9, 2023 06:28
@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 9, 2023
@MarkusLassila MarkusLassila changed the title applications: serial_lte_modem: Fix socket option numbering applications: serial_lte_modem: Fix socket options. Oct 9, 2023
@tomi-font tomi-font self-requested a review October 9, 2023 06:36
SeppoTakalo
SeppoTakalo approved these changes Oct 9, 2023
View reviewed changes
Copy link
Contributor

@SeppoTakalo SeppoTakalo left a comment

Choose a reason for hiding this comment

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

Why the numbering changed?

This is a breaking API change and should probably be mentioned in migration guide as well.

@MarkusLassila
Copy link
Contributor Author

Why the numbering changed?

This is a breaking API change and should probably be mentioned in migration guide as well.

@SeppoTakalo: Numbering changes were due to ncs_socket.h options making their way to socket.h in Zephyr. With NCS 2.4.2 only TLS_DTLS_HANDSHAKE_TIMEO existed. Should that be mentioned in changelog?

@MarkusLassila MarkusLassila force-pushed the slm-fix-socket-option-numbering branch from e5e378f to d55f0e2 Compare October 9, 2023 11:50
@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 Oct 9, 2023
MarkusLassila
MarkusLassila commented Oct 9, 2023
View reviewed changes
doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst
@@ -236,6 +236,7 @@ Serial LTE modem
* Modem FOTA to only need a modem reset to apply the firmware update.
The full chip reset (using the ``#XRESET`` AT command) remains supported.
* ``#XGPSDEL`` AT command to disallow deleting local clock (TCXO) frequency offset data because it is an internal value that should not be deleted when simulating a cold start.
* Socket option ``TLS_DTLS_HANDSHAKE_TIMEO`` to a new name value.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, the other values were not documented in v2.4.0.

@MarkusLassila
applications: serial_lte_modem: Fix socket options.
06234e1 
Numbering has changed for: TLS_DTLS_CID, TLS_DTLS_CID_STATUS and
TLS_DTLS_HANDSHAKE_TIMEO.

SO_BINDTODEVICE has been replaced with SO_BINDTOPDN.

Signed-off-by: Markus Lassila <[email protected]>
@MarkusLassila MarkusLassila force-pushed the slm-fix-socket-option-numbering branch from d55f0e2 to 06234e1 Compare October 9, 2023 14:55
@nordicjm nordicjm merged commit f42e7b4 into nrfconnect:main Oct 10, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomi-font tomi-font tomi-font approved these changes

@wiba-nordic wiba-nordic wiba-nordic approved these changes

@SeppoTakalo SeppoTakalo SeppoTakalo approved these changes

@rlubos rlubos Awaiting requested review from rlubos

@carlescufi carlescufi Awaiting requested review from carlescufi

Assignees
No one assigned
Labels
doc-required PR must not be merged without tech writer approval.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MarkusLassila @NordicBuilder @tomi-font @SeppoTakalo @wiba-nordic @nordicjm