nuttxpr commented on PR #15879: URL: https://github.com/apache/nuttx/pull/15879#issuecomment-2671221451
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR does not fully meet the NuttX requirements. While it provides a summary and testing information, it lacks crucial details. Here's a breakdown of what's missing: * **Summary:** While it describes the *what*, it lacks the *why*. Why is MCX-N236 support needed? What use cases does it address? The "how" is also too brief. It lists peripherals but doesn't explain *how* they are implemented (e.g., new drivers, modifications to existing ones). Issue references are missing. * **Impact:** The impact section is too general. While it states "no impact on existing code," it needs to address all the specific points: * **New feature?** Yes, explicitly state this. * **User impact?** Likely yes, as a new architecture and board are available. Even if minimal, this should be mentioned. * **Build impact?** Yes, the build system needs to be aware of the new architecture and board. This should be described. * **Hardware impact?** Yes, obviously, as this supports new hardware. Be more specific. * **Documentation?** Does this PR include documentation for the new architecture and board? If not, it should, and this needs to be called out. * **Security?** Even if there's no *known* security impact, it's good practice to explicitly state that security implications were considered. * **Compatibility?** Likely no impact, but state it explicitly. * **Testing:** "No other testing was performed whatsoever" is a red flag. While booting into nsh is a good start, it's insufficient. At a minimum: * **More Specific Build Host Info:** Provide details like the specific Linux distribution, GCC version, etc. * **More Target Details:** Specify the FRDM-MCXN236 configuration used (e.g., clock speed, memory configuration). * **More Extensive Tests:** Test the listed peripherals (GPIO, PINMUX, CLOCK, LPUART) individually. Provide logs for *each* peripheral test, both before (if applicable) and after the changes. Just booting to nsh doesn't prove these peripherals work correctly. * **Before Logs:** If there were no "before" logs (because the functionality didn't exist), explain that explicitly. Don't just leave the section empty. In short, the PR needs significantly more detail and more thorough testing before it can be considered complete. It needs to convince reviewers that the changes are well-motivated, correctly implemented, and thoroughly tested. -- 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
