nuttxpr commented on PR #15351: URL: https://github.com/apache/nuttx/pull/15351#issuecomment-2562977371
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) The PR mostly meets the NuttX requirements, but needs improvement in providing specific details. Here's a breakdown: **Strengths:** * **Clear Summary of the Change:** The summary explains what was changed, why, and how. * **Impact Assessment Started:** The impact section begins to address the scope of the change. * **Testing Evidence Provided:** Logic analyzer captures and a functional test log demonstrate effort in verifying the changes. **Weaknesses and Required Improvements:** * **Incomplete Impact Assessment:** The impact section is too brief. It needs to specifically address *all* the points in the template. For example: * **Impact on user:** Even if the answer is NO, explicitly state "NO". If QSPI wasn't available before, this IS a change for the user and needs describing. * **Impact on build:** Will any Kconfig options need to be added/modified to enable this driver? If so, explain how. * **Impact on hardware:** Be specific. "STM32H5" is a family. List the specific chip(s) tested. Mention any board(s) used. * **Impact on documentation:** If documentation updates are needed (they likely are!), mention that they will be provided in a separate commit or are included in the PR. If not needed, explicitly state "NO". * **Impact on security, compatibility, etc.:** Address *all* impact points, even if the answer is "NO." Explicitly stating "NO" clarifies that these aspects were considered. * **Missing Build/Target Details (Testing):** The testing section lacks crucial information. Provide the following: * **Host OS:** e.g., Ubuntu 22.04 * **Host Compiler:** e.g., GCC 11.2.0 * **Target Architecture/Board/Configuration:** e.g., STM32H563ZI, Nucleo-H563ZI, `nsh` configuration * **Before/After Logs:** The attached text file is good, but ideally, include relevant snippets directly in the PR description. Show *specific* commands and their output *before* and *after* the change, highlighting the differences. What happens if QSPI is used before this change? An error? What happens after? Success? * **Missing "Before" Test Logs:** While "after" logs are present, there are no explicit "before" logs demonstrating the previous behavior (e.g., the lack of QSPI functionality or errors encountered). This makes it harder to assess the effectiveness of the change. **Example of Improved Testing Section:** ``` I confirm that changes are verified on local setup and works as intended: * Build Host(s): Ubuntu 22.04, x86_64, GCC 11.2.0 * Target(s): STM32H563ZI, Nucleo-H563ZI board, `nsh` configuration Testing logs before change (QSPI not implemented): ``` nsh> qspi_test ERROR: QSPI driver not initialized ``` Testing logs after change (QSPI functional): ``` nsh> qspi_test QSPI initialized successfully. W25QXX ID: 0xEF4018 NXFFS mounted successfully. Wrote 1024 bytes to NXFFS. Read 1024 bytes from NXFFS. Data verified. ``` [Full test log](https://github.com/user-attachments/files/18254399/qspi_func_test.txt) <-- Still link to full log ``` By addressing these weaknesses and providing more specific details, the PR will better meet the NuttX requirements and significantly improve its reviewability. Remember, the goal is to make it as easy as possible for reviewers to understand and verify 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
