Mailbox requirements #83
Conversation
jkey-eng
force-pushed
the
mailbox-requirements
branch
3 times, most recently
from
73d3fc5 to
78c6661
Compare
tobiaskaestner
left a comment
tobiaskaestner
left a comment
There was a problem hiding this comment.
First of all, many thanks for the PR.
My main concern atm is to not introduce another subject for this set of requirements, ie. the Mailbox module. Where I too notice I have replaced this with the subject Zephyr RTOS as this is what we have been using in the other requirements (eg. semaphore) so far.
The rationale behind this is to avoid irritations as to where these 'modules' need to be defined.
Other than that, the other points relate to avoiding requirements that say what is not provided and how to deal with error cases. I'd like to see thoughts from the others on those two points, too.
docs/system_requirements/index.sdoc
Outdated
| COMPONENT: Mailboxes | ||
| TITLE: Mailboxes | ||
| STATEMENT: >>> | ||
| The Zephyr RTOS shall provide a framework to pass messages between threads. |
There was a problem hiding this comment.
| The Zephyr RTOS shall provide a framework to pass messages between threads. | |
| The Zephyr RTOS shall provide a framework to pass messages of arbitrary size between threads. |
There was a problem hiding this comment.
Trying to capture the difference to a normal message queue here (which is fixed size messages)
| STATUS: Draft | ||
| TYPE: Functional | ||
| COMPONENT: Mailboxes | ||
| TITLE: Mailbox Initialization - Run Time |
There was a problem hiding this comment.
| TITLE: Mailbox Initialization - Run Time | |
| TITLE: Mailbox Initialization At Run Time |
| STATUS: Draft | ||
| TYPE: Functional | ||
| COMPONENT: Mailboxes | ||
| TITLE: Mailbox Initialization - Compile Time |
There was a problem hiding this comment.
| TITLE: Mailbox Initialization - Compile Time | |
| TITLE: Mailbox Initialization At Compile Time |
| COMPONENT: Mailboxes | ||
| TITLE: Mailbox Initialization - Compile Time | ||
| STATEMENT: >>> | ||
| The Mailbox module shall provide a mechanism to statically define and initialize a mailbox at compile time. |
There was a problem hiding this comment.
| The Mailbox module shall provide a mechanism to statically define and initialize a mailbox at compile time. | |
| The Zephyr RTOS shall provide a mechanism to statically define and initialize a mailbox object at compile time. |
| COMPONENT: Mailboxes | ||
| TITLE: Queue Initialization | ||
| STATEMENT: >>> | ||
| When a mailbox is initialized, the Mailbox module shall set both the send queue and receive queue of that mailbox to empty. |
There was a problem hiding this comment.
| When a mailbox is initialized, the Mailbox module shall set both the send queue and receive queue of that mailbox to empty. | |
| When a mailbox is initialized, the Zephyr RTOS shall set both the send queue and receive queue of that mailbox to empty. |
| COMPONENT: Mailboxes | ||
| TITLE: FIFO Message Delivery | ||
| STATEMENT: >>> | ||
| When multiple messages are available in the send queue, the Mailbox module shall deliver them in FIFO (First In, First Out) order to waiting threads. |
There was a problem hiding this comment.
Again, this is only true IMHO for message from the same sender, ie. only messages from matching sender to matching receivers are delivered in order. As in the above case usage of K_ANY for rx/tx thread may complicate things further.
| COMPONENT: Mailboxes | ||
| TITLE: Mailbox Quantity Support | ||
| STATEMENT: >>> | ||
| The Mailbox module shall support an arbitrary number of mailboxes, limited only by available RAM in the system. |
There was a problem hiding this comment.
| The Mailbox module shall support an arbitrary number of mailboxes, limited only by available RAM in the system. | |
| The Zephyr RTOS shall support an arbitrary number of mailbox objects, limited only by available RAM in the system. |
| COMPONENT: Mailboxes | ||
| TITLE: Resource Cleanup | ||
| STATEMENT: >>> | ||
| The Mailbox module shall ensure proper cleanup of message resources when operations are canceled or time out to prevent resource leaks. |
There was a problem hiding this comment.
| The Mailbox module shall ensure proper cleanup of message resources when operations are canceled or time out to prevent resource leaks. | |
| The Zephyr RTOS shall ensure proper cleanup of mailbox object resources when operations are canceled or time out to prevent resource leaks. |
There was a problem hiding this comment.
IMHO, the problem with this kind of requirement is in the adjective. What do we mean with 'proper'? I know that above I made the reverse point for 'synchronous' and 'asynchronous' to imply blocking and non-blocking respectively. Still, in the context of computer science they seem to be somewhat better defined than 'proper' or 'correct'
There was a problem hiding this comment.
This looks like another generated requirement that I should have removed.
I don't see any functions in the API that would cancel or drive resource leaks if they were to time out.
| COMPONENT: Mailboxes | ||
| TITLE: Invalid Parameter Handling | ||
| STATEMENT: >>> | ||
| The Mailbox module shall handle invalid parameters by returning appropriate error codes rather than causing system failures. |
There was a problem hiding this comment.
Okay, that's the third option besides timeouts and out-of-memory ;-)
jkey-eng
left a comment
jkey-eng
left a comment
There was a problem hiding this comment.
@tobiaskaestner Thanks for the review and feedback!
I've made updates accordingly.
It doesn't look like the feedback stayed in-line, not sure if that's due to something I did on my end.
| COMPONENT: Mailboxes | ||
| TITLE: Variable Message Size | ||
| STATEMENT: >>> | ||
| The Mailbox module shall support messages of any size that the application requires. |
| COMPONENT: Mailboxes | ||
| TITLE: Message Descriptor Updates | ||
| STATEMENT: >>> | ||
| The Mailbox module shall update message descriptor fields during the exchange to allow both threads to know what has occurred during the message exchange process. |
There was a problem hiding this comment.
Removed.
It seems redundant to the others. Not sure what behavior this was looking to capture. I should have deleted it when I reviewed the generated requirements.
| COMPONENT: Mailboxes | ||
| TITLE: Buffer Data Transfer | ||
| STATEMENT: >>> | ||
| Where message data is in a buffer, the Mailbox module shall correctly handle the data transfer between the sending and receiving threads. |
There was a problem hiding this comment.
Candidate for removal.
I've updated this one to cover the basic behavior of passing the data between mailbox objects. I see justification to delete it if that's assumed behavior as part of the other functions.
| COMPONENT: Mailboxes | ||
| TITLE: Synchronous Sending Behavior | ||
| STATEMENT: >>> | ||
| When k_mbox_put is called, the Mailbox module shall block the sending thread until a receiver processes the message or the timeout expires. |
There was a problem hiding this comment.
I'd missed the function name when reviewing the generated requirements. It would definitely be too messy to include function names in the requirements.
I included this requirement to explicitly define what was meant by synchronous sending and with the expectation it would drive a specific test to verify that behavior. If the behavior can be assumed from the previous requirement I'd agree it makes sense to remove this requirement.
| COMPONENT: Mailboxes | ||
| TITLE: Asynchronous Sending Behavior | ||
| STATEMENT: >>> | ||
| When an asynchronous message is sent, the Mailbox module shall not wait for a receiver to process the message before returning to the caller. |
There was a problem hiding this comment.
Same counter-point as above. I agree it makes sense to remove it if we're expecting the behavior can be assumed.
| COMPONENT: Mailboxes | ||
| TITLE: Thread-Only Message Exchange | ||
| STATEMENT: >>> | ||
| The Mailbox module shall allow only threads, not ISRs, to exchange messages using the mailbox functions. |
There was a problem hiding this comment.
Removed.
When I first read the requirement, I was thinking it was an architectural decision that would need testing to ensure separation. I agree that it makes sense to remove it after rereading the documentation.
| COMPONENT: Mailboxes | ||
| TITLE: Single-Receiver Message Delivery | ||
| STATEMENT: >>> | ||
| The Mailbox module shall ensure that each message may be received by only one thread (i.e., point-to-multipoint and broadcast messaging is not supported). |
There was a problem hiding this comment.
Removed.
Same rationale as above.
| COMPONENT: Mailboxes | ||
| TITLE: Priority-Based Message Delivery | ||
| STATEMENT: >>> | ||
| When multiple threads are waiting on an empty mailbox, the Mailbox module shall deliver the next message to the highest priority thread that has waited longest. |
There was a problem hiding this comment.
Split this into two requirements. One focused on the prioritization, another to handle the edge case where two threads have equal priority.
| COMPONENT: Mailboxes | ||
| TITLE: Resource Cleanup | ||
| STATEMENT: >>> | ||
| The Mailbox module shall ensure proper cleanup of message resources when operations are canceled or time out to prevent resource leaks. |
There was a problem hiding this comment.
This looks like another generated requirement that I should have removed.
I don't see any functions in the API that would cancel or drive resource leaks if they were to time out.
|
@tobiaskaestner Have you had a chance to review the requirement updates I've made in response to your feedback? |
| STATEMENT: >>> | ||
| When a mailbox is initialized, the Zephyr RTOS shall set both the send queue and receive queue of that mailbox to empty. | ||
| <<< |
There was a problem hiding this comment.
Suggestion to relocate that requirement to component/unit design requirements
Reason: This requirement contains implementation details (receive/send queue in a mailbox) that can only be known at after Architecture design or at Component/Unit design stage.
There was a problem hiding this comment.
Removed. What is the plan to capture design requirements?
| STATEMENT: >>> | ||
| Where message data is non-existent (i.e., an empty message), the Zephyr RTOS shall correctly handle the message exchange without requiring buffer allocation. | ||
| <<< |
There was a problem hiding this comment.
Suggestion to relocate that requirement to component/unit design requirements
Reason: This requirement contains implementation details (underlying buffer to mailbox) that can only be known at after Architecture design or at Component/Unit design stage.
| Where message data is non-existent (i.e., an empty message), the Zephyr RTOS shall correctly handle the message exchange without requiring buffer allocation. | ||
| <<< | ||
| USER_STORY: >>> | ||
| As a Zephyr RTOS developer, I want to be able to send signals or notifications that don't carry data so that I can trigger actions in other threads without the overhead of data transfer. |
There was a problem hiding this comment.
Suggestion to make this the requirement, The Zephyr RTOS shall support sending empty messages through mailbox
There was a problem hiding this comment.
Note, this is already done by UID: ZEP-SRS-22-4 "".
So consider removing altogether as suggested in STATEMENT comment
There was a problem hiding this comment.
Removed ZEP-SRS-22-7 if that's what this comment is referring to. The comment traces are a little tricky to follow across file versions.
tobiaskaestner
left a comment
tobiaskaestner
left a comment
There was a problem hiding this comment.
@parphane and I did the review together and found a couple of RQTs that we can probably remove/merge with others and a few wording suggestions.
| TITLE: Queue Initialization | ||
| STATEMENT: >>> | ||
| When a mailbox is initialized, the Mailbox module shall set both the send queue and receive queue of that mailbox to empty. | ||
| When a mailbox is initialized, the Zephyr RTOS shall set both the send queue and receive queue of that mailbox to empty. |
There was a problem hiding this comment.
Consider removing this at the SRS level, we can re-introduce this at the design level since this contains implementation details.
| TITLE: Message Descriptor Usage | ||
| STATEMENT: >>> | ||
| The Mailbox module shall use message descriptors to specify where a message's data is located and how the message is to be handled by the mailbox. | ||
| The Zephyr RTOS shall use message descriptors to specify where a message's data is located and how the message is to be handled by the mailbox. |
There was a problem hiding this comment.
Seeing this again, IMHO consider removing this at the SRS level, we can re-introduce this at the design level since this contains implementation details.
| TITLE: Empty Message Support | ||
| STATEMENT: >>> | ||
| Where message data is non-existent (i.e., an empty message), the Mailbox module shall correctly handle the message exchange without requiring buffer allocation. | ||
| Where message data is non-existent (i.e., an empty message), the Zephyr RTOS shall correctly handle the message exchange without requiring buffer allocation. |
There was a problem hiding this comment.
| Where message data is non-existent (i.e., an empty message), the Zephyr RTOS shall correctly handle the message exchange without requiring buffer allocation. | |
| The Zephyr RTOS shall allow for empty messages to be sent. |
There was a problem hiding this comment.
avoiding buffer allocation is an implementation specific detail again which we can omit at the SRS level I think
There was a problem hiding this comment.
Removed all-together since it's effectively redundant to ZEP-SRS-22-4
| TITLE: Synchronous Sending Behavior | ||
| STATEMENT: >>> | ||
| When k_mbox_put is called, the Mailbox module shall block the sending thread until a receiver processes the message or the timeout expires. | ||
| When a synchronous message is sent, the Zephyr RTOS shall block the sending thread until a receiver processes the message or the timeout expires. |
There was a problem hiding this comment.
I think this can be merged into 22-8 as this is the definition of what synchronous means in 22-8
There was a problem hiding this comment.
I rephrased ZEP-SRS-22-8 as follows to clarify the meaning of synchronous
The Zephyr RTOS shall provide a mechanism for a thread to send a message through a mailbox and block until the message is processed or a timeout occurs.
| TITLE: Synchronous Send Timeout Handling | ||
| STATEMENT: >>> | ||
| If the synchronous message send operation times out before a receiver processes the message, the Mailbox module shall return an appropriate timeout error code to the sending thread. | ||
| If the synchronous message send operation times out before a receiver processes the message, the Zephyr RTOS shall return an appropriate timeout error code to the sending thread. |
There was a problem hiding this comment.
the word 'appropriate' is ambiguous here, I'd just remove it.
| TITLE: Asynchronous Completion Signaling | ||
| STATEMENT: >>> | ||
| When an asynchronous send command receives a semaphore, the Mailbox module shall give the semaphore when the message has been both received and completely processed by the receiver. | ||
| When a sending thread provides a semaphore during an asynchronous send command using a mailbox object, the Zephyr RTOS shall give the semaphore when the message has been both received and completely processed by the receiver. |
There was a problem hiding this comment.
| When a sending thread provides a semaphore during an asynchronous send command using a mailbox object, the Zephyr RTOS shall give the semaphore when the message has been both received and completely processed by the receiver. | |
| When a sending thread asynchronously sends a message to a mailbox object, the Zephyr RTOS shall provide a mechanism to signal to the sending thread that the message has been both received and completely processed by the receiver. |
There was a problem hiding this comment.
that the current implementation uses a semaphore to achieve this is an implementation detail I'd prefer to hide
There was a problem hiding this comment.
Updated with your phrasing to remove the implementation details
| TITLE: Message Data Retrieval | ||
| STATEMENT: >>> | ||
| The Mailbox module shall provide a mechanism for a thread to retrieve mailbox message data into a buffer and complete the message processing. | ||
| The Zephyr RTOS shall provide a mechanism for a thread to retrieve message data via a mailbox object into a buffer and complete the message processing. |
There was a problem hiding this comment.
| The Zephyr RTOS shall provide a mechanism for a thread to retrieve message data via a mailbox object into a buffer and complete the message processing. | |
| The Zephyr RTOS shall provide a mechanism for a thread to retrieve message data via a mailbox object. |
There was a problem hiding this comment.
Updated with new phrasing.
| TITLE: Non-Anonymous Messaging | ||
| STATEMENT: >>> | ||
| The Mailbox module shall handle messages non-anonymously, allowing both the sending and receiving threads to know the identity of the other thread. | ||
| The Zephyr RTOS shall handle message exchange via a mailbox object non-anonymously, allowing both the sending and receiving threads to know the identity of the other thread. |
There was a problem hiding this comment.
the receiving thread will always know the sending thread, but with K_ANY the sending thread cannot tell which thread received the message via a mailbox
There was a problem hiding this comment.
I'm struggling to understand what update (if any) to make based on this feedback.
From the docs:
Messages exchanged using a mailbox are handled non-anonymously, allowing both threads participating in an exchange to know (and even specify) the identity of the other thread.
Are you suggesting the current behavior is out of spec, or maybe that this requirement would be a should statement where it would be a wanted feature but not mandatory?
| TITLE: Receiver Thread Queuing | ||
| STATEMENT: >>> | ||
| While a thread is waiting to receive a message, the Mailbox module shall keep the thread in the receive queue until a matching message arrives or the waiting period expires. | ||
| While a thread is waiting to receive a message via a mailbox object, the Zephyr RTOS shall keep the thread in the receive queue until a matching message arrives or the waiting period expires. |
There was a problem hiding this comment.
Consider removing this at the SRS level, we can re-introduce this at the design level since this contains implementation details.
| TITLE: Invalid Parameter Handling | ||
| STATEMENT: >>> | ||
| The Mailbox module shall handle invalid parameters by returning appropriate error codes rather than causing system failures. | ||
| The Zephyr RTOS shall handle invalid parameters by returning appropriate error codes rather than causing system failures. |
There was a problem hiding this comment.
please remove 'appropriate'
Hi @jkey-eng , just finished another round of review ;-) |
jkey-eng
left a comment
jkey-eng
left a comment
There was a problem hiding this comment.
@tobiaskaestner @parphane I've pushed an updated commit based on your feedback and addressed the review comments. Hopefully I caught everything, it's a little tricky to understand which review comments are stale.
As a general practice I leave resolving Github feedback to the poster as a confirmation step that the feedback has been suitable addressed.
| TITLE: Queue Initialization | ||
| STATEMENT: >>> | ||
| When a mailbox is initialized, the Mailbox module shall set both the send queue and receive queue of that mailbox to empty. | ||
| When a mailbox is initialized, the Zephyr RTOS shall set both the send queue and receive queue of that mailbox to empty. |
| STATEMENT: >>> | ||
| When a mailbox is initialized, the Zephyr RTOS shall set both the send queue and receive queue of that mailbox to empty. | ||
| <<< |
There was a problem hiding this comment.
Removed. What is the plan to capture design requirements?
| TITLE: Message Descriptor Usage | ||
| STATEMENT: >>> | ||
| The Mailbox module shall use message descriptors to specify where a message's data is located and how the message is to be handled by the mailbox. | ||
| The Zephyr RTOS shall use message descriptors to specify where a message's data is located and how the message is to be handled by the mailbox. |
| TITLE: Empty Message Support | ||
| STATEMENT: >>> | ||
| Where message data is non-existent (i.e., an empty message), the Mailbox module shall correctly handle the message exchange without requiring buffer allocation. | ||
| Where message data is non-existent (i.e., an empty message), the Zephyr RTOS shall correctly handle the message exchange without requiring buffer allocation. |
There was a problem hiding this comment.
Removed all-together since it's effectively redundant to ZEP-SRS-22-4
| STATEMENT: >>> | ||
| Where message data is non-existent (i.e., an empty message), the Zephyr RTOS shall correctly handle the message exchange without requiring buffer allocation. | ||
| <<< |
| TITLE: Asynchronous Completion Signaling | ||
| STATEMENT: >>> | ||
| When an asynchronous send command receives a semaphore, the Mailbox module shall give the semaphore when the message has been both received and completely processed by the receiver. | ||
| When a sending thread provides a semaphore during an asynchronous send command using a mailbox object, the Zephyr RTOS shall give the semaphore when the message has been both received and completely processed by the receiver. |
There was a problem hiding this comment.
Updated with your phrasing to remove the implementation details
| TITLE: Message Data Retrieval | ||
| STATEMENT: >>> | ||
| The Mailbox module shall provide a mechanism for a thread to retrieve mailbox message data into a buffer and complete the message processing. | ||
| The Zephyr RTOS shall provide a mechanism for a thread to retrieve message data via a mailbox object into a buffer and complete the message processing. |
There was a problem hiding this comment.
Updated with new phrasing.
| TITLE: Receiver Thread Queuing | ||
| STATEMENT: >>> | ||
| While a thread is waiting to receive a message, the Mailbox module shall keep the thread in the receive queue until a matching message arrives or the waiting period expires. | ||
| While a thread is waiting to receive a message via a mailbox object, the Zephyr RTOS shall keep the thread in the receive queue until a matching message arrives or the waiting period expires. |
| TITLE: Invalid Parameter Handling | ||
| STATEMENT: >>> | ||
| The Mailbox module shall handle invalid parameters by returning appropriate error codes rather than causing system failures. | ||
| The Zephyr RTOS shall handle invalid parameters by returning appropriate error codes rather than causing system failures. |
| TITLE: Non-Anonymous Messaging | ||
| STATEMENT: >>> | ||
| The Mailbox module shall handle messages non-anonymously, allowing both the sending and receiving threads to know the identity of the other thread. | ||
| The Zephyr RTOS shall handle message exchange via a mailbox object non-anonymously, allowing both the sending and receiving threads to know the identity of the other thread. |
There was a problem hiding this comment.
I'm struggling to understand what update (if any) to make based on this feedback.
From the docs:
Messages exchanged using a mailbox are handled non-anonymously, allowing both threads participating in an exchange to know (and even specify) the identity of the other thread.
Are you suggesting the current behavior is out of spec, or maybe that this requirement would be a should statement where it would be a wanted feature but not mandatory?
| <<< | ||
| USER_STORY: >>> | ||
| As a Zephyr RTOS developer, I want the system to automatically clean up resources when operations fail or time out so that my application doesn't experience memory leaks. | ||
| When multiple messages are available in the send queue from the same sender, the Zephyr RTOS shall deliver them in FIFO (First In, First Out) order to waiting threads. |
There was a problem hiding this comment.
The linked documentation (link) mentioned the following:
Receiving threads do not always receive messages in a first in, first out (FIFO) order
So I was surprised to see this requirement.
There was a problem hiding this comment.
I see your point here. Deleted as well to keep it simpler for now.
I was thinking it might be valuable to suggest that the requirement was FIFO would be followed in the specific instances where it'd be true, but don't see that as a valuable addition.
woodmatthews
left a comment
woodmatthews
left a comment
There was a problem hiding this comment.
I flagged one comment on FIFO, but the requirements look good otherwise.
Used Perplexity to automatically create requirements for the mailbox module and added them to a new sdoc signed-off-by: Joel Key <[email protected]>
Performed a manual review and cleanup of the generated requirements. Compared the requirements to the Mailboxes documentation page for functionality. Generalized the requirements to remove function names. signed-off-by: Joel Key <[email protected]>
Added a basic System Requirement for the Mailboxes functionality. signed-off-by: Joel Key <[email protected]>
Linked the newly created Mailboxes Software Requirements to the Generic System Requirement. signed-off-by: Joel Key <[email protected]>
Updated requirements based on the feedback after the initial requirement review. signed-off-by: Joel Key <[email protected]>
Deleted / simplified requirements with implementation details Updated phrasing of synchronous and asynchronous mailbox sends signed-off-by: Joel Key <[email protected]>
jkey-eng
force-pushed
the
mailbox-requirements
branch
from
87c7e24 to
e97ce7d
Compare
Updated to incorporate fix for StrictDoc migration changes (changing [SECTION] to [[SECTION]]) Cleaned up requirement numbering based on external changes. signed-off-by: Joel Key <[email protected]>
jkey-eng
force-pushed
the
mailbox-requirements
branch
from
e97ce7d to
f80d1a2
Compare
Removed the proposed requirement for FIFO message transfer based on review feedback. signed-off-by: Joel Key <[email protected]>
|
Rebased the PR based on the recent changes & StrictDoc compatibility. @tobiaskaestner Will you be reviewing and closing out requested changes? |
5 participants
I've created requirements for the mailbox module in support of #63.
I used Perplexity to generate an initial list of requirements, reviewed them with the Mailboxes documentation and did some simple verbiage updates to be more consistent with some other requirements I've reviewed.
Looking to get some early feedback on whether this is what the team is looking for before I spend too much more time reviewing. It didn't take too long to do and seems like a good way to get the ball rolling forward.
Remaining Work: