nuttxpr commented on PR #15906: URL: https://github.com/apache/nuttx/pull/15906#issuecomment-2681290580
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR mostly meets the NuttX requirements, but it could be improved in a few key areas: **Strengths:** * **Clear Summary:** The summary explains the what and why of the change effectively. * **Impact Assessment:** Addresses all the required impact points. * **Testing Provided:** Testing instructions and example output are included. The addition of the write test is good. **Weaknesses & Suggestions for Improvement:** * **Summary - Missing Issue References:** If there are related NuttX or NuttX Apps issues, link them. Even if there aren't directly related issues, consider creating one to track the feature request for I2C slave support if it didn't exist before. * **Testing - Insufficient Detail:** * **Build Host(s):** You mention using `make`, implying a Linux/macOS environment, but be explicit about the OS and compiler version used for testing. This helps reproduce your results. Example: `Linux, Ubuntu 22.04, GCC 11.3.0` * **Target(s):** Be more specific about which ESP32 variant you tested with each command. For instance: "Tested on ESP32-C3-DevKitM-1 with `esp32c3-devkit:i2c` config." Also, if you tested with `esp32c3-generic:i2c`, specify *which* generic board. * **Logs Before Change:** You haven't included logs from *before* the change. These are essential to demonstrate that the functionality didn't exist previously and that your change introduced it. Show the output of the `i2c` commands on the slave devices *before* your I2C slave implementation. This will probably show an error or no response. * **More Comprehensive Testing:** While you demonstrate basic read and write, test other aspects of I2C slave mode: * **Different Data Sizes:** Test with various data lengths, both small and large. * **Repeated Starts/Stops:** Ensure the driver handles repeated start and stop conditions correctly. * **Error Handling:** Test what happens if the master sends an invalid command or the slave encounters an error during a transfer. How are errors reported to the application? * **Slave Address Conflicts:** What happens if two slaves have the same address? * **Bitbang Driver Testing:** While you mention it, provide specific commands and logs for the bitbang driver testing. Don't just say "apply the same command"—show the actual commands and results. * **Documentation - Be Specific:** You say documentation is updated. Be precise: "Updated the `esp32c3.txt`, `esp32c6.txt`, `esp32h2.txt`, `esp32s2.txt`, and `esp32s3.txt` board documentation files to include details on I2C slave mode configuration and usage." By addressing these weaknesses, you can make your PR stronger and easier for reviewers to accept. Remember, clarity, completeness, and reproducibility are key to a successful PR. -- 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
