nuttxpr commented on PR #14707: URL: https://github.com/apache/nuttx/pull/14707#issuecomment-2465640167
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR description *does not* fully meet the NuttX requirements, although it's closer than many. Here's a breakdown of what's missing and how to improve it: **Missing/Needs Improvement:** * **Summary:** While the provided summary explains the *what*, it lacks the *why*. Why was the naming scheme changed in `board.h`? Was it a bug fix, a standardization effort, or something else? This context is crucial. Also, briefly mention the fix for `HSIDIV` bits not being cleared. * **Impact:** Simply stating "STM32H5 chips" is insufficient. Be explicit about *which* aspects are impacted: * **Impact on user:** Likely NO, unless user code directly relied on the old `STM32H5_BOARD_USExxx` naming. If so, mention that and the required changes. * **Impact on build:** Possibly NO. If only source changes are involved, it's likely NO. * **Impact on hardware:** YES. Specifically, STM32H5 chips. Explain *how* the hardware is impacted. Does it affect functionality, power consumption, or just code organization? * **Impact on documentation:** YES if `board.h` changes require documentation updates. NO otherwise. Be explicit. * **Impact on security:** Potentially YES if the `HSIDIV` issue had security implications (e.g., related to clock glitching attacks). If not, explain why it's NO. * **Impact on compatibility:** Likely YES if the naming change breaks backward compatibility. Explain the extent and any migration steps. * **Testing:** The testing description is vague. Provide: * **Build Host(s):** Be specific: e.g., "Linux Ubuntu 20.04, x86_64, GCC 9.4" * **Target(s):** Be specific: e.g., "STM32H563ZI-Nucleo board with `nsh` configuration" * **Testing logs:** While saying you confirmed with a debugger is good, provide *snippets* of relevant logs or register values. Showing the `HSIDIV` bits before and after the change would be helpful. "Successful boot to the Nuttx shell" is too generic. Demonstrate specific functionality related to the clock changes. Did you test different clock speeds? Show the output of a command that displays clock configuration. **Example of an Improved PR Description:** ```markdown ## Summary This PR addresses inconsistencies in the STM32H5 board configuration and fixes an issue with the `HSIDIV` bits in the `RCC_CR` register. The naming scheme in `board.h` was recently standardized, removing the `STM32H5_` prefix. This change required corresponding updates to the `STM32H5_BOARD_USExxx` variables in `stm32h5xx_rcc.c` for consistency. Additionally, the `HSIDIV` bits were not being cleared before being set, which could lead to unpredictable behavior. This PR ensures the bits are cleared before being written. Finally, the PR allows enabling all three clock sources (HSI, CSI, HSE). Fixes #[NuttX issue number if applicable]. ## Impact * **Impact on user:** NO. Users should not be impacted unless they directly relied on the old `STM32H5_BOARD_USExxx` naming within their applications. * **Impact on build:** NO. Only source code changes are involved. * **Impact on hardware:** YES. This change affects STM32H5 chips. The fix for `HSIDIV` ensures correct clock configuration and prevents potential instability or unexpected behavior. Enabling all three clock sources provides more flexibility in clock configuration. * **Impact on documentation:** YES. The documentation for `board.h` needs to be updated to reflect the naming scheme changes. Updates are included in this PR. * **Impact on security:** NO. While incorrect clock configuration could theoretically have security implications, the `HSIDIV` issue did not pose a direct security vulnerability. * **Impact on compatibility:** YES. This change breaks backward compatibility with code that relied on the old `STM32H5_BOARD_USExxx` naming. Users will need to update their code accordingly. * **Anything else to consider:** None. ## Testing I confirm that changes are verified on my local setup and work as intended: * **Build Host(s):** Linux Ubuntu 22.04, x86_64, GCC 11.3.0 * **Target(s):** STM32H563ZI-Nucleo board with `nsh` configuration **Testing logs before change:** ``` # Previous clock configuration output (showing incorrect HSIDIV value) clock get ... ``` **Testing logs after change:** ``` # Clock configuration output after the change (showing correct HSIDIV value) clock get ... # Demonstrating CSI enabled alongside HSI and PLL1 clock set ... (command to set CSI) clock get ... # Output demonstrating successful boot and operation with different HSIDIV values: nsh> ... (commands and their output) ``` ``` By providing more details and context, you make it easier for reviewers to understand and approve your PR. Remember to replace the placeholders with actual values and logs from your testing. -- 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]
