nuttxpr commented on PR #16218: URL: https://github.com/apache/nuttx/pull/16218#issuecomment-2807304242
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR mostly meets the NuttX requirements, but could be improved. Here's a breakdown: **Strengths:** * Clearly explains the *why* of the change (supporting future DMA driver). * Concisely describes the *how* (adding register definitions). * Addresses most of the impact categories. * Provides some basic testing verification. **Weaknesses & Suggestions for Improvement:** * **Missing Issue References:** Even if there isn't a directly related issue, consider creating one to track the overall DMA driver implementation effort. Referencing this issue in the PR would provide valuable context. * **Incomplete Summary:** The "What functional part of the code is being changed?" is missing. Specify that the change adds files to the `arch/arm/src/stm32h5` directory (or wherever the files are located). Also, explicitly mention the creation of the skeleton DMA header file. * **Impact Overly Simplified:** While marking many impacts as "NA" is fine, providing slightly more detail builds confidence. For example, instead of "Build process: NA", consider "Build process: No changes to the build process are expected at this stage, as the new header file is not yet included in any build targets." This clarifies that the impact is understood, even if it's negligible. * **Testing Insufficient:** While `nxstyle` and basic compilation are good, they don't fully test the functionality of the register definitions. Consider adding a simple test program (even if it doesn't utilize DMA yet) that at least includes the header file and perhaps checks the values of a few defined macros to ensure they are correct. This will prevent regressions if the header file is modified later. Also, specify the build host and target details used for testing (OS, compiler version, etc.). Even "Linux, GCC 12.2, nucleo-h563zi:nsh" is better than nothing. Providing "before" logs is less critical in this specific case since there was no prior functionality, but "after" logs showing the successful compilation output (even just a snippet) would be helpful. **Example of Improved Testing Section:** ``` ## Testing I confirm that changes are verified on local setup and works as intended: * Build Host: Linux, GCC 12.2 * Target: nucleo-h563zi:nsh (simulated on QEMU) Testing logs after change (snippet of successful compilation): ``` ... LINK: nuttx ... ``` A simple test program (stm32h5_dma_test.c) was created to include the new header file and verify a few register definitions. This program compiled and linked without error. ``` By addressing these points, the PR will be stronger and provide more confidence in the changes. -- 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
