nuttxpr commented on PR #15834: URL: https://github.com/apache/nuttx/pull/15834#issuecomment-2656275540
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR summary does not adequately meet the NuttX requirements. Here's why and how to fix it: **Missing Information in Summary:** * **Why is this change necessary?** Simply stating "enable SocketCAN utils" doesn't explain the motivation. Are these utils needed for a specific application? Does it fix a bug where they were unavailable before? Does it improve usability? * **What functional part of the code is being changed?** While the paths are given, a more descriptive explanation is needed. Something like "Configuration files for several STM32 boards were modified to enable SocketCAN support." would be better. * **How does the change exactly work?** What specific lines were added/modified in the configuration files? Were any Kconfig options changed? * **Links to related issues:** Are there any associated NuttX or NuttX Apps issues that prompted this change? Even if there aren't, stating "None" clarifies this. **Missing Information in Impact:** * **Is new feature added? Is existing feature changed?** Be explicit. This sounds like a new feature (enabling existing utils), so state that clearly. * **All other impact categories need explicit YES/NO answers followed by descriptions if YES.** Even if there's no impact, stating "NO" for each category demonstrates that these aspects were considered. For example: * Impact on user: YES (Users can now use SocketCAN utils on these boards.) * Impact on build: Potentially YES (If a user previously disabled CAN support, they might need to reconfigure.) Even if you believe it's NO, explain why. * Impact on hardware: Probably NO (unless the changes enable hardware that was previously unused), but explain your reasoning. * Impact on documentation: YES (Documentation should be updated to reflect the availability of SocketCAN on these boards.) Specify where the documentation changes are. * Impact on security: Likely NO, but explain. * Impact on compatibility: Likely NO, but explain. **Missing Information in Testing:** * **Be much more specific about your testing environment.** List the OS, compiler version, etc. for your build host. For your targets, specify the exact board configurations used. Example: * Build Host: Linux (Ubuntu 22.04), GCC 11.2.0 * Targets: nucleo-f722ze (default config), nucleo-g431rb (default config), ... * **Provide *actual* testing logs (or summaries of key information from them).** "Different STM32 board combinations in the CAN network" is insufficient. Show commands used and the output demonstrating that SocketCAN is functioning correctly. What tools did you use to test? `candump`, `cansend`? Demonstrate sending and receiving CAN messages. **Example of an Improved Summary:** ``` ## Summary This change enables SocketCAN utilities on several STM32 Nucleo boards, allowing users to utilize standard SocketCAN tools for CAN communication. This enhances the usability of these boards for CAN-based applications. Configuration files for these boards were modified to enable SocketCAN support. Related NuttX Issue: None Related NuttX Apps Issue/PR: None ## Impact * Is new feature added? YES (SocketCAN utils are now enabled on these boards) * Is existing feature changed? NO * Impact on user: YES (Users can now leverage SocketCAN utilities on these STM32 boards for CAN communication.) * Impact on build: NO (This only changes configuration defaults and should not impact existing builds unless a user had explicitly disabled CAN previously). * Impact on hardware: NO (No hardware changes are introduced; this enables software support for existing hardware.) * Impact on documentation: YES (Documentation will be updated to reflect SocketCAN availability - see the updated board configuration documentation.) * Impact on security: NO (This change does not introduce any new security considerations.) * Impact on compatibility: NO (This change maintains backward compatibility.) ## Testing Build Host: Linux (Ubuntu 22.04), GCC 11.2.0 Targets: nucleo-f722ze (default config), nucleo-g431rb (default config), nucleo-f302r8 (default config), nucleo-f446re (default config), b-g431b-esc1 (default config) Testing logs before change (on nucleo-f722ze - similar results observed on other boards): ``` $ candump can0 (No output - candump doesn't work because SocketCAN isn't enabled) ``` Testing logs after change (on nucleo-f722ze - similar results observed on other boards): ``` $ cansend can0 123#1122334455667788 $ candump can0 can0 123 [8] 11 22 33 44 55 66 77 88 ``` (Repeat similar tests demonstrating communication between different boards) ``` By providing this level of detail, reviewers can understand the change, its impact, and verify that it has been properly tested. This significantly increases the chances of your PR being 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
