Re: [PR] nuttx/msgq: add kernel message queue support [nuttx]
anchao commented on code in PR #16226:
URL: https://github.com/apache/nuttx/pull/16226#discussion_r2049203623
##
include/nuttx/msgq.h:
##
@@ -0,0 +1,315 @@
+/
+ * include/nuttx/msgq.h
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ /
+
+#ifndef ___INCLUDE_NUTTX_MSGQ_H
+#define ___INCLUDE_NUTTX_MSGQ_H
+
+/
+ * Included Files
+ /
+
+#include
+#include
+#include
+#include
+
+/
+ * Pre-processor Definitions
+ /
+
+/
+ * Name: NXMSGQ_INITIALIZER
+ *
+ * Description:
+ * Statically define and initialize a message queue.
+ *
+ * The message queue's ring buffer contains space for max_msgs messages,
+ * each of which is msg_size bytes long.
+ *
+ * Input Parameters:
+ * buffer - Ring buffer of Message Queue
+ * msg_size - Message size (in bytes).
+ * max_msgs - Maximum number of messages that can be queued.
+ *
+ /
+
+#ifdef CONFIG_SMP
+# define NXMSGQ_INITIALIZER(buffer, msg_size, max_msgs) \
+{ \
+ CIRCBUF_INITIALIZER(buffer, msg_size * max_msgs), \
+ msg_size, \
+ NXSEM_INITIALIZER(0, 0), \
+ NXSEM_INITIALIZER(0, 0), \
+ false, \
+ SP_UNLOCKED, \
+}
+#else
+# define NXMSGQ_INITIALIZER(buffer, msg_size, max_msgs) \
+{ \
+ CIRCBUF_INITIALIZER(buffer, msg_size * max_msgs), \
+ msg_size, \
+ NXSEM_INITIALIZER(0, 0), \
+ NXSEM_INITIALIZER(0, 0), \
+ false, \
+}
+#endif
+
+/
+ * Public Type Declarations
+ /
+
+/* This structure defines a kernel message queue */
+
+typedef struct nxmsgq
+{
+ struct circbuf_s cbuf;
Review Comment:
Yes, `list` is a good suggestion. I will improve it if we want to implement
a priority queue in the future, However in this stage, if we only compare the
API with freertos/zephyr, the current data type already meets the requirements.
`max_msgs` is the behavior expected by users. Most kernel developers expect
that the queue is within a controllable range, which will help them find
problems in advance.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] nuttx/msgq: add kernel message queue support [nuttx]
anchao commented on PR #16226: URL: https://github.com/apache/nuttx/pull/16226#issuecomment-2813564551 let me mark this PR as a draft first . I want to support the priority queue and zero-copy in this pull request. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] nuttx/msgq: add kernel message queue support [nuttx]
anchao commented on code in PR #16226:
URL: https://github.com/apache/nuttx/pull/16226#discussion_r2049210554
##
include/nuttx/msgq.h:
##
@@ -0,0 +1,315 @@
+/
+ * include/nuttx/msgq.h
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ /
+
+#ifndef ___INCLUDE_NUTTX_MSGQ_H
+#define ___INCLUDE_NUTTX_MSGQ_H
+
+/
+ * Included Files
+ /
+
+#include
+#include
+#include
+#include
+
+/
+ * Pre-processor Definitions
+ /
+
+/
+ * Name: NXMSGQ_INITIALIZER
+ *
+ * Description:
+ * Statically define and initialize a message queue.
+ *
+ * The message queue's ring buffer contains space for max_msgs messages,
+ * each of which is msg_size bytes long.
+ *
+ * Input Parameters:
+ * buffer - Ring buffer of Message Queue
+ * msg_size - Message size (in bytes).
+ * max_msgs - Maximum number of messages that can be queued.
+ *
+ /
+
+#ifdef CONFIG_SMP
+# define NXMSGQ_INITIALIZER(buffer, msg_size, max_msgs) \
+{ \
+ CIRCBUF_INITIALIZER(buffer, msg_size * max_msgs), \
+ msg_size, \
+ NXSEM_INITIALIZER(0, 0), \
+ NXSEM_INITIALIZER(0, 0), \
+ false, \
+ SP_UNLOCKED, \
+}
+#else
+# define NXMSGQ_INITIALIZER(buffer, msg_size, max_msgs) \
+{ \
+ CIRCBUF_INITIALIZER(buffer, msg_size * max_msgs), \
+ msg_size, \
+ NXSEM_INITIALIZER(0, 0), \
+ NXSEM_INITIALIZER(0, 0), \
+ false, \
+}
+#endif
+
+/
+ * Public Type Declarations
+ /
+
+/* This structure defines a kernel message queue */
+
+typedef struct nxmsgq
+{
+ struct circbuf_s cbuf;
+ int msg_size;
+ sem_ttxsem;
+ sem_trxsem;
+ bool alloc;
+#ifdef CONFIG_SMP
+ spinlock_t lock;
+#endif
+} nxmsgq_t;
+
+/
+ * Public Function Prototypes
+ /
+
+#ifdef __cplusplus
+#define EXTERN extern "C"
+extern "C"
+{
+#else
+#define EXTERN extern
+#endif
+
+/
+ * Name: nxmsgq_init
+ *
+ * Description:
+ * Initialize a message queue with static ring buffer
+ *
+ * This routine initializes a message queue object, prior to its first use.
+ *
+ * The message queue's ring buffer must contain space for max_msgs
+ * messages, each of which is msg_size bytes long. Alignment of the
+ * message queue's ring buffer is not necessary.
+ *
+ * Input Parameters:
+ * msgq - Address of the message queue.
+ * buffer - Pointer to ring buffer that holds queued messages.
+ * msg_size - Message size (in bytes).
+ * max_msgs - Maximum number of messages that can be queued.
+ *
+ * Returned Value:
+ * None.
+ *
+ /
+
+void nxmsgq_init(FAR nxmsgq_t *msgq, FAR void *buffer,
+ size_t msg_size, uint32_t max_msgs);
+
+/
+ * Name: nxmsgq_create
+ *
+ * Description:
+ * Create a message queue.
+ *
+ * This routine initializes a message queue object, prior to its first use,
+ * allocating its internal ring buffer from the calling thread's resource
+ * pool.
+ *
+ * Memory allocated for the ring buffer can be released by calling
+ * nxmsgq_destroy()
+ *
+ * Input Parameters:
+ * msg_size - Message
Re: [PR] nuttx/msgq: add kernel message queue support [nuttx]
anchao commented on code in PR #16226:
URL: https://github.com/apache/nuttx/pull/16226#discussion_r2049203623
##
include/nuttx/msgq.h:
##
@@ -0,0 +1,315 @@
+/
+ * include/nuttx/msgq.h
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ /
+
+#ifndef ___INCLUDE_NUTTX_MSGQ_H
+#define ___INCLUDE_NUTTX_MSGQ_H
+
+/
+ * Included Files
+ /
+
+#include
+#include
+#include
+#include
+
+/
+ * Pre-processor Definitions
+ /
+
+/
+ * Name: NXMSGQ_INITIALIZER
+ *
+ * Description:
+ * Statically define and initialize a message queue.
+ *
+ * The message queue's ring buffer contains space for max_msgs messages,
+ * each of which is msg_size bytes long.
+ *
+ * Input Parameters:
+ * buffer - Ring buffer of Message Queue
+ * msg_size - Message size (in bytes).
+ * max_msgs - Maximum number of messages that can be queued.
+ *
+ /
+
+#ifdef CONFIG_SMP
+# define NXMSGQ_INITIALIZER(buffer, msg_size, max_msgs) \
+{ \
+ CIRCBUF_INITIALIZER(buffer, msg_size * max_msgs), \
+ msg_size, \
+ NXSEM_INITIALIZER(0, 0), \
+ NXSEM_INITIALIZER(0, 0), \
+ false, \
+ SP_UNLOCKED, \
+}
+#else
+# define NXMSGQ_INITIALIZER(buffer, msg_size, max_msgs) \
+{ \
+ CIRCBUF_INITIALIZER(buffer, msg_size * max_msgs), \
+ msg_size, \
+ NXSEM_INITIALIZER(0, 0), \
+ NXSEM_INITIALIZER(0, 0), \
+ false, \
+}
+#endif
+
+/
+ * Public Type Declarations
+ /
+
+/* This structure defines a kernel message queue */
+
+typedef struct nxmsgq
+{
+ struct circbuf_s cbuf;
Review Comment:
Yes, `list` is a good suggestion. I will improve it ff we want to implement
a priority queue in the future, However in this stage, if we only compare the
API with freertos/zephyr, the current data type already meets the requirements.
`max_msgs` is the behavior expected by users. Most kernel developers expect
that the queue is within a controllable range, which will help them find
problems in advance.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] nuttx/msgq: add kernel message queue support [nuttx]
pussuw commented on code in PR #16226:
URL: https://github.com/apache/nuttx/pull/16226#discussion_r2048647549
##
include/nuttx/msgq.h:
##
@@ -0,0 +1,315 @@
+/
+ * include/nuttx/msgq.h
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ /
+
+#ifndef ___INCLUDE_NUTTX_MSGQ_H
+#define ___INCLUDE_NUTTX_MSGQ_H
+
+/
+ * Included Files
+ /
+
+#include
+#include
+#include
+#include
+
+/
+ * Pre-processor Definitions
+ /
+
+/
+ * Name: NXMSGQ_INITIALIZER
+ *
+ * Description:
+ * Statically define and initialize a message queue.
+ *
+ * The message queue's ring buffer contains space for max_msgs messages,
+ * each of which is msg_size bytes long.
+ *
+ * Input Parameters:
+ * buffer - Ring buffer of Message Queue
+ * msg_size - Message size (in bytes).
+ * max_msgs - Maximum number of messages that can be queued.
+ *
+ /
+
+#ifdef CONFIG_SMP
+# define NXMSGQ_INITIALIZER(buffer, msg_size, max_msgs) \
+{ \
+ CIRCBUF_INITIALIZER(buffer, msg_size * max_msgs), \
+ msg_size, \
+ NXSEM_INITIALIZER(0, 0), \
+ NXSEM_INITIALIZER(0, 0), \
+ false, \
+ SP_UNLOCKED, \
+}
+#else
+# define NXMSGQ_INITIALIZER(buffer, msg_size, max_msgs) \
+{ \
+ CIRCBUF_INITIALIZER(buffer, msg_size * max_msgs), \
+ msg_size, \
+ NXSEM_INITIALIZER(0, 0), \
+ NXSEM_INITIALIZER(0, 0), \
+ false, \
+}
+#endif
+
+/
+ * Public Type Declarations
+ /
+
+/* This structure defines a kernel message queue */
+
+typedef struct nxmsgq
+{
+ struct circbuf_s cbuf;
+ int msg_size;
+ sem_ttxsem;
+ sem_trxsem;
+ bool alloc;
+#ifdef CONFIG_SMP
+ spinlock_t lock;
+#endif
+} nxmsgq_t;
+
+/
+ * Public Function Prototypes
+ /
+
+#ifdef __cplusplus
+#define EXTERN extern "C"
+extern "C"
+{
+#else
+#define EXTERN extern
+#endif
+
+/
+ * Name: nxmsgq_init
+ *
+ * Description:
+ * Initialize a message queue with static ring buffer
+ *
+ * This routine initializes a message queue object, prior to its first use.
+ *
+ * The message queue's ring buffer must contain space for max_msgs
+ * messages, each of which is msg_size bytes long. Alignment of the
+ * message queue's ring buffer is not necessary.
+ *
+ * Input Parameters:
+ * msgq - Address of the message queue.
+ * buffer - Pointer to ring buffer that holds queued messages.
+ * msg_size - Message size (in bytes).
+ * max_msgs - Maximum number of messages that can be queued.
+ *
+ * Returned Value:
+ * None.
+ *
+ /
+
+void nxmsgq_init(FAR nxmsgq_t *msgq, FAR void *buffer,
+ size_t msg_size, uint32_t max_msgs);
+
+/
+ * Name: nxmsgq_create
+ *
+ * Description:
+ * Create a message queue.
+ *
+ * This routine initializes a message queue object, prior to its first use,
+ * allocating its internal ring buffer from the calling thread's resource
+ * pool.
+ *
+ * Memory allocated for the ring buffer can be released by calling
+ * nxmsgq_destroy()
+ *
+ * Input Parameters:
+ * msg_size - Message
Re: [PR] nuttx/msgq: add kernel message queue support [nuttx]
pussuw commented on PR #16226: URL: https://github.com/apache/nuttx/pull/16226#issuecomment-2812403874 I like the idea a lot, I was always wondering how NuttX did not already have this kernel internal message system (which does not depend on a file system). Like stated in the summary field, many RTOSes provide this and I think it is an extremely useful and powerful synchronization mechanism. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] nuttx/msgq: add kernel message queue support [nuttx]
pussuw commented on code in PR #16226:
URL: https://github.com/apache/nuttx/pull/16226#discussion_r2048639403
##
include/nuttx/msgq.h:
##
@@ -0,0 +1,315 @@
+/
+ * include/nuttx/msgq.h
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ /
+
+#ifndef ___INCLUDE_NUTTX_MSGQ_H
+#define ___INCLUDE_NUTTX_MSGQ_H
+
+/
+ * Included Files
+ /
+
+#include
+#include
+#include
+#include
+
+/
+ * Pre-processor Definitions
+ /
+
+/
+ * Name: NXMSGQ_INITIALIZER
+ *
+ * Description:
+ * Statically define and initialize a message queue.
+ *
+ * The message queue's ring buffer contains space for max_msgs messages,
+ * each of which is msg_size bytes long.
+ *
+ * Input Parameters:
+ * buffer - Ring buffer of Message Queue
+ * msg_size - Message size (in bytes).
+ * max_msgs - Maximum number of messages that can be queued.
+ *
+ /
+
+#ifdef CONFIG_SMP
+# define NXMSGQ_INITIALIZER(buffer, msg_size, max_msgs) \
+{ \
+ CIRCBUF_INITIALIZER(buffer, msg_size * max_msgs), \
+ msg_size, \
+ NXSEM_INITIALIZER(0, 0), \
+ NXSEM_INITIALIZER(0, 0), \
+ false, \
+ SP_UNLOCKED, \
+}
+#else
+# define NXMSGQ_INITIALIZER(buffer, msg_size, max_msgs) \
+{ \
+ CIRCBUF_INITIALIZER(buffer, msg_size * max_msgs), \
+ msg_size, \
+ NXSEM_INITIALIZER(0, 0), \
+ NXSEM_INITIALIZER(0, 0), \
+ false, \
+}
+#endif
+
+/
+ * Public Type Declarations
+ /
+
+/* This structure defines a kernel message queue */
+
+typedef struct nxmsgq
+{
+ struct circbuf_s cbuf;
+ int msg_size;
+ sem_ttxsem;
+ sem_trxsem;
+ bool alloc;
+#ifdef CONFIG_SMP
+ spinlock_t lock;
+#endif
+} nxmsgq_t;
+
+/
+ * Public Function Prototypes
+ /
+
+#ifdef __cplusplus
+#define EXTERN extern "C"
+extern "C"
+{
+#else
+#define EXTERN extern
+#endif
+
+/
+ * Name: nxmsgq_init
+ *
+ * Description:
+ * Initialize a message queue with static ring buffer
+ *
+ * This routine initializes a message queue object, prior to its first use.
+ *
+ * The message queue's ring buffer must contain space for max_msgs
+ * messages, each of which is msg_size bytes long. Alignment of the
+ * message queue's ring buffer is not necessary.
+ *
+ * Input Parameters:
+ * msgq - Address of the message queue.
+ * buffer - Pointer to ring buffer that holds queued messages.
+ * msg_size - Message size (in bytes).
+ * max_msgs - Maximum number of messages that can be queued.
+ *
+ * Returned Value:
+ * None.
+ *
+ /
+
+void nxmsgq_init(FAR nxmsgq_t *msgq, FAR void *buffer,
+ size_t msg_size, uint32_t max_msgs);
+
+/
+ * Name: nxmsgq_create
+ *
+ * Description:
+ * Create a message queue.
+ *
+ * This routine initializes a message queue object, prior to its first use,
+ * allocating its internal ring buffer from the calling thread's resource
+ * pool.
+ *
+ * Memory allocated for the ring buffer can be released by calling
+ * nxmsgq_destroy()
+ *
+ * Input Parameters:
+ * msg_size - Message
Re: [PR] nuttx/msgq: add kernel message queue support [nuttx]
pussuw commented on code in PR #16226:
URL: https://github.com/apache/nuttx/pull/16226#discussion_r2048637427
##
include/nuttx/msgq.h:
##
@@ -0,0 +1,315 @@
+/
+ * include/nuttx/msgq.h
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ /
+
+#ifndef ___INCLUDE_NUTTX_MSGQ_H
+#define ___INCLUDE_NUTTX_MSGQ_H
+
+/
+ * Included Files
+ /
+
+#include
+#include
+#include
+#include
+
+/
+ * Pre-processor Definitions
+ /
+
+/
+ * Name: NXMSGQ_INITIALIZER
+ *
+ * Description:
+ * Statically define and initialize a message queue.
+ *
+ * The message queue's ring buffer contains space for max_msgs messages,
+ * each of which is msg_size bytes long.
+ *
+ * Input Parameters:
+ * buffer - Ring buffer of Message Queue
+ * msg_size - Message size (in bytes).
+ * max_msgs - Maximum number of messages that can be queued.
+ *
+ /
+
+#ifdef CONFIG_SMP
+# define NXMSGQ_INITIALIZER(buffer, msg_size, max_msgs) \
+{ \
+ CIRCBUF_INITIALIZER(buffer, msg_size * max_msgs), \
+ msg_size, \
+ NXSEM_INITIALIZER(0, 0), \
+ NXSEM_INITIALIZER(0, 0), \
+ false, \
+ SP_UNLOCKED, \
+}
+#else
+# define NXMSGQ_INITIALIZER(buffer, msg_size, max_msgs) \
+{ \
+ CIRCBUF_INITIALIZER(buffer, msg_size * max_msgs), \
+ msg_size, \
+ NXSEM_INITIALIZER(0, 0), \
+ NXSEM_INITIALIZER(0, 0), \
+ false, \
+}
+#endif
+
+/
+ * Public Type Declarations
+ /
+
+/* This structure defines a kernel message queue */
+
+typedef struct nxmsgq
+{
+ struct circbuf_s cbuf;
Review Comment:
Would using a linked list (sq or dq) be an option here? The messages
themselves would obviously need a bit more space, but it would remove the
"max_msgs" limit from this implementation ?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] nuttx/msgq: add kernel message queue support [nuttx]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
