nuttxpr commented on PR #15098: URL: https://github.com/apache/nuttx/pull/15098#issuecomment-2528226495
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) The PR mostly meets the NuttX requirements, but it has some weaknesses that should be addressed before merging. **Strengths:** * **Clear Summary:** The summaries in both sections explain the purpose and functionality of the changes well. * **Impact Description:** The impact section clearly states the affected components and provides a link to the configuration used for testing. * **Testing Evidence:** The testing section provides detailed information on the hardware setup and the tests performed. Including oscilloscope captures is a great way to demonstrate functionality. * **Cross-Referencing:** Mentioning the STM32H7 driver as a base helps reviewers understand the approach and potentially identify areas for improvement. **Weaknesses:** * **Commit Descriptions in Summary Section:** The commit descriptions in the Summary section should be moved to the commit messages themselves. The Summary section should provide a higher-level overview of the change. * **Missing Information in Impact Section:** The Impact section needs to explicitly address all the points listed in the requirements (build process, documentation, security, compatibility). Even if there's no impact, state "NO" explicitly for each item. * **Limited Testing Logs:** While oscilloscope captures are helpful, they don't replace text-based logs from the NuttX console showing SPI initialization, data transfer, and mmcsd mounting. These logs would provide more concrete evidence of functionality and aid in debugging if issues arise. * **Testing on One Board:** While testing on the Nucleo-H563ZI is a good start, testing on other STM32H5 boards, if available, would increase confidence in the driver's stability and portability. * **Clock Source Selection Clarity:** The description of clock source selection in board.h and stm32h5xx_rcc.c could be more detailed. Explain how the user configures the clock source and provide an example. * **Missing Details on Register Differences:** While the PR mentions register differences, it lacks specifics. It would be beneficial to explicitly list the differing registers and bits and explain how the driver handles these differences. This is particularly important for future maintainability. * **"May Just Work As Is" Comment:** The comment "Therefore this driver may just work as is" raises concerns. The PR should confirm with certainty that the driver works as intended, not just "may" work. **Recommendations for Improvement:** 1. **Revise the Summary Section:** Remove the commit descriptions and focus on a concise overview of the changes. 2. **Complete the Impact Section:** Address all impact points explicitly, even if the answer is "NO." 3. **Add Console Logs:** Include NuttX console logs demonstrating SPI initialization, data transfer, and mmcsd mounting. 4. **Expand Testing:** Test on other STM32H5 boards if possible. 5. **Clarify Clock Source Selection:** Provide a clear explanation and example of how to configure the SPI clock source. 6. **Detail Register Differences:** List the specific registers and bits that differ between STM32H7 and H5 and explain how the driver handles these differences. 7. **Remove Speculative Language:** Replace "may just work as is" with confident statements based on testing results. By addressing these weaknesses, the PR will be significantly stronger and more likely to be accepted. -- 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]
