Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes NCM device notification state handling during interface restart and refactors buffer management for improved performance. When an NCM interface is deactivated and reactivated, the link state notification is now correctly sent. Additionally, the buffer management has been optimized by replacing linear search operations with circular buffer implementations.
- Fixes notification state handling to ensure link state updates are sent when interface is reactivated
- Refactors recv_ready_ntb and xmit_ready_ntb from linear arrays with memmove operations to circular buffers for O(1) operations
- Adds LED blinking functionality to the example application with USB device state indicators
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/class/net/ncm_device.c | Implements circular buffer management for recv/xmit ready buffers with head/tail/count tracking; fixes notification state reset when interface is deactivated to ensure proper link state updates on reactivation; corrects log message in xmit_get_next_ready_ntb |
| examples/device/net_lwip_webserver/src/main.c | Adds LED blinking task with different patterns for device states (not mounted, mounted, suspended) and implements USB device callbacks (mount, unmount, suspend, resume) to control blink intervals |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Size Difference ReportBecause TinyUSB code size varies by port and configuration, the metrics below represent the averaged totals across all example builds. Note: If there is no change, only one value is shown. Changes >1% in sizeNo entries. Changes <1% in size
No changes
|
Signed-off-by: HiFiPhile <admin@hifiphile.com>
Signed-off-by: HiFiPhile <admin@hifiphile.com>
Signed-off-by: HiFiPhile <admin@hifiphile.com>
HiFiPhile
force-pushed
the
ncm_restart
branch
from
2a19aec to
bb94472
Compare
Collaborator
Author
|
@ceedriic how's it doing on your Mac ? |
1 task
Contributor
It works much better with the USB power supply fixed :) |
Contributor
Ok, I redid the tests with TinyUSB from 1 week ago, and with or without the one-liner fix here: I see no difference with or without this patch unfortunately. The following examples are all with the patch applied. When I plug my fixed board board in, most of the time I get an IP address, but sometime not. Here is when I didn't get an IP address: And here is when I get one: That's I guess the same problem that I and other other people have reported with the iPad. Note that even when DHCP fails, the RUNNING flag is still here. Now, If I try to bring the interface down and up: Then nothing works and the RUNNING flag is not set. So there is still something fishy here. I've tried with a regular usb-ethernet interface, and it works fine after up/down: both the RUNNING flag and DHCP work. I've also tried again to connect the TinyUSB NCM interface to Apline Linux 3.23 in a Mac VM: And when I connect the USB: Note the "(NO ZLP)" message, I don't know if that's something to be worried about. I need to "UP" the interface manually on Alpine: And then start dhcp manually: At this point, if I bring the interface down and up, the interface goes back as "RUNNING", but DHCP does not work anymore: I don't know what to make of all that. Cedric |
Signed-off-by: HiFiPhile <admin@hifiphile.com>
Collaborator
Author
|
I feel like something shady is going on, with this PR on my Windows PC and Arch laptop Nucleo-U5A5/H747-DISCO boards work very well the 1st plug or link up/down by ip command or board button. Could you try H747-DISCO with net lwip example ? If the initial connection doesn't work, what about wakeup button toggle ? I've added a board button fix. |
Contributor
Well, I just tried on one of my FreeBSD server and it works well there too: So there is something special with the Mac. Note that I'm working on full-speed only, and I guess not many people use an ethernet adapter with full-speed these days... I wonder if there is a bug in MacOS at low speed?
Yes, but not before middle of week-end, I'm about to leave and travel to Sion to visit a customer there. However, in 10 days I'll be near Lyon :) |
Contributor
|
Ok, there are problems on the Mac (see issue #3505) but this patch fix a real problem at least on Windows and should be applied IMHO. |
hathach
reviewed
Mar 12, 2026
| notification_xmit(rhport, false); | ||
| } else { | ||
| // Reset notification state to send link state update when interface is re-activated | ||
| ncm_interface.notification_xmit_state = NOTIFICATION_CONNECTED; |
Owner
There was a problem hiding this comment.
@HiFiPhile should we reset to NOTIFICATION_SPEED instead of NOTIFICATION_CONNECTED. I got claude suggestion below
NCM spec ch.7.1 mandates that CONNECTION_SPEED_CHANGE must be sent before NETWORK_CONNECTION. Both notifications are required, in that order.
All major implementations confirm this:
┌───────────────────────┬────────────────────────────┬─────────────────────────────────────────────────────────────────────────────┐
│ Implementation │ On alt=1 reactivation │ Reset state │
├───────────────────────┼────────────────────────────┼─────────────────────────────────────────────────────────────────────────────┤
│ Linux gadget f_ncm.c │ Resets to NCM_NOTIFY_SPEED │ Full sequence (speed → connected) │
├───────────────────────┼────────────────────────────┼─────────────────────────────────────────────────────────────────────────────┤
│ Zephyr usbd_cdc_ncm.c │ Resets to IF_STATE_INIT │ Full sequence, with comment: "Speed change must be sent first, chapter 7.1" │
├───────────────────────┼────────────────────────────┼─────────────────────────────────────────────────────────────────────────────┤
│ Linux host cdc_ncm.c │ Expects both │ Comment cites spec ch.7.1 │
├───────────────────────┼────────────────────────────┼─────────────────────────────────────────────────────────────────────────────┤
│ macOS │ Requires both │ Won't do DHCP without NETWORK_CONNECTION preceded by speed │
└───────────────────────┴────────────────────────────┴─────────────────────────────────────────────────────────────────────────────┘
What this means for the PR
The fix at line 978 should change from:
ncm_interface.notification_xmit_state = NOTIFICATION_CONNECTED;
to:
ncm_interface.notification_xmit_state = NOTIFICATION_SPEED;
And additionally, when itf_data_alt == 1 (reactivation), the state should also be reset to NOTIFICATION_SPEED before calling notification_xmit(), so the full sequence is sent on every
reactivation — matching Linux and Zephyr behavior. The current code at line 941-942 calls notification_xmit() but doesn't reset the state, so if the previous cycle completed
(NOTIFICATION_DONE), nothing gets sent.
```
Collaborator
Author
There was a problem hiding this comment.
Right, I've also updated tud_network_link_state
Signed-off-by: Zixun LI <admin@hifiphile.com>
Signed-off-by: Zixun LI <admin@hifiphile.com>
|
| target | .text | .rodata | .data | .bss | total | % diff |
|---|---|---|---|---|---|---|
| ea4357/net_lwip_webserver | 47,208 → 47,848 (+640) | — | 244 → 248 (+4) | 21,980 → 21,988 (+8) | 69,878 → 70,530 (+652) | +0.9% |
| apard32690/net_lwip_webserver | 48,016 → 48,192 (+176) | — | 196 → 200 (+4) | 17,660 → 17,668 (+8) | 65,880 → 66,068 (+188) | +0.3% |
| ea4088_quickstart/net_lwip_webserver | 41,496 → 41,640 (+144) | — | 188 → 192 (+4) | 13,304 → 13,312 (+8) | 55,004 → 55,160 (+156) | +0.3% |
| lpcxpresso18s37/net_lwip_webserver | 46,368 → 46,496 (+128) | — | 188 → 192 (+4) | — | 47,002 → 47,134 (+132) | +0.3% |
| stm32f103_bluepill/net_lwip_webserver | 41,000 → 41,140 (+140) | — | 200 → 204 (+4) | 17,044 → 17,052 (+8) | 64,684 → 64,832 (+148) | +0.2% |
| lpcxpresso1769/net_lwip_webserver | 44,492 → 44,636 (+144) | — | 188 → 192 (+4) | — | 64,884 → 65,032 (+148) | +0.2% |
| metro_m0_express/net_lwip_webserver | 43,144 → 43,284 (+140) | — | — | 16,944 → 16,952 (+8) | 68,472 → 68,628 (+156) | +0.2% |
| stlinkv3mini/net_lwip_webserver | 45,772 → 45,924 (+152) | — | 428 → 432 (+4) | 19,740 → 19,748 (+8) | 74,392 → 74,560 (+168) | +0.2% |
| nutiny_sdk_nuc505/net_lwip_webserver | — | — | 49,436 → 49,584 (+148) | 20,084 → 20,092 (+8) | 69,520 → 69,676 (+156) | +0.2% |
| msp_exp432e401y/net_lwip_webserver | 49,692 → 49,840 (+148) | — | 1,484 → 1,488 (+4) | 20,048 → 20,056 (+8) | 71,372 → 71,532 (+160) | +0.2% |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixed notification state handling so that when the interface is deactivated and reactivated, the link state update notification is correctly sent. Fix NCM interface stop transmitting packets after ifconfig down+up on Linux side (STM32H7 DWC2) #3436
Refactored both
recv_ready_ntbandxmit_ready_ntbbuffers inncm_interface_tto use circular buffers, adding head, tail, and count fields for each, replacing the previous linear search and memmove approach.