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(ncm): Changed NTB default value to fix DRAM overflow on esp32s2 #125

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

roma-jam
Copy link
Collaborator

@roma-jam roma-jam commented Jan 21, 2025

Requirements

Default value for NCM NTB buffers were 3/3 of 8192 bytes, which led to DRAM overflow on ESP32S2 during the esp-idf sta2eth example build.
Amount of memory, that could be used to hold NTB does depend on the application logic.

Description

This is a 2/3 PR to fix the problem.

To enable the build of the Network example, default value of the buffers amount for transmission and reception side were changed to 1.

Memory Usage for ESP32S2

Comparison values for examples/network/sta2eth to select the optimal buffer size and count.

Count(In/Out) Size(In/Out) Remain DRAM (Bytes, %) release
3/3 3200/3200 35769 (20,3%) v5.2
3/3 3200/3200 28437 (11,53%) v5.3
3/3 3200/3200 31477 (18.3%) v5.4
3/3 3200/3200 28008 (16,28%) latest

PR plan:

  1. Add all esp-idf releases to the current CI build process (ci(workflow): Added esp-idf releases matrix for examples build #123)
  2. Fix the NCM Driver memory usage on ESP32S2 (this PR)
  3. Add network examples to the CI build process (ci(workflow): Add esp-idf network examples build #124)

Related

Testing

N/A


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.

@roma-jam roma-jam added this to the esp_tinyusb v1.7.1 milestone Jan 21, 2025
@roma-jam roma-jam self-assigned this Jan 21, 2025
@roma-jam roma-jam force-pushed the fix/ncm_buffer_count_default_value branch from 212e73f to e4e5fcf Compare January 21, 2025 10:30
@roma-jam roma-jam changed the title fix(ncm): Changed default value to fix DRAM overflow on esp32s2 fix(ncm): Changed NTB default value to fix DRAM overflow on esp32s2 Jan 21, 2025
@peter-marcisovsky
Copy link
Collaborator

@roma-jam Thanks for this fix. Sorry I missed it.

I did idf.py size on the tusb_ncm example with the NTB buffers configuration from the #114 on esp32s2 and the DRAM is almost full (98.08 %) occupied.

This bug was nearly caught by our CI as well.

@tore-espressif
Copy link
Collaborator

@roma-jam @peter-marcisovsky It seems we were kind of hasty when we set the default NCM buffer sizes...

From my tests, 3 buffers and 3200 size perform well. This allocates 19kB (out of 320kB on S2) compared to current 48kB.

@roma-jam
Copy link
Collaborator Author

@tore-espressif I am on it.
On v5.4 for two buffers the remain DRAM is 31477. I will check for the rest of esp-idf and fill the table.

@peter-marcisovsky
Did it make sense to have more buffers but smaller size, or one buffer but bigger? Was there any difference in speed?

@peter-marcisovsky
Copy link
Collaborator

The network/sta2eth example with 1 NCM buffer with 8192 bytes is now, is now DRAM occupied by 82% on esp32s2. Which still does not leave much free space on s2.

@roma-jam reffer to the MR description with the table about RAM vs Performance.

@roma-jam
Copy link
Collaborator Author

@peter-marcisovsky Thanks, anyway that is a good time to add the example/network to our CI.

Also, the DRAM could vary between the releases and any changes in DRAM, occupied by other parts could lead to overflow.

I think, that 10% free remaining ram is enough.

@tore-espressif WDYT? 10%? 15%? 20%?

@roma-jam roma-jam force-pushed the fix/ncm_buffer_count_default_value branch from e4e5fcf to 4c4de68 Compare January 21, 2025 11:22
@roma-jam
Copy link
Collaborator Author

@tore-espressif nevermind, I checked the remaining DRAM for all releases, and with 3/3 X 3200 it always > 10%.

I think that is enough memory for other modules to manipulate.

@roma-jam roma-jam marked this pull request as ready for review January 21, 2025 11:28
@roma-jam roma-jam force-pushed the fix/ncm_buffer_count_default_value branch from 4c4de68 to a25e028 Compare January 21, 2025 14:17
@roma-jam
Copy link
Collaborator Author

roma-jam commented Jan 21, 2025

@tore-espressif
just tried it with https://www.speedtest.net/:
image

and with my script:
image

UPD: latest master, esp32s3, 3/3 X 3200, DMA mode

And nothing in the debug regarding the errors and/or warnings... :(

@peter-marcisovsky
Copy link
Collaborator

@roma-jam if you are trying to catch the tud_network_can_xmit: request blocked "error" it's acually a debug log, not an error log or warning log.

To be able to see it, I had to change the verbosity level for this message specifically in tinyusb. And it was pretty common to see it for some higher traffic scenarios.

@roma-jam
Copy link
Collaborator Author

@peter-marcisovsky I was aimed for TU_LOG_DRV("(EE) VALIDATION FAILED. WHAT CAN WE DO IN THIS CASE?\n");

@roma-jam roma-jam mentioned this pull request Jan 21, 2025
6 tasks
Copy link
Collaborator

@tore-espressif tore-espressif left a comment

Choose a reason for hiding this comment

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

LGTM

Please don't forget to add release note in the release PR

@roma-jam
Copy link
Collaborator Author

@tore-espressif Already there: #126

@roma-jam roma-jam merged commit 6dd8b4e into master Jan 21, 2025
36 checks passed
@roma-jam roma-jam deleted the fix/ncm_buffer_count_default_value branch January 21, 2025 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants