nuttxpr commented on PR #15801: URL: https://github.com/apache/nuttx/pull/15801#issuecomment-2647431110
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the NuttX requirements, although some minor improvements could be made. **Strengths:** * **Clear Summary:** The summary clearly explains the problem, the proposed solution, and the rationale behind it. The mention of potential alternative solutions (larger alignment) and why they were not chosen is helpful. * **Impact Description:** The impact section adequately covers the effects of the change, both in fixing the functional issue and the compile-time error. It notes the limited reproducibility of the original problem. * **Testing Description:** The testing description explains the specific test setup and the observed behavior before and after the change. **Areas for Improvement:** * **Testing Logs:** While the testing *description* is good, actual logs are missing. Include snippets of logs or output demonstrating the incorrect behavior before the change and the correct behavior after the change. Even brief snippets would improve confidence in the testing process. * **Build Host/Target Details:** Be more specific in the Testing section about the build host and target. For example, instead of "SAMv7 custom board," specify the board name and any relevant configuration options. Instead of "Linux," provide the distribution and version. This allows others to reproduce your testing environment more easily. * **Impact - Documentation:** Mention whether documentation updates are required or provided. Since the issue wasn't documented in the manual, consider adding a brief note explaining the limitations/requirements around message RAM placement. * **Impact - Compatibility:** While it seems unlikely, consider if this change might impact backwards compatibility. Does relocating the `.mcan` section affect any existing user configurations or assumptions? Even if the answer is NO, explicitly stating it is helpful. **Example of Improved Testing Section:** ``` Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): Linux (Ubuntu 20.04), GCC 9.4.0 * Target(s): SAMV71-Xplained Ultra (rev B), Configuration: <specify config details> Testing logs before change: ``` <Serial output showing incorrect data being transmitted from address 0x20400000> <Example: CAN ID: 0x123, Data: 0xDEADBEEF (incorrect)> ``` Testing logs after change: ``` <Serial output showing correct data being transmitted from address 0x20410000> <Example: CAN ID: 0x123, Data: 0xCAFEBABE (correct)> ``` By addressing these minor points, the PR would be even stronger and demonstrate a more thorough testing and review process. -- 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
