nuttxpr commented on PR #16226: URL: https://github.com/apache/nuttx/pull/16226#issuecomment-2808873313
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR does **not** fully meet the NuttX requirements. Here's why: * **Impact is understated:** The addition of a new message queue implementation has a significant impact. The PR author marks impact as N/A, which is incorrect. At minimum, the documentation impact should be YES, as new APIs require documentation. Compatibility may also be affected if existing code uses other message queue implementations and wants to switch. The performance impact also needs to be documented and justified. While a benchmark is provided, a more detailed analysis of the trade-offs would be beneficial. * **Testing is incomplete:** While tests are provided, they don't appear to cover all code paths. Edge cases such as queue full/empty conditions, timeout handling, and interrupt context usage (if supported) should be tested. More details about the test environment are also needed. "sim/nsh, Cortex-M55" isn't sufficient. Specific simulator details and the Cortex-M55 board/configuration used should be listed. The "before change" logs are missing entirely. These logs are crucial for demonstrating that the changes fix or improve an existing issue without introducing regressions. * **Missing information:** The summary mentions Zephyr and FreeRTOS. While useful for context, it doesn't explain *why* the NuttX-specific implementation is needed beyond some general statements. A more concrete comparison (e.g., a table showing feature support and limitations of each) would be helpful in justifying the design choices. The summary also lacks a clear description of *how* the message queue is implemented (e.g., data structures used, synchronization mechanisms). To fully meet the requirements, the PR author needs to: 1. **Expand the Impact section:** Detail the documentation updates, potential compatibility issues, and the reasoning behind the performance trade-offs. 2. **Improve the Testing section:** Provide "before change" logs, test edge cases and error conditions, and specify the exact testing environment. Consider adding tests for each of the new API functions. 3. **Clarify the Summary:** Explain the implementation details of the message queue and provide a more structured comparison with existing NuttX message queue implementations as well as Zephyr and FreeRTOS. Justify the design choices and the need for a new implementation more convincingly. It's helpful to clarify whether this new implementation is meant to replace the existing POSIX and System V queues eventually, or if it's intended to coexist with them and serve different use cases. -- 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
