nuttxpr commented on PR #15161: URL: https://github.com/apache/nuttx/pull/15161#issuecomment-2538859983
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the basic NuttX requirements, but could be improved. While it addresses the key points, it lacks detail in several crucial areas. **Areas for Improvement:** * **Summary:** While the "what" is described, the "why" is missing. *Why* is it necessary to add these QEMU-only drivers? What use case do they enable? How exactly do these drivers work (beyond the linked QEMU documentation)? * **Impact:** The impact section is too brief. While it mentions user impact (simulation with `vcan`), it doesn't address most of the other points. Specifically: * **Build impact:** Does adding these drivers change the build process at all (e.g., new Kconfig options)? * **Hardware impact:** While these are QEMU only *now*, is the intention to eventually support physical cards? If so, that should be mentioned. * **Documentation impact:** Be more specific about the added documentation. Where is it located? What does it cover? * **Other impact categories:** Explicitly state "NO" for any categories that aren't applicable (security, compatibility, etc.) rather than omitting them. This adds clarity. * **Testing:** The testing section is weak. "It works" is insufficient. Provide more specific test cases. What exactly was tested? What commands were used? Show example output demonstrating successful CAN communication. The "missing bus timings" comment also belongs in the impact section (and potentially the summary as a known limitation). Provide full details of your build host (OS version, compiler version, etc.) and the QEMU configurations used. **Example of Improved Testing Section:** ``` Testing: Build Host: Ubuntu 22.04, GCC 11.3.0 Target: * QEMU x86_64, virt machine, NuttX configuration: `configs/qemu-x86_64/nsh` * QEMU ARMv7-A (Cortex-A15), virt machine, NuttX Configuration: `configs/qemu-a15/nsh` Tested using the following steps: 1. Started QEMU with the following command (x86_64 example): `qemu-system-x86_64 -enable-kvm -m 1024 -nographic -net none -serial stdio -drive file=nuttx.img,if=virtio` 2. Within the NuttX NSH console, started the Kvaser CAN driver: `can_kvaser start` 3. On the host system, used `candump` to monitor the `vcan0` interface: `candump vcan0` 4. Within the NuttX NSH console, sent a CAN message: `cansend vcan0 123#1122334455667788` 5. Observed the message correctly received on the host system via `candump`: `vcan0 123 [8] 11 22 33 44 55 66 77 88` Similar tests were performed for the CTU CAN FD driver and on the ARMv7-A target. Known Limitation: Bus timing configuration is not currently implemented and is ignored by the QEMU implementation. This will need to be addressed for support on physical hardware. ``` By adding more detail and addressing the missing information, you can significantly strengthen the PR and make it easier for reviewers to understand and approve. -- 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]
