nuttxpr commented on PR #15438: URL: https://github.com/apache/nuttx/pull/15438#issuecomment-2573750101
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) __Fill In The Commit Message:__ This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary. No, this PR description does not fully meet the NuttX requirements. While it provides a decent overview, it lacks crucial details required for proper review. Here's a breakdown of missing information: * **Summary:** While the "what" is described, the "why" is missing. *Why* is merging linker scripts beneficial? Is it for code size reduction, maintainability, fixing a bug, or something else? The "how" is also vague. Does this involve just copying and pasting, or is there more complex logic involved? * **Impact:** While it mentions modifications to ESP32/S2 boards, it lacks specifics. * **User Impact:** Will users notice any functional changes? Will they need to reconfigure anything? * **Build Impact:** While implied, explicitly state if the build process *itself* changes (beyond just using a different linker script). Do users need to clean their build directory? * **Hardware Impact:** Specify the affected architectures/boards explicitly (e.g., `esp32:esp32devkitc`, `esp32s2:esp32s2_saola_1`). * **Documentation Impact:** Does this require documentation updates? If so, were they made in this PR or a separate one? * **Security Impact:** Even if the answer is NO, explicitly state it. The same applies to Compatibility. * **Testing:** "Internal CI test on all boards" is insufficient. Provide snippets of relevant log output showing the before and after behavior. Ideally, showcase a specific test case that demonstrates the improvement or fix. Also, list the specific build host environment used for testing. **Example of improved description:** ## Summary This PR merges the MCUBoot and Simple Boot linker scripts into a single file to improve maintainability and reduce code duplication. This was achieved by identifying common sections and conditionally including MCUBoot-specific sections based on the `CONFIG_BOOT_MCUBOOT` Kconfig option. This change affects ESP32 and ESP32S2 boards (S3 already uses a unified linker script). Resolves [NuttX Issue #XXXX] (if applicable). ## Impact * **New Feature Added:** No * **Existing Feature Changed:** Yes, linker script handling for ESP32 and ESP32S2. * **User Impact:** NO. Users should not experience any functional changes. * **Build Impact:** NO. The build process remains the same, only the linker script used is changed. * **Hardware Impact:** YES. This affects all ESP32 and ESP32S2 boards, including but not limited to: `esp32:esp32devkitc`, `esp32:esp32_wrover_kit`, `esp32s2:esp32s2_saola_1`, `esp32s2:esp32s2_kaluoya`. * **Documentation Impact:** YES. The linker script documentation has been updated to reflect the changes (included in this PR). * **Security Impact:** NO. * **Compatibility Impact:** NO. This change maintains backward compatibility. * **Anything else to consider:** None. ## Testing I confirm that changes are verified on my local setup and work as intended: * **Build Host:** macOS Ventura, Apple M1 Pro, GCC 12.2 * **Target(s):** `sim:nsh`, `esp32:esp32devkitc`, `esp32s2:esp32s2_saola_1` Testing logs before change (esp32:esp32devkitc - showing flash usage): ``` ... .text 0x40080000 0x19d44 ... ``` Testing logs after change (esp32:esp32devkitc - showing flash usage is similar): ``` ... .text 0x40080000 0x19d44 ... ``` This improved description provides more context, clarifies the impact, and includes more concrete testing information, making it easier for reviewers to assess the PR. Remember to replace placeholders like `[NuttX Issue #XXXX]` and the log output with real data. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
