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.

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

fix(esp_tinyusb): Fix NCM NTB buffers configuration #114

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

peter-marcisovsky
Copy link
Collaborator

@peter-marcisovsky peter-marcisovsky commented Jan 9, 2025

Description

This MR adds a possibility to configure NCM transfer blocks via menuconfig.

A table below shows RAM usage and performance dependency on NTB buffers configuration, NTB buffers count and NTB buffers length. Memory occupation was measured using NCM Device Example on esp32s3.

NTB Buffers count (IN/OUT) 1/1 2/2 2/2 3/3 4/4
NTB Buffers length (IN/OUT) 3200/3200 3200/3200 6400/6400 8192/8192 8192/8192
Used RAM [bytes] 112320 118736 131552 155120 171520
Used RAM [%] 32.9 34.7 38.5 45.5 50.2
Download [Mbps] 1.19 4.22 5.7 7.21 7.6
Upload [Mbps] 5.04 5.67 5.96 6.14 6.03

Measuring the performance did prove a significant gain dependency on NTB buffers configurations, similarly to the description in tinyusb upstream is caliming for RP2040.

NTB Buffers count 1/1 and length 3200/3200 is a default value in tinyusb upstream.
The most efficient settings (RAM to performance gain ratio) seems to be the 4th option (NTB Buffers count 3/3, Buffers length 8192/8192).

Increasing the NTB Buffers count and length did also help to mitigate the tud_network_can_xmit: request blocked warning messages while running NCM device.

Related

Closes #113
Relates to #107

Testing

Performance measurement using Iperf did not prove to be useful.

Online speed test was used for the performance measurement (using the esp32s3 NCM device WIFi station to wired interface)

Below is a performance comparison between the

  • tinyusb's upstream default settings of the NTB buffers (count 1/1, length 3200/3200)
  • NTB Buffers configured to: count 4/4, length 8192/8192

NTB Buffers(IN/OUT) count: 1/1, length(bytes): 3200/3200

alt text

NTB Buffers(IN/OUT) count: 4/4, length(bytes): 8192/8192

alt text


Checklist

Before submitting a Pull Request, please ensure the following:

  • 🚨 This PR does not introduce breaking changes.
  • All CI checks (GH Actions) pass.
  • Documentation is updated as needed.
  • Tests are updated or added as necessary.
  • Code is well-commented, especially in complex areas.
  • Git history is clean — commits are squashed to the minimum necessary.

Copy link
Collaborator

@roma-jam roma-jam left a comment

Choose a reason for hiding this comment

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

Two general ideas:

  1. Default value for CFG_TUD_NCM_IN_NTB_N: 2 or 3?
  2. Based on the NCM specification the NTB - is a NCM Transfer Block. So the shortend NCM NTB means NCM NCM Transfer Block which might be not necessary.
    Maybe, just check the menuconfig description based on this thought.

device/esp_tinyusb/Kconfig Outdated Show resolved Hide resolved
device/esp_tinyusb/Kconfig Outdated Show resolved Hide resolved
device/esp_tinyusb/CHANGELOG.md Outdated Show resolved Hide resolved
@peter-marcisovsky
Copy link
Collaborator Author

@roma-jam @tore-espressif PTAL at the performance results from IPERF.

I did not observe any significant performance when configuring the NCM Device settings, but as stated in my comment, I still observed somer reliability issues when running some higher traffic through the device, whit fairly "low" count of the NTB buffs.

I am thinking, what would be a reasonable setting of ranges in Kconfig for the NTB buffs count.

@peter-marcisovsky peter-marcisovsky marked this pull request as ready for review January 10, 2025 15:33
@roma-jam roma-jam self-requested a review January 10, 2025 19:47
Copy link
Collaborator

@roma-jam roma-jam left a comment

Choose a reason for hiding this comment

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

Thanks for the testing and providing iperf results!
LGTM

P.S. Do not forget to squash before merge.

@peter-marcisovsky peter-marcisovsky force-pushed the fix/esp_tinyusb_fix_ncm_ntb_buffs_config branch from ee84b11 to 3a9f7a6 Compare January 13, 2025 16:14
@tore-espressif
Copy link
Collaborator

tore-espressif commented Jan 14, 2025

@peter-marcisovsky @roma-jam Thank you for all the testing. I also run some tests (with Win10), here are my results:

  1. Our WiFi (20-30Mbs/) is faster than USB FS (12Mb/s), so some requests will get 'blocked' just because we cannot send them so fast through USB... This is generally not a problem, the packet will be retransmitted by the networking layer
  2. Default Ethernet MTU size is 1500bytes (plus we need some more for metadata). Thus, increasing the buffer length from 3200 to 4096 bytes will have almost no effect, as both sizes can fit only 2 datagrams. Increasing it to eg. 8096 will have great effect because now we can send 5 datagrams in one NTB buffer! -> I'd propose increasing the max size in menconfig to 16kB, just to give the users enough flexibility
  3. Funny things is, that with 4 NTBs and 8196 buffers, I got 8Mbit/s. I think this is very close to Bulk FS throughput limit -> we should investigate why are users reporting extremely slow throughput Extremely slow RNDIS throughput with esp-tinyusb (IEC-137) #46
  4. I was also able to reproduce USB-NCM driver 0.17 hangs after a period of sending packets (IEC-243) #107 pretty often, seems to be unrelated to what you are fixing in this PR
  5. Please also check fix(ncm): Use IN buffer for transmit checks tinyusb#48 . It should fix some ugly bugs in the NCM driver

