Rewrite of NCM device driver #2227
Conversation
src/class/net/ncm_device.c
Outdated
| #endif | ||
|
|
||
| #define TU_LOG_DRV(...) TU_LOG(CFG_TUD_NCM_LOG_LEVEL, __VA_ARGS__) | ||
| #if 0 | ||
| #define INFO_OUT(...) printf(__VA_ARGS__) |
There was a problem hiding this comment.
Can we us the debug macros defined in tusb_debug.h ?
|
|
||
|
|
||
| static void notification_xmit(uint8_t rhport, bool force_next) | ||
| /** |
There was a problem hiding this comment.
can we make comment format consistent with tinyUSB ?
There was a problem hiding this comment.
I agree that block comments looks much cleaner than inline comment format used in TinyUSB 🤔
There was a problem hiding this comment.
not sure about this: what are the TinyUSB comment conventions?
| .wNtbOutMaxDatagrams = 6 // 0=no limit | ||
| }; | ||
|
|
||
| // Some confusing remarks about wNtbOutMaxDatagrams... |
There was a problem hiding this comment.
Remove redundant Debug comments
|
Hi, sorry for huge delay. I'm not an expert of NCM class but I'd like to move the things on. Compared to RNDIS current NCM implementation is problematic, and new implementation doesn't perform well at high MSS on Linux, with STM32F723:
On the bus I observed high delay between OUT and IN transfer, also between 2 OUT transfers for both NCM and NCM New. I'll try to review your implementation, below is the USB capture in case if you're interested. |
Hello HiFiPhile very interesting observation. Could you do the little benchmark comparison also with Win10. |
It seems like iperf on Windows doesn't support MSS adjustment, I see no difference in captured packet. On Windows there is only one TCP ACK packet each time instead of 2, also delay between transfer is reduced. I got a throughput of 24Mbps. Here is the USB capture : One thing strange is on USB bus ACK seq number is shifted from data packet, or maybe it's a decoding issue.
Here is the Netif capture on Linux: |
|
Hmmm... don't know what to do here. Unfortunately I don't have any Hi-Speed device in my drawer, so incapable of checking the above. At least I have a project using NCM which also works in conjunction with Win10. Speed is comparable. My major concern was compatibility. |
|
I was able to massively boost the performance simply by increasing
MSS=1450 comparaison
Instead of one complete datagram 2 segments are sent, both are acked, then high delay (~1ms) until next transfer.
Two complete datagrams are sent, both are acked, then high delay (~1ms) until next transfer.
Two complete datagrams are sent, only 1 ack, ack delay is a little bit higher maybe for processing, no delay until next transfer. It makes me to believe there must be some relationships between TCP window and NTB size to make it works better, so I decreased NTB size to 1600 while keeping TCP_WND = 2*TCP_MSS:
2 Packets are sent and 2 ack, no delay until next transfer. Below are all USB captures: Bonus: With #2576 applied, I got 116Mbps @ MSS=1450, TCP_WND = 4*TCP_MSS ! |
|
Wow... those are great numbers. Out of curiosity: did you test different TCP_WND with RNDIS as well? And the other point: anything I can do on the driver side to enhance throughput? |
Here is RNDIS on Arch Linux:
At high MSS throughput is little higher than my previous test... so TCP_WND helps RNDIS as well. On Windows 10 there isn't much difference, always at ~11Mbps @ 1450.
With your understanding is my finding about TCP_WND vs NTB size coherent ? How did you chose the size of 3200 ? It looks like TCP_WND=2*TCP_MSS and NTB_SIZE=1600 has a good performance and balanced RAM usage, does it works well in your case ? |
|
Meaning, that RNDIS benefits only for MSS=800/1450 (and is still faster than NCM for this cases :-/). Buffer size: I have taken the 3200 from the original TinyUSB NCM driver. So no thoughts from my side concerning buffer size. |
|
Hmmm... currently checking Zephyr sources. They do not transmit frames from device -> host which are > NET_ETH_MAX_FRAME_SIZE, which is 1500+header_size |
|
Not sure if lwip's iperf server is buggy or something else, reverse throughput doesn't work well, most cases it just doesn't transfer.
NTB is not used efficiently, always cut in half. |
|
Yes, I had the feeling as well and as far as I remember I found a bug in iperf sources but was too busy/lazy to put the fix into their merge machinery. You can give the attached file a try. Another point concerning buffer/frame sizes: I'm currently checking Zephyr sources. They do not transmit frames from device -> host which are > NET_ETH_MAX_FRAME_SIZE, which is 1500+header_size |
|
Ah... yes... one more thing: in function_ncm.c there is a definition of up/downlink speed. Never really tried values for tuning but perhaps the Linux NCM driver is smart and checks those values? |
|
Interesting... when I implemented the driver, I did a lot of performance testing. And honestly: a lot was also guess work. One of the assumptions was, that having at least two buffers for each RX/TX would be of benefit, because while one frame is sent, the other can be filled (TX) or while a received frame is being handled it should be a good idea to have one concurrently receiving new data. And I'm almost sure, that on my relatively slow RP2040 that was the case: I'm getting there around 5MBit/s in both directions, so the link is at 50% usage. For your case with hi-speed, it probably doesn't matter because transfer is not the bottleneck. Nevertheless, I have the feeling we are hunting some performance issues (which are optimizations and perhaps further to investigate). Any real reason to not merge the driver? |
Could you make test if current settings works well in your case ? Edit: I saw some improvement increasing NTBs with Full-Speed on STM32F407.
Well main reason is lacking of ressource, even my own PR's are missing from review. I'd like to ensure if everything works smoothly, since we often met the case where original authors are hard to be contacted when something happens, there are pending issues hard to fix. We are close, once we choose a parameter set I'll do a formating pass then it's ready. |
|
I understand the situation & I'm actually one of those :-/ My last (side) project was based on RP2040 and its SDK which uses TinyUSB et al. My current (professional) project is based on Zephyr, so chances are that I will drift away from TinyUSB. Concerning the settings: of course I will test those. Give me some minutes. |
|
Gosh... the number of screws to turn tends to drive me crazy: on my device, if setting CFG_TUD_NCM_*_NTB_MAX_SIZE=2048 then iperf produces retries. This can be circumvented by setting wNtbOutMaxDatagrams=1, but this is actually not the intention. So the 3200 are not randomly selected. But don't ask me, why the 2048 length produces errors. So here are my results (number=buffer-size, n=x means number of buffers):
So the current configuration isn't there for no reason, at least with my configuration. I've done also intensive tests with SystemView and outcome was the configuration you can see in the repo. |
|
Perhaps a parameter for wNtbOutMaxDatagrams should be added to allow the regular user as well to enter parameter hell!? |
|
fighting with the parameters and trying to understand... that's the current outcome: #ifndef CFG_TUD_NCM_IN_NTB_MAX_SIZE
/// must be >> MTU
#define CFG_TUD_NCM_IN_NTB_MAX_SIZE (2 * TCP_MSS + 100) // can be set to 2048 without impact
#endif
#ifndef CFG_TUD_NCM_OUT_NTB_MAX_SIZE
/// must be >> MTU
#define CFG_TUD_NCM_OUT_NTB_MAX_SIZE (2 * TCP_MSS + 100) // can be set to smaller values if wNtbOutMaxDatagrams==1
#endifMeaning: CFG_TUD_NCM_OUT_NTB_MAX_SIZE is dependent on TCP_MSS while CFG_TUD_NCM_IN_NTB_MAX_SIZE is not. CFG_TUD_NET_MTU=1514 in my case. For me it is not clear, why CFG_TUD_NCM_OUT_NTB_MAX_SIZE must be twice TCP_MSS (plus some overhead). Is it a host driver issue or what? Too many TCP parameters... |
Yes why not.
If you agree we can set NTB default at
Me too I saw this behavior on FS, maybe a driver thing. |
|
I have added your suggestions. Currently the NCM driver is not built in the CI. Any idea how to add that? |
|
I'll do comment and format things :) Also I have to move |
Thank you very much for testing and reviewing. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Thank you for your great effort for this new driver ! Now it's setup. |
| tusb_control_request_t header; | ||
| uint32_t downlink, uplink; | ||
| // Alignment must be 4 | ||
| #define TUD_NCM_ALIGNMENT 4 |
There was a problem hiding this comment.
I made this a constant, it's used to be configurable but there was also an error check to ensure TUD_NCM_ALIGNMENT=4, which is quite awkward.
|
|
||
| // Number of NCM transfer blocks for reception side | ||
| #ifndef CFG_TUD_NCM_OUT_NTB_N | ||
| #define CFG_TUD_NCM_OUT_NTB_N 1 |
There was a problem hiding this comment.
For example build I reduced both NTBs to 1 for less RAM usage, users can read ncm.h for tuning details.
| #endif | ||
|
|
||
| //------------- CLASS -------------// | ||
|
|
||
| // Network class has 2 drivers: ECM/RNDIS and NCM. | ||
| // Only one of the drivers can be enabled | ||
| #define CFG_TUD_ECM_RNDIS 1 | ||
| #define CFG_TUD_NCM (1-CFG_TUD_ECM_RNDIS) | ||
| #define CFG_TUD_ECM_RNDIS USE_ECM |
Wonderful to hear that my contribution will be part of tinyusb. |
Describe the PR
This is a rewrite of the original NCM device driver. You can check tests concerning functionality and performance here.
Additional context
Buggyness of the current driver can be easily verified with
for MSS in 100 200 400 800 1200 1450 1500; do iperf -c 192.168.14.1 -e -i 1 -M $MSS -l 8192 -P 1; sleep 2; done.I'm using the driver in my own project. The USB device is running together with Linux/Win10 without any problems.
This solves #2068.