nuttxpr commented on PR #15332: URL: https://github.com/apache/nuttx/pull/15332#issuecomment-2561345323
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to mostly meet the requirements, but is missing some crucial details. Here's a breakdown of what's good and what needs improvement: **Good:** * **Clear Summary of the Change:** The summary clearly states the purpose and scope of the change (new I2C driver for BL808). * **Impact Summary:** It highlights the improved hardware support, which is the primary impact. * **Testing Methodology:** The testing approach is described, including tools (i2ctool, logic analyzer) and comparison with a known-good implementation (RP2040). Instructions on how to replicate the testing are also provided. **Needs Improvement:** * **Summary (Details):** While the "what" is addressed, the "how" is missing. Briefly describe how the driver interacts with the hardware and any specific techniques used. Mention any relevant NuttX issues, even if there isn't a directly related one. For example, was this driver modeled after another I2C driver in NuttX? * **Impact (Details):** While improved hardware support is mentioned, expand on this. Does this affect any existing drivers or configurations? Are there any new Kconfig options? All the "NO/YES" questions in the template *must* be answered explicitly. Pay particular attention to these: * **Impact on build:** Does the build system need any modifications to support this new driver? New Kconfig options? * **Impact on hardware:** Specifically which BL808 boards are supported? Be explicit. * **Impact on documentation:** At a minimum, the driver should be documented in the code. Does any other documentation need updating (e.g., board configuration files, user guides)? Confirm YES/NO. * **Testing (Details):** The testing description is a good start, but actual log output is *required*. Include snippets showing successful I2C communication before and after the change. Also provide details about the Build Host and Target(s) used for testing. Just saying "Tested by communicating" isn't sufficient. Show the commands used, the expected output, and the actual output. Be specific about the BL808 board and configuration used. **Example of improved testing section:** ``` Testing 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): BL808 (PineCone BL602), `nsh` configuration Testing logs before change: (No I2C driver available) ``` nsh> i2ctool -b 0 -s 0x42 i2ctool: failed to register I2C driver ``` Testing logs after change: ``` nsh> i2ctool -b 0 -s 0x42 # Write 0x12 to register 0x00 nsh> i2ctool -b 0 -r 0x42 -l 1 # Read 1 byte from register 0x00 0x12 ``` ``` By filling in the missing details, the PR will be much stronger and easier to review. This demonstrates due diligence and increases the chances of quick acceptance. -- 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
