You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "tim-sony (via GitHub)" <gi...@apache.org> on 2025/02/18 06:55:27 UTC
[PR] feat(arch/xensa): add CONFIG_SPIRAM_MEMTEST from ESP-IDF [nuttx]
tim-sony opened a new pull request, #15855:
URL: https://github.com/apache/nuttx/pull/15855
*Note: Please adhere to [Contributing Guidelines](https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md).*
## Summary
A trivial change. Considering the CONFIG item is from [the ESP-IDF performance tuning guide,](https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-guides/performance/speed.html#improving-startup-time), it would be beneficial to include it in Nuttx as well. Begin by adding it to 'arch/xtensa', which currently lacks it.
## Impact
- One more option for startup performance tuning.
## Testing
- build against
- esp32-devkitc:psram
- _Skipped ESP32S2 since no existing board configure has SPIRAM enabled._
- esp32s3-devkit:psram_quad
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(arch/xensa): add CONFIG_SPIRAM_MEMTEST from ESP-IDF [nuttx]
Posted by "nuttxpr (via GitHub)" <gi...@apache.org>.
nuttxpr commented on PR #15855:
URL: https://github.com/apache/nuttx/pull/15855#issuecomment-2664771979
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues)
Yes, this PR appears to mostly meet the NuttX requirements, but is missing some key information.
Here's a breakdown of what's good and what needs improvement:
**Strengths:**
* **Clear Summary:** The summary explains the "why," "what," and "how" of the change. The link to the ESP-IDF guide provides good context.
* **Concise Impact Description:** The impact description is brief but gets the point across.
* **Testing Mentions Targets:** The testing section identifies specific targets.
**Weaknesses:**
* **Missing Issue References:** If this relates to any existing NuttX issues, those should be linked in the Summary.
* **Impact Section Incomplete:** Several "Impact" fields are left blank (build, hardware, documentation, security, compatibility). Even if the answer is "NO," it should be explicitly stated. The current description focuses solely on the user impact and ignores potential impacts on other aspects.
* **Insufficient Testing Detail:** While targets are mentioned, there are no actual logs provided. "Before" and "after" logs demonstrating the change are crucial. Furthermore, just stating "build against" isn't sufficient. What was the observed behavior? Did the change have the intended effect? Quantify the improvement if possible. The rationale for skipping ESP32S2 should be in the Impact section (hardware impact).
* **Build Host Information Missing:** The testing section lacks information about the build host used (OS, CPU, compiler).
* **Missing Contributing Guidelines Adherence Confirmation:** While the note is there, there's no explicit statement confirming adherence to the guidelines.
**Recommendations for Improvement:**
1. **Complete the Impact Section:** Explicitly address *all* impact categories, even if the answer is "NO." Justify the "NO" if necessary. For example, if there's no documentation impact, state "Impact on documentation: NO. This change does not require documentation updates." Explain the ESP32S2 exclusion under hardware impact.
2. **Provide Detailed Testing Logs:** Include actual "before" and "after" logs demonstrating the effect of the change. Quantify the improvement in startup time if possible. Show the commands used for building and testing.
3. **Include Build Host Information:** Specify the OS, CPU, and compiler version used for the build.
4. **Link Related Issues (if any):** If no issues exist, explicitly state that.
5. **Explicitly Confirm Adherence to Contributing Guidelines:** Add a statement like "I confirm that this PR adheres to the NuttX Contributing Guidelines."
By addressing these points, the PR will be significantly stronger and easier to review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(arch/xensa): add CONFIG_SPIRAM_MEMTEST from ESP-IDF [nuttx]
Posted by "tim-sony (via GitHub)" <gi...@apache.org>.
tim-sony commented on PR #15855:
URL: https://github.com/apache/nuttx/pull/15855#issuecomment-2667373132
> Please run a sanity check on S2. If you don't have one available I can trigger internal CI so we can be sure.
Thanks for your comment, @fdcavalcanti! Since we don't have an S2 board, **how about de-scoping S2 from this PR change?** This would account for the additional action required for the internal CI job.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(arch/xensa): add CONFIG_SPIRAM_MEMTEST from ESP-IDF [nuttx]
Posted by "fdcavalcanti (via GitHub)" <gi...@apache.org>.
fdcavalcanti commented on PR #15855:
URL: https://github.com/apache/nuttx/pull/15855#issuecomment-2669031178
CI is ok, all good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(arch/xensa): add CONFIG_SPIRAM_MEMTEST from ESP-IDF [nuttx]
Posted by "sdc-g (via GitHub)" <gi...@apache.org>.
sdc-g commented on code in PR #15855:
URL: https://github.com/apache/nuttx/pull/15855#discussion_r1959211389
##########
arch/xtensa/src/esp32s2/esp32s2_start.c:
##########
@@ -404,7 +404,13 @@ static void noreturn_function IRAM_ATTR __esp32s2_start(void)
else
{
esp_spiram_init_cache();
- esp_spiram_test();
+# if defined(CONFIG_ESP32S2_SPIRAM_BOOT_INIT)
+ if (esp_spiram_test() != OK)
Review Comment:
ESP32S2_SPIRAM_MEMTEST?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(arch/xensa): add CONFIG_SPIRAM_MEMTEST from ESP-IDF [nuttx]
Posted by "tim-sony (via GitHub)" <gi...@apache.org>.
tim-sony commented on code in PR #15855:
URL: https://github.com/apache/nuttx/pull/15855#discussion_r1959306469
##########
arch/xtensa/src/esp32s2/esp32s2_start.c:
##########
@@ -404,7 +404,13 @@ static void noreturn_function IRAM_ATTR __esp32s2_start(void)
else
{
esp_spiram_init_cache();
- esp_spiram_test();
+# if defined(CONFIG_ESP32S2_SPIRAM_BOOT_INIT)
+ if (esp_spiram_test() != OK)
Review Comment:
Thanks @sdc-g review! Yes, a wrong CONFIG name was used here. I've corrected it. Please take another look!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(arch/xensa): add CONFIG_SPIRAM_MEMTEST from ESP-IDF [nuttx]
Posted by "tim-sony (via GitHub)" <gi...@apache.org>.
tim-sony commented on PR #15855:
URL: https://github.com/apache/nuttx/pull/15855#issuecomment-2670521721
> CI is ok, all good.
Thank you so much! @fdcavalcanti
Let's wait for our maintainers to take a final look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(arch/xensa): add CONFIG_SPIRAM_MEMTEST from ESP-IDF [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #15855:
URL: https://github.com/apache/nuttx/pull/15855
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(arch/xensa): add CONFIG_SPIRAM_MEMTEST from ESP-IDF [nuttx]
Posted by "fdcavalcanti (via GitHub)" <gi...@apache.org>.
fdcavalcanti commented on PR #15855:
URL: https://github.com/apache/nuttx/pull/15855#issuecomment-2668325731
> > Please run a sanity check on S2. If you don't have one available I can trigger internal CI so we can be sure.
>
> Thanks for your comment, @fdcavalcanti! Since we don't have an S2 board, **how about de-scoping S2 from this PR change?** This would account for the additional action required for the internal CI job.
Better to leave it there. I'll trigger a CI run just for the sake of procedure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(arch/xensa): add CONFIG_SPIRAM_MEMTEST from ESP-IDF [nuttx]
Posted by "tim-sony (via GitHub)" <gi...@apache.org>.
tim-sony commented on code in PR #15855:
URL: https://github.com/apache/nuttx/pull/15855#discussion_r1959306469
##########
arch/xtensa/src/esp32s2/esp32s2_start.c:
##########
@@ -404,7 +404,13 @@ static void noreturn_function IRAM_ATTR __esp32s2_start(void)
else
{
esp_spiram_init_cache();
- esp_spiram_test();
+# if defined(CONFIG_ESP32S2_SPIRAM_BOOT_INIT)
+ if (esp_spiram_test() != OK)
Review Comment:
Thanks @sdc-g! Yes,I used the wrong CONFIG name here. I've corrected it. Please take another look!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(arch/xensa): add CONFIG_SPIRAM_MEMTEST from ESP-IDF [nuttx]
Posted by "fdcavalcanti (via GitHub)" <gi...@apache.org>.
fdcavalcanti commented on PR #15855:
URL: https://github.com/apache/nuttx/pull/15855#issuecomment-2665492304
Looks good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org