nuttxpr commented on PR #16279: URL: https://github.com/apache/nuttx/pull/16279#issuecomment-2836029611
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR mostly meets the NuttX requirements but needs a few improvements: * **Summary:** While the summary explains the "what" and "why," it lacks clarity on the "how." Specify precisely *which* files were modified and *how* the stack size was changed (e.g., modified Kconfig, direct code change). Removing the comment about not adding the `usbhost_stacksize` configuration to board-specific Kconfigs in this commit and instead creating a follow-up issue/PR for that is a good approach. Keep the summaries concise and focused on this PR's specific changes. * **Impact:** While the "Impact on user" section is good, explicitly stating "NO" for the other impacts (documentation, build, security, compatibility) is preferred instead of relying on the bot comment. This ensures clarity and avoids potential misinterpretations. * **Testing:** The testing section is insufficient. The screenshot shows the stack usage *after* increasing the size, but lacks crucial information: * **Before-change logs:** Demonstrating the stack overflow *before* the change is essential. This could be a crash log, a backtrace, or other evidence of the problem. * **After-change logs:** While the screenshot shows the stack isn't overflowing *now*, it doesn't confirm that USB host functionality works correctly. Include logs showing successful USB host operation (e.g., enumeration, data transfer) *after* the change. * **Specifics:** "OS: yes, CPU: yes, compiler: yes" is unhelpful. Provide the actual OS, CPU, and compiler versions used for testing. Also, specify the NuttX version. Mentioning that testing was performed on a nucleo-STM32H743zi and custom STM32H753zi hardware is good, but ideally, test on at least one officially supported board if possible. If not possible, justify why. **Suggested improvements:** * **Summary:** "Changed the USB host stack size to 2048 for affected STM32H7 boards by modifying [files_changed.c/h] This fixes stack overflows reported in #16256 which occurred when inserting a USB device. A follow-up issue/PR will address adding `usbhost_stacksize` to board-specific Kconfigs." * **Impact:** * Impact on user: YES, STM32H7 users will no longer experience memory corruption when using USB host. * Impact on build: NO * Impact on hardware: YES, affects all STM32H7 boards. * Impact on documentation: NO * Impact on security: NO * Impact on compatibility: NO * **Testing:** * Build Host: [OS name and version], [CPU architecture], [Compiler name and version] * Target: STM32H7 (nucleo-STM32H743zi, custom STM32H753zi), NuttX version [version number] * Testing logs before change: [Provide logs/backtrace/evidence of stack overflow crash] * Testing logs after change: [Provide logs demonstrating successful USB host enumeration and operation (e.g., lsusb output, data transfer logs)] By addressing these points, the PR will be much 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
