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

samples: net: Add UDP sample #11773

Merged
merged 1 commit into from
Oct 11, 2023
Merged

samples: net: Add UDP sample #11773

merged 1 commit into from
Oct 11, 2023

Conversation

simensrostad
Copy link
Contributor

@simensrostad simensrostad commented Jul 11, 2023 •
edited
Loading

Add UDP sample that demonstrates basic UDP communication.
The sample supports both Wi-Fi and LTE depending on the target board.

@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 Jul 11, 2023
lemrey
lemrey previously requested changes Jul 12, 2023
View reviewed changes
Copy link
Contributor

@lemrey lemrey left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This sample represents one of the most common use-cases for nRF91 and it is extensively used as an example/benchmark for power profiling the product. It's important that it keeps showing clearly what to do on the nRF91 to achieve low power consumption in this most common use-case of sending UDP packets.

It should remain in samples/cellular in its current form, moving it under samples/net hides everything that it is trying to show.

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Sep 11, 2023
@simensrostad
Copy link
Contributor Author

Marking DNM for now pending a discussion on how we want to split up our samples between net and cellular.

@github-actions github-actions bot removed the Stale label Sep 13, 2023
@lemrey
Copy link
Contributor

lemrey commented Oct 3, 2023

Please create a new sample in net/, we use the current sample in /cellular to showcase LTE-related features.

@simensrostad
Copy link
Contributor Author

Please create a new sample in net/, we use the current sample in /cellular to showcase LTE-related features.

Will do

@simensrostad simensrostad marked this pull request as ready for review October 6, 2023 12:40
@simensrostad simensrostad requested a review from a team as a code owner October 6, 2023 12:40
@simensrostad simensrostad changed the title samples: net: Migrate to using Zephyr NET Connection Manager samples: net: Add UDP sample Oct 6, 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 Oct 6, 2023
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Oct 6, 2023 •
edited
Loading

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite

Detailed information of selected test modules

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

@simensrostad simensrostad removed the DNM label Oct 6, 2023
@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.

@simensrostad simensrostad force-pushed the udp-sample-convert branch 2 times, most recently from 9c70ec8 to 24139bb Compare October 9, 2023 07:12
@simensrostad simensrostad force-pushed the udp-sample-convert branch 2 times, most recently from a01d307 to 0bfe8ad Compare October 10, 2023 05:52
lemrey
lemrey reviewed Oct 10, 2023
View reviewed changes
samples/net/udp/prj.conf Outdated Show resolved Hide resolved
samples/net/udp/prj.conf Outdated Show resolved Hide resolved
samples/net/udp/prj.conf Outdated Show resolved Hide resolved
@simensrostad
Copy link
Contributor Author

@lemrey @umapraseeda Comments addressed

rlubos
rlubos reviewed Oct 10, 2023
View reviewed changes
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

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

Just some nitpicking

samples/net/udp/sample.yaml Outdated Show resolved Hide resolved
samples/net/udp/src/main.c Outdated Show resolved Hide resolved
samples/net/udp/src/main.c Outdated Show resolved Hide resolved
balaji-nordic
balaji-nordic suggested changes Oct 10, 2023
View reviewed changes
doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst Outdated Show resolved Hide resolved
samples/net/udp/Kconfig Outdated Show resolved Hide resolved
samples/net/udp/README.rst
===============

If you have issues with connectivity on nRF91 Series devices, see the `Cellular Monitor`_ documentation to learn how to capture modem traces in order to debug network traffic in Wireshark.
This sample enables modem traces by default.
Copy link

Choose a reason for hiding this comment

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

Should we also add a note about debugging wifi connectivity issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't really have a good way of doing this ATM other than the usual suspects for conventional debugging. Nice improvement would be do offer a a guide to our users on how to setup wifi tracing or something equivalent. cc @ioannisg

samples/net/udp/sample.yaml Outdated Show resolved Hide resolved
samples/net/udp/boards/thingy91_nrf9160_ns.overlay Show resolved Hide resolved
samples/net/udp/boards/thingy91_nrf9160_ns.overlay Show resolved Hide resolved
samples/net/udp/src/main.c Outdated Show resolved Hide resolved
samples/net/udp/src/main.c Show resolved Hide resolved
samples/net/udp/src/main.c Show resolved Hide resolved
@balaji-nordic balaji-nordic added this to the 2.5.0 milestone Oct 10, 2023
@simensrostad simensrostad force-pushed the udp-sample-convert branch 2 times, most recently from 610d4ea to 1f1513e Compare October 11, 2023 06:53
@simensrostad
Copy link
Contributor Author

@rlubos @balaji-nordic Renamed some functions and added some more error checking.
@divipillai Added your suggested change

@jhn-nordic jhn-nordic dismissed lemrey’s stale review October 11, 2023 09:34

outdated. (and I am trying this functionality)

lemrey
lemrey approved these changes Oct 11, 2023
View reviewed changes
Copy link
Contributor

@lemrey lemrey left a comment

Choose a reason for hiding this comment

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

Looks good, just a leftover in one of the project configurations.

samples/net/udp/boards/thingy91_nrf9160_ns.conf Outdated Show resolved Hide resolved
@simensrostad
samples: net: Add UDP sample
3b44bfb 
Add UDP sample that demonstrates basic UDP communication.
The sample supports both Wi-Fi and LTE depending on the target board.

Signed-off-by: Simen S. Røstad <simen.rostad@nordicsemi.no>
@rlubos rlubos merged commit 799f66b into nrfconnect:main Oct 11, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eivindj-nordic eivindj-nordic eivindj-nordic left review comments

@divipillai divipillai divipillai approved these changes

@balaji-nordic balaji-nordic balaji-nordic approved these changes

@rlubos rlubos rlubos approved these changes

@lemrey lemrey lemrey approved these changes

@carlescufi carlescufi Awaiting requested review from carlescufi

@KajaKoren KajaKoren Awaiting requested review from KajaKoren

@jtguggedal jtguggedal Awaiting requested review from jtguggedal

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

Successfully merging this pull request may close these issues.
7 participants
@simensrostad @lemrey @NordicBuilder @rlubos @balaji-nordic @eivindj-nordic @divipillai