nuttxpr commented on PR #15753: URL: https://github.com/apache/nuttx/pull/15753#issuecomment-2633595231
[**\[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 it lacks some important details. While it addresses the core questions, it's too brief and lacks the specificity expected in a good PR. Here's a breakdown of what's missing and how to improve it: **Missing/Needs Improvement:** * **Summary:** While the "what" is explained, the "why" for the stack size increase is missing. *Why* has NSH stack usage grown significantly? This is crucial context. The summary should also explicitly state that this is a fix for a broken build (implied, but should be stated). * **Impact:** While CI time reduction is mentioned, quantify it if possible (e.g., "Reduces CI time by approximately X minutes"). The "make mpfs bootable again" should be more descriptive: "Fixes a regression that prevented MPFS targets from booting after [commit hash or PR #]." Address all other impact categories explicitly with "NO" or a detailed "YES" explanation. Even if the answer is "NO," explicitly stating it ensures reviewers don't have to guess. * **Testing:** This is the weakest part. "Tested on Microchip MPFS Icicle board" is insufficient. Provide: * **Specific Build Host Details:** OS version, compiler version (including minor version), any relevant build tools. e.g., "Ubuntu 22.04, GCC 11.3.0, make 4.3" * **Specific Target Details:** MPFS Icicle board revision, NuttX configuration used (e.g., `nsh`), any specific hardware setup. * **Actual Test Logs (Before & After):** These are *required*. Show the boot process failing before the change and succeeding after. Demonstrate the improved stack usage (e.g., output of a stack analysis tool or demonstration of previously failing functionality now working). Even a simple "booted successfully" message in the logs is better than nothing. * **Related Issues:** Link to any related NuttX or NuttX Apps issues/PRs that prompted this change. **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, GCC 11.3.0, make 4.3 * Target(s): Microchip MPFS Icicle Kit rev. A, configurations: nsh, hwtest Testing logs before change (nsh config): ``` <Paste logs showing boot failure or NSH stack overflow> ``` Testing logs after change (nsh config): ``` <Paste logs showing successful boot and NSH functioning correctly> NuttShell (NSH) nsh> free <Output of free command showing adequate stack space> ``` Testing logs before change (hwtest config - demonstrating network/USB functionality now present): ``` <Paste logs showing missing network/USB functionality> ``` Testing logs after change (hwtest config): ``` <Paste logs showing network/USB functionality working correctly> ``` ``` By providing more detail and specifically addressing all the requirements, the PR will be much easier to review and merge. This thoroughness demonstrates that the changes are well-tested and understood, increasing confidence in their correctness. -- 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
