nuttxpr commented on PR #14768: URL: https://github.com/apache/nuttx/pull/14768#issuecomment-2474104075
[**\[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 it is lacking in testing. While a successful build is a good first step, it doesn't actually test the new register definitions. Here's a breakdown and suggestions for improvement: **Strengths:** * **Clear Summary:** The summary adequately explains the "what" and "why" of the change. * **Impact Assessment:** The impact assessment correctly identifies the limited impact of this foundational change. * **Nxstyle Check:** Performing the Nxstyle check is good practice. **Weaknesses:** * **Insufficient Testing:** Building the `nsh` config doesn't test the register definitions. You need to demonstrate that the defined macros correctly access the ADC registers. This might involve writing a simple test program that reads and/or writes to these registers and verifies the results. * **Missing Target Details:** While you mention `nucleo-h563zi:nsh`, specify your build host details (OS, compiler version, etc.) for reproducibility. * **Empty Test Logs:** The "Testing logs before change" and "Testing logs after change" sections are empty. Even if the functionality isn't directly testable yet, include any relevant build logs or output. Explain *why* testing is limited at this stage. **Recommendations:** 1. **Write a Simple Test:** Create a minimal test program (perhaps in `nuttx/arch/arm/src/stm32h5/stm32_adc_test.c`) that uses the new register definitions. This could involve: * Reading the ADC control registers and printing their values. * Writing known values to certain registers and then reading them back to verify. 2. **Populate Test Logs:** Include the output of your test program (before and after the change). If you can't fully test, explain why and provide any relevant build output. 3. **Complete Build Host Details:** Specify your host OS, compiler version, etc. in the "Testing" section. 4. **Consider a `CONFIG_STM32H5_ADC` Kconfig Option:** For a larger change like adding ADC driver support, a Kconfig option would be appropriate to enable/disable the feature. For this foundational change of just adding register definitions, it's probably not strictly necessary yet. By addressing these points, your PR will be much 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]