@tore-espressif
Copy link
Collaborator

image

@peter-marcisovsky
Copy link
Collaborator Author

  1. Our WiFi (20-30Mbs/) is faster than USB FS (12Mb/s), so some requests will get 'blocked' just because we cannot send them so fast through USB... This is generally not a problem, the packet will be retransmitted by the networking layer

I am getting request blocked on Linux even with 4 buffs and 8192 buff lengths, but I wouldn't call it an issue either. Did you get rid of the warning message completely on WIndows?

  1. Default Ethernet MTU size is 1500bytes (plus we need some more for metadata). Thus, increasing the buffer length from 3200 to 4096 bytes will have almost no effect, as both sizes can fit only 2 datagrams. Increasing it to eg. 8096 will have great effect because now we can send 5 datagrams in one NTB buffer! -> I'd propose increasing the max size in menconfig to 16kB, just to give the users enough flexibility

Good findings.
Iperf did not get better even after increasing the lenghts to 8192, but I was running Iperf between 2 ESPs, so there could be something else.. or maybe the Iperf settings.

Running speed tests looks like a better idea, I did not think about it. On Linux I am getting 7.5 Mbps Download and 6.1 Mbps Upload with 4 Buffs 8192 lengths.. Maybe bit worse connection in the office? Eitherway, nice improvement comparing to the TInyUSB's default buffs settings where I got only 1.5 Mbps download.

Going to 16384 lengths was for some reason so bad. I was getting huge latency. Can you try 16k buff lengths as well?

  1. Funny things is, that with 4 NTBs and 8196 buffers, I got 8Mbit/s. I think this is very close to Bulk FS throughput limit -> we should investigate why are users reporting extremely slow throughput [Extremely slow RNDIS throughput with esp-tinyusb (IEC-137Extremely slow RNDIS throughput with esp-tinyusb (IEC-137) #46](Extremely slow RNDIS throughput with esp-tinyusb (IEC-137) #46)

I think, those buffer settings we are configuring in this MR, are valid only for NCM device, not for RNDIS device. Maybe there is some different setting for RNDIS which we can configure.

@tore-espressif thank you for the investigations, and idea of using Speed test (so obvious..) I will update the MR based on this findings.

@tore-espressif
Copy link
Collaborator

Did you get rid of the warning message completely on WIndows?

No no, I still got the message (if I enabled it :D ). But it is expected behavior. USB is bottleneck in this data stream

On Linux I am getting 7.5 Mbps Download and 6.1 Mbps Upload with 4 Buffs 8192 lengths.. Maybe bit worse connection in the office?

I doubt it would be so bad in the office. 7.5Mbps is good enough for Full Speed bulk transfer

Going to 16384 lengths was for some reason so bad.

I did not test, will do

are valid only for NCM device, not for RNDIS device

NCM is mentioned in #46 too... in the 'Benchmark' table

@peter-marcisovsky peter-marcisovsky force-pushed the fix/esp_tinyusb_fix_ncm_ntb_buffs_config branch from 3a9f7a6 to ddd359a Compare January 15, 2025 08:56
@tore-espressif
Copy link
Collaborator

@peter-marcisovsky I tested again, and it seems that TinyUSB driver breaks if the buffer are > (10*1024) bytes.

So I'd limit max buffer length to 10240.

@peter-marcisovsky peter-marcisovsky force-pushed the fix/esp_tinyusb_fix_ncm_ntb_buffs_config branch from ddd359a to 80ecd79 Compare January 15, 2025 12:39
@peter-marcisovsky peter-marcisovsky force-pushed the fix/esp_tinyusb_fix_ncm_ntb_buffs_config branch from 851765f to 959e79b Compare January 17, 2025 08:48
@peter-marcisovsky peter-marcisovsky merged commit 85d479b into master Jan 17, 2025
31 checks passed
@peter-marcisovsky peter-marcisovsky deleted the fix/esp_tinyusb_fix_ncm_ntb_buffs_config branch January 17, 2025 09:40
@tore-espressif tore-espressif restored the fix/esp_tinyusb_fix_ncm_ntb_buffs_config branch January 28, 2025 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: In Progress Issue is being worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Number of NTB buffers configuration option (IEC-251)
3 participants