Adjust native memory for ESP32_S3 without PSRAM #3048
Conversation
WalkthroughThe changes in this pull request introduce a new conditional compilation directive for the ESP32S3 target in the Changes
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
targets/ESP32/_nanoCLR/Memory.cpp (1)
28-29: Document the memory requirement changeConsider adding a comment explaining why ESP32S3 requires 130KB of minimum free memory, particularly since it's higher than other variants. This will help future maintenance and prevent accidental modifications.
#elif defined(CONFIG_IDF_TARGET_ESP32S3) +// ESP32S3 requires more minimum free memory after IDF 5.2.3 upgrade when PSRAM is not used #define MINIMUM_FREE_MEMORY_FOR_IDF (130 * 1024)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
targets/ESP32/_nanoCLR/Memory.cpp(1 hunks)
🔇 Additional comments (2)
targets/ESP32/_nanoCLR/Memory.cpp (2)
28-29: Verify the 130KB minimum memory requirement for ESP32S3
The increase to 130KB for ESP32S3 is notably higher than other variants (ESP32H2: 20KB, ESP32C6: 90KB, Others: 100KB). While this aligns with addressing the memory reduction after IDF 5.2.3 upgrade, we should verify this value.
✅ Verification successful
From the search results, I can see that the memory configuration for ESP32S3 is part of a well-structured memory management system where different ESP32 variants have different minimum memory requirements based on their capabilities and constraints. The increase to 130KB for ESP32S3 is justified by the following findings:
-
The memory configuration is target-specific with clear progression:
- ESP32H2: 20KB
- ESP32C6: 90KB
- ESP32S3: 130KB
- Others: 100KB
-
The value is used as
MINIMUM_FREE_MEMORY_FOR_IDFwhich is the minimum memory needed for IDF (native code) operation. -
The implementation includes additional safeguards:
- Extra memory reservations for BLE (30KB) and Thread (20KB) if enabled
- Internal RAM allocation considerations with
INTERNAL_RAM_LEAVE_FREE_FOR_ALLOCATION
-
The memory management system has built-in flexibility to handle both SPIRAM and internal RAM scenarios with appropriate fallback mechanisms.
Memory requirement of 130KB for ESP32S3 is appropriate
The increased memory requirement aligns with the IDF 5.2.3 upgrade needs and follows the established pattern of target-specific memory allocations. The implementation includes proper memory management safeguards and fallback mechanisms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing memory-related tests or configurations specific to ESP32S3
rg -l "ESP32S3.*memory|memory.*ESP32S3" --type cpp --type h
# Look for any related discussions or configurations in build files
rg -l "ESP32S3.*KB|KB.*ESP32S3" --type cmake --type make
Length of output: 120
Script:
#!/bin/bash
# Let's try a broader search for memory configurations in ESP32 targets
rg -l "MINIMUM_FREE_MEMORY_FOR_IDF" --type cpp --type h
# Search for ESP32S3 related configurations
rg "ESP32S3" --type cpp --type h -C 2
# Look for memory-related configurations in ESP32 target
fd Memory.cpp -t f --exec cat {} \;
Length of output: 66178
28-29: Verify memory availability with BLE and Thread features
Since ESP32S3 now requires 130KB minimum free memory, and additional memory is required for BLE (+30KB) and Thread (+20KB) features, we should verify that there's sufficient memory when these features are enabled.
✅ Verification successful
Memory requirements are properly handled for ESP32S3 with BLE and Thread features
The code correctly handles memory requirements by dynamically adding BLE (30KB) and Thread (20KB) memory reserves on top of the base 130KB for ESP32S3. The memory allocation is properly managed through conditional compilation:
- Base ESP32S3 memory requirement: 130KB
- Additional for BLE if enabled: +30KB
- Additional for Thread if enabled: +20KB
- Maximum total when all features enabled: 180KB
This implementation ensures sufficient memory allocation regardless of which features are enabled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for BLE and Thread configurations with ESP32S3
rg "CONFIG_IDF_TARGET_ESP32S3.*HAL_USE_BLE|HAL_USE_THREAD" --type cmake --type cpp
# Look for any memory-related test cases combining these features
rg "TEST.*ESP32S3.*(BLE|THREAD)" --type cpp
Length of output: 1566
Script:
#!/bin/bash
# Check for ESP32S3 specific memory configurations and BLE/Thread related memory adjustments
rg "MINIMUM_FREE_MEMORY|HAL_USE_BLE|HAL_USE_THREAD" targets/ESP32/_nanoCLR/Memory.cpp -A 2
# Check for ESP32S3 specific BLE configurations
rg "CONFIG_IDF_TARGET_ESP32S3.*BLE|HAL_USE_BLE.*ESP32S3" --type cmake
# Check for memory configurations in ESP32S3 platform files
fd "ESP32S3" --type f --exec rg "MINIMUM_FREE_MEMORY|CONFIG_SPIRAM|HAL_USE_BLE|HAL_USE_THREAD" {}
Length of output: 804
Script:
#!/bin/bash
# Check for BLE and Thread memory reserve definitions
rg "BLE_EXTRA_IDF_MEMORY_RESERVE|THREAD_EXTRA_IDF_MEMORY_RESERVE" targets/ESP32/
# Check for SPIRAM configurations for ESP32S3
rg "CONFIG_SPIRAM.*ESP32S3|ESP32S3.*SPIRAM" --type cmake
Length of output: 501
|
Simple change just merging |
josesimoes
left a comment
josesimoes
left a comment
There was a problem hiding this comment.
@AdrianSoundy considering that we're tweaking this for pretty much all series and that this is deep in the code, I was thinking that this should go back to cmakepresets to expose it and make it adjustable at build time without having to touch the code.
|
@josesimoes This is just changing the default value for ESP32_S3 running without psram. So really just for the firmware ESP3_S3. The value still can be set in the build if required. |
3 participants
Description
Since upgrading to IDF 5.2.3 the available native memory has been reduced for ESP32_S3 devices without psram
This change increases native memory to give about the same as before upgrade.
Motivation and Context
Discord discussion
How Has This Been Tested?
Locally run with supplied test program
Types of changes
Checklist
Summary by CodeRabbit