nuttxpr commented on PR #15987: URL: https://github.com/apache/nuttx/pull/15987#issuecomment-2721431682
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR mostly meets the requirements, but needs minor improvements. **Strengths:** * Clearly explains the "why" and "how" of the change. * Provides context by referencing similar implementations (`sst26.c`). * Explicitly acknowledges the backwards compatibility break. * Lists target and host details for testing. **Areas for improvement:** * **Testing Logs:** The template requests "Testing logs before change" and "Testing logs after change". Even simple logs demonstrating basic functionality before and after the change would significantly improve the PR. Examples: output showing successful initialization, data read/write operations, etc. * **Impact - Documentation:** Since `ramtron_initialize`'s signature changed, the documentation MUST be updated. The PR description should state this explicitly (YES) and ideally, the documentation changes should be included in the PR itself. * **Impact - Hardware:** While the target is specified, be more explicit about _which_ hardware is impacted. "Custom board" isn't sufficient. Mention the specific board name or, if none exists, provide more details about its SPI configuration. The description already mentions STM32H753BIT6 which is good; consider adding explicit mention of SPI4. * **Summary (conciseness):** The first summary section is a bit too detailed for a summary. Keep it shorter and higher level; the details are already present in the rest of the description. For instance, "Adds a device ID parameter to `ramtron_initialize` for multi-device SPI bus support, breaking backwards compatibility." would suffice. * **Related Issues:** If there's a related issue, link it. Even if this change isn't directly tied to an open issue, consider creating one to track the backwards compatibility break. By addressing these points, the PR will be even stronger and easier for reviewers to assess. -- 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
