nuttxpr commented on PR #3061: URL: https://github.com/apache/nuttx-apps/pull/3061#issuecomment-2808851044
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR description does not fully meet the NuttX requirements. While it addresses some aspects, it lacks crucial details. Here's a breakdown: **Missing Information:** * **Summary:** While it links to a related NuttX PR, it doesn't explain *what* this PR itself does. It needs to describe the functional change within `nuttx-apps`. What code is being changed in `nuttx-apps` to accommodate the `nuttx` change? How does this change work? * **Impact:** While it states "no user impact," it needs to explicitly address *all* impact categories with "NO" or "YES" followed by a description if "YES." Even if the answer is "NO," it should be stated explicitly for each category (e.g., "Impact on build: NO"). * **Testing:** The testing description is insufficient. It mentions using `slcan` but doesn't provide any actual logs or specific details about the test setup (host OS, target architecture, board, configuration). "CAN message still arrive properly" is vague; it needs concrete evidence, like timestamps, message IDs, and data before and after the change. It should also clarify what "tools" are affected and how they were tested. **How to improve this PR description:** 1. **Summary:** Explain the specific changes made in `nuttx-apps`. What files were modified? What functions were added or changed? How does this change interact with the corresponding `nuttx` PR? 2. **Impact:** Explicitly address all impact categories (build, hardware, documentation, security, compatibility) with "NO" or "YES" and provide a brief explanation even if the answer is "NO." 3. **Testing:** Provide detailed test logs demonstrating the issue before the change and the correct behavior after the change. Include specific information about the test environment (host OS, target, configuration) and the tools used. Show concrete evidence of the bitrate being set correctly and messages being transmitted/received as expected. Quantify the results if possible (e.g., "Successfully sent and received 1000 CAN messages at 500 kbit/s"). **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.3.0 * Target(s): STM32F429I-DISC1, sim:nsh Testing logs before change (using slcand and candump): ``` # slcand -o -s8 -S 500000 /dev/ttyACM0 # Attempt to set bitrate to 500 kbit/s # candump can0 <no CAN messages received - bitrate setting failed> ``` Testing logs after change: ``` # slcand -o -s8 -S 500000 /dev/ttyACM0 # Set bitrate to 500 kbit/s # candump can0 can0 1F0 [8] 00 11 22 33 44 55 66 77 # CAN messages now received correctly can0 1F1 [8] 88 99 AA BB CC DD EE FF ... ``` The `candump` output demonstrates that before the change, the bitrate was not being set correctly by `slcand`, resulting in no messages received. After the change, the bitrate is set correctly, and CAN messages are received as expected. The tool `slcand` now functions correctly with the updated NuttX ioctl implementation. ``` By providing this level of detail, reviewers can properly assess the PR and ensure it meets the required standards. -- 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
