nuttxpr commented on PR #16178: URL: https://github.com/apache/nuttx/pull/16178#issuecomment-2794386884
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR description is pretty good, but could be improved to meet the NuttX requirements more thoroughly. Here's a breakdown and suggestions: **Strengths:** * **Clear Summary:** The summary explains the "what" and "why" effectively. The reference to the STM32H7 implementation is helpful context. * **Impact Description:** Addresses the Kconfig impact. * **Testing Detail:** Provides good detail about the testing environment and procedure, including the test application code. Mentioning debugger usage for verification is excellent. **Weaknesses:** * **Missing Issue References:** If this addresses a specific issue in either NuttX or NuttX Apps, those should be linked. * **Incomplete Impact Assessment:** Several impact categories are unaddressed. Consider the following: * **User Impact:** Even if the answer is NO, explicitly state it. * **Build Impact:** Describing the Kconfig changes constitutes a build impact, so you should say YES and elaborate slightly (e.g., "New Kconfig options are added for the H5 to enable the progmem driver."). * **Hardware Impact:** This definitely impacts hardware (H5 support). Specify YES and detail which H5 chips are supported. * **Documentation Impact:** Does this require documentation updates? If not, state NO. If yes, indicate if the PR includes the documentation changes or if they will be done separately. * **Security Impact:** Even if there's no impact, explicitly say NO. The assumption about non-secure memory access should also be moved here from the summary. * **Compatibility Impact:** Explicitly address backward/forward compatibility. * **Missing "Before/After Logs":** The template asks for logs, but they are not provided. While debugger inspection is mentioned, including relevant snippets of output from the test application (even if it's just "Erase successful," "Write successful") would strengthen the testing section. **Revised PR Description (Example):** ## Summary Implementation of functions defined in `include/nuttx/progmem.h` for the STM32H5. The driver supports a subset of the flash peripheral functions and assumes non-secure memory accesses. It does not check for memory protection mechanisms. This implementation references `arch/arm/src/stm32h7/stm32h743xx_flash.c` with adaptations for the H5's flash register layout. **(Add issue references here if applicable: Relates to NuttX Issue #XXX, NuttX Apps Issue/PR #YYY)** ## Impact * **Is new feature added? Is existing feature changed?** New feature added (progmem support for STM32H5). * **Impact on user (will user need to adapt to change)?** NO. Users will need to enable progmem support in their configuration, but the usage should be as per normal progmem behavior. * **Impact on build (will build process change)?** YES. New Kconfig options are added for the H5 to enable the progmem driver. * **Impact on hardware (will arch(s) / board(s) / driver(s) change)?** YES. Adds support for the STM32H5 family (specifically the STM32H563ZIT6U used in testing; add other supported chips here). * **Impact on documentation (is update required / provided)?** (YES/NO - Indicate whether documentation updates are included or planned). * **Impact on security (any sort of implications)?** NO. However, the driver assumes non-secure memory access and does not incorporate memory protection checks. * **Impact on compatibility (backward/forward/interoperability)?** NO (assuming no breaking changes to the progmem API). * **Anything else to consider?** None. ## Testing I confirm that changes are verified on local setup and works as intended: * **Build Host:** Ubuntu 24.04 (Docker container running on Ubuntu 24.04 host), using Arm GNU Toolchain 13.3.Rel1 (Build arm-13.24). * **Target:** STM32H563ZIT6U on Nucleo-H563ZI board. Testing was performed using a custom application (included in the PR) that writes and erases flash blocks. The flash memory was inspected using a debugger after each step to verify correct operation. **Testing logs before change:** (Not applicable, as this is a new feature.) ``` (Provide some output from the test application, e.g., erase/write success messages. If not possible, explain why). Example: Erase Block 128: Success Write to Block 0: Success ... ``` **Testing logs after change:** ``` Erase Block 128: Success Write to Block 0: Success ... ``` By filling in the missing details and structuring the information according to the template, you'll create a more complete and easier-to-review PR. Remember to replace the placeholder examples with actual information. -- 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
