Conversation
NordicBuilder
added
doc-required
CI InformationTo view the history of this post, click the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 4ef631a3654d4c88f29a997ec8e9224bad5e1d24 more detailssdk-nrf:
Github labels
List of changed files detected by CI (8)Outputs:ToolchainVersion: 4894884e96 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved download deadlock and native_sim event-handling issues can prevent correct completion or socket setup.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds IPv4/IPv6 coexistence support to the UDP and download samples, including family selection, connectivity tracking, and retries.
Changes:
- Adds IPv4/IPv6 UDP handling.
- Adds family-aware download retries.
- Enables IPv6 and updates documentation/logging.
File summaries
| File | Summary |
|---|---|
subsys/net/lib/downloader/src/dl_socket.c |
Adjusts IPv6 lookup failure logging. |
samples/net/udp/src/main.c |
Adds IPv4/IPv6 parsing, socket, and event handling. |
samples/net/udp/README.rst |
Documents IPv4/IPv6 server support. |
samples/net/udp/prj.conf |
Enables IPv6 networking. |
samples/net/download/src/main.c |
Tracks families and retries downloads. |
samples/net/download/prj.conf |
Enables IPv6 networking. |
Review details
Suppressed comments (4)
samples/net/download/src/main.c:536
- When all attempts fail, the preceding code prints
Giving up, but this unconditional message immediately saysDownload finished, which makes a failed download look successful in the logs. Use a cleanup-only message here or emit the success wording only whendownload_succeededis true.
printk("Download finished, bringing network interfaces down\n");
samples/net/download/src/main.c:483
- After the eighth failed attempt, the failure branch still sleeps using the maximum backoff, then the next iteration increments
attemptto 9 and breaks. This adds an unnecessary delay after retries are exhausted and causes the final message to report 9 failed attempts even though only 8 downloads ran; the loop should stop/back off only when another attempt will actually be made.
if (attempt > MAX_ATTEMPTS) {
break;
}
samples/net/download/src/main.c:538
- This changes the terminal output, but
samples/net/download/README.rststill documentsByeand omits the new shutdown message. Update the sample output documentation so it matches the messages emitted by this implementation.
printk("Download finished, bringing network interfaces down\n");
(void)conn_mgr_if_disconnect(net_if);
(void)conn_mgr_all_if_down(true);
samples/net/udp/src/main.c:96
- The new IPv6 path still reports packet size using
UDP_IP_HEADER_SIZE, which is 28 bytes for the IPv4 header. IPv6 has a 40-byte base header, so IPv6 transmissions now log an incorrect size; make the accounting family-dependent or remove the header adjustment.
if (host_addr_family == AF_INET6) {
struct sockaddr_in6 *server6 = ((struct sockaddr_in6 *)&host_addr);
if (server6->sin6_port == 0) {
server6->sin6_port = htons(CONFIG_UDP_SAMPLE_SERVER_PORT);
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
4b2f2a0 to
0b2dceb
Compare
0b2dceb to
5fe6e16
Compare
|
You can find the documentation preview for this PR here. Preview links for modified nRF Connect SDK documents: https://ncsbmdoc.z6.web.core.windows.net/ncs/PR-31381/nrf/protocols/multiprotocol/index.html |
0c9d243 to
6abf567
Compare
PavelVPV
left a comment
PavelVPV
left a comment
There was a problem hiding this comment.
Copilot comments seems valid
| * This is necessary because the network interface is automatically brought up | ||
| * at SYS_INIT() before main() is called. | ||
| * This means that NET_EVENT_L4_CONNECTED fires before the | ||
| * This means that the L4 connected event fires before the |
There was a problem hiding this comment.
Wouldn't it be better to keep event name here?
There was a problem hiding this comment.
This is because we now wait for specifically the v4 or v6 event depending on which destination address we're going to send to. Updated the comment for clarity
4f13a48 to
d89e1a6
Compare
Catch IPv4 vs IPv6 connected events separately so as to run the downloder on the available families only. This avoids failure to reach the server on a family advertised by it but that the device has no address for. Signed-off-by: Simon Duquennoy <simon.duquennoy@nordicsemi.no>
Retry the download a few times with an increasing backoff until it completes successfully. Sometimes the device will get an IP family first then attempt download on this family and we may need a retry on the other family. Signed-off-by: Simon Duquennoy <simon.duquennoy@nordicsemi.no>
Callers to dl_socket_host_lookup() log EHOSTUNREACH err with LOG_ERR(), making the "Failed to resolve hostname" log inside the function mostly redundant. The thing it adds is the lower-level zsock_getaddrinfo() error code. Demote to debug to avoid clutter. Signed-off-by: Simon Duquennoy <simon.duquennoy@nordicsemi.no>
d89e1a6 to
788210e
Compare
PavelVPV
left a comment
PavelVPV
left a comment
There was a problem hiding this comment.
THanks! Approving! I wonder though how our test will behave with the retry considering instabilties we know: will it hide these instabilities or will it fail?
Agree, created this ticket https://nordicsemi.atlassian.net/jira/software/c/projects/NCSDK/boards/2963?selectedIssue=NCSDK-41388 |
Make the sample work when supplied with an IPv6 server address. Extract IP family from UDP_SAMPLE_SERVER_ADDRESS_STATIC then wait for corresponding network manager connected event and then transmit. Signed-off-by: Simon Duquennoy <simon.duquennoy@nordicsemi.no>
Without these, zsock_setsockopt(SO_RCVTIMEO) fails with ENOPROTOOPT. Signed-off-by: Simon Duquennoy <simon.duquennoy@nordicsemi.no>
788210e to
4ef631a
Compare
Fixes IP family management for both the download and udp samples.
UDP:
Download:
Tested on nRF7120dk.
Logs for the download sample under different conditions:
A. Device has two IPs but IPv6 server is not reachable
B. Device has only IPv4
C. Device gets IPv6 first with server not available, then only later gets IPv4
D. Functioning end-to-end IPv6
Tickets: