nuttxpr commented on PR #15483: URL: https://github.com/apache/nuttx/pull/15483#issuecomment-2581600496
[**\[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. This PR mostly meets the NuttX requirements but could be improved in terms of clarity and detail. Here's a breakdown: **Strengths:** * Provides a clear statement of the overall change: adding a BMI088 driver. * Identifies configuration options required to enable the driver. * Lists tested host and compiler. * Confirms testing functionality via device file and uorb interface. **Weaknesses:** * **Summary lacks detail:** * The "why" is weak. State the benefit of adding this driver. Is it for a specific application or general purpose? * The "how it works" is vague. Explain briefly how the driver interfaces with the hardware and software. Mention key data structures or algorithms if applicable. * No related NuttX issues are referenced. If there isn't one, consider creating one to track the feature request. * **Impact section lacks detail:** * While answering "YES" to impacting hardware, it simply restates that the driver is added. Specify *which* hardware (specific architectures, boards) this driver supports. List the SPI controller it uses. Mention any required board configuration changes (e.g., pinmuxing). * "NO" to documentation impact is problematic. At minimum, the Kconfig file needs updating to document the new configuration options. Ideally, there should be some documentation in the docs/ directory explaining how to use the driver. * **Testing section lacks essential information:** * **No "before change" logs:** This makes it impossible to assess the impact of the change. * **"After change" logs are missing crucial data:** What *specific* values were read? Include examples of the output showing the accelerometer and gyroscope readings. This proves the driver is functioning correctly. * **Target(s) missing:** Which architecture and board was this tested on? Just stating "ubuntu20.04" is the build host information; we need the target details. e.g., `sim:nsh`, `stm32f4discovery:nsh` **Recommendations for Improvement:** 1. **Expand the Summary:** Explain the motivation for adding the driver and provide more technical detail about its implementation. 2. **Detail the Hardware Impact:** Specify supported architectures, boards, SPI controller used, and any necessary board configuration updates. 3. **Address Documentation:** Update the Kconfig file and consider adding documentation in the `docs/` directory on how to use the driver. 4. **Provide Complete Testing Logs:** Include "before" logs, and in the "after" logs, show specific accelerometer and gyroscope readings. State the *target* used for testing (architecture, board, configuration). 5. **Link to a NuttX Issue:** If one doesn't exist, create one to track the feature request and link it in the PR. By addressing these weaknesses, the PR will be significantly 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: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
