nuttxpr commented on PR #16303: URL: https://github.com/apache/nuttx/pull/16303#issuecomment-2847202403
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the basic NuttX requirements, but it lacks crucial details. While the *general idea* is presented, the PR description needs significant expansion to be truly acceptable. Here's a breakdown of what's missing and how to improve it: **Summary:** * **Too vague:** "adds the necessary code and function" - What is the name of the function? What code was added specifically? Mentioning file names is very helpful. Example: "Adds the `up_rstcause()` function to `arch/arm/src/sama5/sama5d2_reset.c` to retrieve the reset cause." * **Missing detail:** How does the reset cause determination work? Does it read registers? Which ones? Provide a brief technical overview. **Impact:** * **While technically true,** stating "no impact" should be thoroughly justified. Are you sure there are no subtle interactions with other parts of the system? Even if there are no breaking API changes or build procedure changes, the newly read hardware registers or interrupt configurations *could* have side effects. Even if this is considered unlikely, explain why you are so sure. * **Specify Architecture:** Explicitly state the architecture (e.g., ARM: SAMA5). "This arch" is not precise enough. **Testing:** * **Insufficient detail on build host:** Be specific. "Linux, Ubuntu 20.04, GCC 9.4.0" is a good example. Don't just say "Linux." * **Insufficient detail on target:** "custom board" is too vague. Even if it's custom, provide details: "Custom board based on SAMA5D27C-D1G, configured with X, Y, and Z enabled in the defconfig." * **No logs provided:** The template clearly asks for "Testing logs before change" and "Testing logs after change." Include actual output demonstrating the functionality. Show how the reset cause is reported in NXboot. Show the different reset causes being detected correctly. Don't just say "correctly determined and reported." *Show* it. **Example of Improved Description:** ``` ## Summary This PR adds support for determining the reset cause on the SAMA5D2 architecture. It introduces the `up_rstcause()` function in `arch/arm/src/sama5/sama5d2_reset.c`. This function reads the Reset Status Register (RSTC_SR) to identify the reason for the last reset (power-on, watchdog, software, etc.). The returned value is compatible with the `reset_reason_t` enum. ## Impact * **New Feature:** Provides a `up_rstcause()` implementation for the SAMA5D2 architecture. * **Impact on user:** No. Applications using the `up_rstcause()` API will now have this functionality on SAMA5D2. No changes are required by existing users. * **Impact on build:** No. * **Impact on hardware:** This change affects SAMA5D2-based boards. It reads the RSTC_SR register, which should not have any side effects on normal operation. * **Impact on documentation:** The `up_rstcause()` documentation will be updated to reflect SAMA5D2 support. (Documentation changes included in this PR). * **Impact on security:** No. * **Impact on compatibility:** Backward compatible. No impact on forward or interoperability. ## Testing I confirm that changes are verified on my local setup and work as intended: * Build Host: Linux, Ubuntu 22.04, GCC 11.3.0 * Target: Custom board based on SAMA5D27C-D1G, configured with `sama5d27c_som1_ek_defconfig`. NXboot enabled. Testing logs before change (NXboot output after power cycle): ``` NXBOOT: Starting... ... (no reset cause information available) ``` Testing logs after change (NXboot output after power cycle): ``` NXBOOT: Starting... Reset Cause: Power-on Reset ... ``` Testing logs after change (NXboot output after software reset): ``` NXBOOT: Starting... Reset Cause: Software Reset ... ``` ``` By providing concrete details and evidence, you'll greatly increase the chances of your PR being reviewed and merged quickly. -- 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-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
