nuttxpr commented on PR #16011: URL: https://github.com/apache/nuttx/pull/16011#issuecomment-2731694439
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) __Fill In The Commit Message:__ This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary. The provided information is a good start but needs some improvements to fully meet the NuttX PR requirements. Here's a breakdown: **Strengths:** * **Clear Summary:** The summary explains the what, why, and how of the change. * **Impact Description:** Addresses the impact on users and the build process. * **Detailed Testing:** Provides specific testing scenarios, including the hardware setup and expected/observed results. **Weaknesses:** * **Missing Issue References:** No related NuttX or NuttX Apps issue/PR is mentioned. If this is a new feature and there's no corresponding issue, state that explicitly. * **Incomplete Impact Assessment:** Several impact categories are missing: * **Hardware:** While you mention RP2040, be more specific. List *all* affected architectures/boards. Specify if any board configuration changes (e.g., defconfig modifications) are required. * **Documentation:** You say documentation is included. State where it is located (e.g., in the driver's source file or in the docs directory). * **Security:** Even if there are no security implications, explicitly state "NO." * **Compatibility:** Address backward/forward compatibility and interoperability explicitly, even if "NO" is the answer for each. * **"Anything else to consider?"**: This is important for catching unusual edge cases. Even if there's nothing else, state "None." * **Incomplete Testing Information:** * **Build Host:** Details about the build host system are missing. * **Targets:** While RP2040 is mentioned, mention the specific board used (e.g., Raspberry Pi Pico). Consider testing on other boards or the simulator if possible. * **"Testing logs before change":** While not strictly required if it's a new feature, it can still be helpful to show the behavior before the driver was added (e.g., an error message indicating the ADC is not found). * **Two's complement handling:** You've identified an issue with two's complement handling in the `adc` example app. This should probably be addressed in a separate issue/PR, and this PR should mention the limitation and the planned fix. **How to improve the PR description:** 1. **Link to related issues (if applicable):** If there's a corresponding issue, include it. If not, say "No related issue." 2. **Complete the Impact section:** Fill in all the missing parts, even if the answer is "NO." Be very specific about hardware changes, documentation updates, and compatibility. For example: * **Hardware:** "This PR adds support for the ADS1115 ADC on RP2040-based boards (specifically tested on the Raspberry Pi Pico). It requires adding the `CONFIG_ADC_ADS1115` and `CONFIG_I2C_DRIVER` options to the board's defconfig and enabling the I2C0 peripheral." * **Documentation:** "Documentation is provided in the `drivers/adc/ads1115.c` file and a new section will be added to the NuttX ADC documentation." * **Security:** "NO" * **Compatibility:** "Backward compatible. No forward compatibility issues anticipated. No interoperability concerns." * **Anything else to consider?:** "None." 3. **Enhance the Testing section:** * **Build Host:** "Linux (Ubuntu 22.04), x86_64, GCC 11.2.0" (or similar). * **Targets:** "RP2040 (Raspberry Pi Pico)" * **Testing logs before change:** If this is a new feature, consider adding a log snippet showing that the ADC wasn't available before. * **Address the two's complement issue:** Either fix it within this PR or create a separate issue/PR to track it and mention it in this PR's description. By addressing these weaknesses, your PR will be much stronger and easier for the NuttX maintainers to review and merge. Remember, clarity and completeness are crucial for a successful contribution to an open-source project. -- 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
