Message Queue with per priority memory allocation#5205
Conversation
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
…re conflicts Adopt the reviewed CountingSemaphore API from nasa#5204 (now merged into devel) and adapt PriorityMemQueue/AtomicQueue to the single-argument constructor. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
thomas-bc
left a comment
There was a problem hiding this comment.
Also I agree with your note in your PR, we really ought to be testing this in an actual deployment - we should tag up
Coverage report — base
|
| Module | Line | Δ | Function | Δ | Branch | Δ |
|---|---|---|---|---|---|---|
Os |
14.83 | -3.15 | 17.64 | -1.76 | 11.65 | -2.82 |
Modules changed
| Module | Line | Δ | Function | Δ | Branch | Δ |
|---|---|---|---|---|---|---|
Os |
14.83 | -3.15 | 17.64 | -1.76 | 11.65 | -2.82 |
Os/Generic/Types |
92.03 | -0.42 | 92.86 | +1.19 | 73.33 | -8.81 |
Fw/DataStructures |
98.12 | -0.36 | 97.14 | +0.38 | 82.66 | -0.55 |
Os/Generic |
89.29 | +0.19 | 88.16 | -8.14 | 73.11 | +5.93 |
Os/Posix |
62.67 | +0.27 | 84.21 | +0.00 | 44.34 | +0.24 |
Modules without UTs
CFDP/Checksum/GTest, Drv/ByteStreamDriverModel, Drv/Interfaces, Drv/LinuxGpioDriver, Drv/LinuxI2cDriver, Drv/LinuxSpiDriver, Drv/LinuxUartDriver, Drv/Ports, Drv/Ports/DataTypes, FppTestProject/FppTest/interfaces, FppTestProject/FppTest/topology/async, FppTestProject/FppTest/topology/components/Comp, FppTestProject/FppTest/topology/components/Framework, FppTestProject/FppTest/topology/components/Receiver, FppTestProject/FppTest/topology/components/Sender, FppTestProject/FppTest/topology/guarded, FppTestProject/FppTest/topology/ports, FppTestProject/FppTest/topology/sync, FppTestProject/FppTest/topology/top_ports, FppTestProject/FppTest/topology/types, Fw/Cmd, Fw/Com, Fw/Comp, Fw/FilePacket/GTest, Fw/Fpy, Fw/Interfaces, Fw/Obj, Fw/Port, Fw/Ports/CompletionStatus, Fw/Ports/Ready, Fw/Ports/Signal, Fw/Ports/SuccessCondition, Fw/Prm, Fw/SerializableFile/test/TestSerializable, Fw/Sm, Fw/Test, Fw/Types/GTest, Os/Models, Svc/Cycle, Svc/DpPorts, Svc/Fatal, Svc/FatalHandler, Svc/FileDownlinkPorts, Svc/FprimeProtocol, Svc/Interfaces, Svc/PassiveConsoleTextLogger, Svc/Ping, Svc/PolyIf, Svc/Ports/CommsPorts, Svc/Ports/FilePorts, Svc/Ports/OsTimeEpoch, Svc/Ports/TlmPacketizerPorts, Svc/Ports/VersionPorts, Svc/Sched, Svc/Seq, Svc/Subtopologies/CdhCore, Svc/Subtopologies/ComCcsds, Svc/Subtopologies/ComFprime, Svc/Subtopologies/ComLoggerTee, Svc/Subtopologies/DataProducts, Svc/Subtopologies/FileHandling, Svc/Types/TlmPacketizerTypes, Svc/WatchDog, TestDeploymentsProject/Ref/PingReceiver, TestDeploymentsProject/Ref/RecvBuffApp, TestDeploymentsProject/Ref/SendBuffApp, TestDeploymentsProject/Ref/Top, TestDeploymentsProject/Ref/TypeDemo, cmake/test/data/TestDeployment/TestBuildAutocoder, cmake/test/data/TestDeployment/TestChainedAutocoder, cmake/test/data/TestDeployment/TestHeaderAutocoder, cmake/test/data/TestDeployment/TestTargetAutocoder, cmake/test/data/test-fprime-library/TestLibrary/TestComponent, cmake/test/data/test-fprime-library2/TestLibrary2/TestComponent
lestarch-autobot
left a comment
There was a problem hiding this comment.
Automated review summary (run 3)
Per-agent results
| Agent | must fix | suggestion | could fix | future work | outstanding | Verdict |
|---|---|---|---|---|---|---|
| Security Vulnerabilities | 0 | 1 | 0 | 0 | 1 | Go |
| Supply Chain / Runner Safety | 0 | 0 | 1 | 0 | 1 | Go |
| F Prime C/C++ Design | 2 | 1 | 1 | 0 | 4 | No-Go |
| Documentation Currency | 1 | 0 | 1 | 0 | 2 | No-Go |
| Design | 1 | 1 | 1 | 0 | 1 | Go |
| Architecture | 0 | 0 | 0 | 0 | 0 | Go |
| Test Quality | 0 | 1 | 0 | 0 | 1 | Go |
| CI safety | — | — | — | — | — | Go |
| Totals | 4 | 4 | 4 | 0 | 10 | No-Go |
Since last run
| Agent | resolved | still open | newly added | incorrect-fix follow-ups | improperly resolved | disagreements escalated |
|---|---|---|---|---|---|---|
| Security Vulnerabilities | 0 | 1 | 0 | 0 | 0 | 0 |
| Supply Chain / Runner Safety | 0 | 1 | 0 | 0 | 0 | 0 |
| F Prime C/C++ Design | 0 | 4 | 0 | 0 | 0 | 0 |
| Documentation Currency | 0 | 2 | 0 | 0 | 0 | 0 |
| Design | 2 | 1 | 0 | 0 | 0 | 0 |
| Architecture | 0 | 0 | 0 | 0 | 0 | 0 |
| Test Quality | 0 | 0 | 1 | 0 | 0 | 0 |
Supply-chain surfaces
| Surface | Outstanding |
|---|---|
| Dependencies | 1 could-fix — fprime_python_model not in any manifest and not on PyPI |
| Vendored / submodule | clean |
| Build / test infrastructure | clean |
| Workflows / actions / scripts | clean |
| Generator output | clean |
| Prompt-injection | clean |
| Review-system integrity | clean |
Outstanding must-fix items (3)
F Prime C/C++ Design
AtomicQueuedeclares a destructor managing resources but does not delete or define copy/move operations (CPP-17) — linkPriorityMemQueueHandleholds owning pointers but defines neither a destructor nor deleted copy/move operations (CPP-17) — link
Documentation Currency
Os/docs/sdd.md§4 "Generic Services" table must also listPriorityMemQueue— link
Merge readiness
Merge readiness: No-Go — F Prime C/C++ Design has 2 and Documentation Currency has 1 outstanding must-fix item. Design cleared its prior must-fix this run; CI safety remains Go (security and supply-chain both clean).
Two decks still flagged on the board — seal the copy/move hatches and log PriorityMemQueue in the SDD, and this one's cleared for final approach.
Applied suggestion from @lestarch-autobot Co-authored-by: lestarch-autobot <lestarch+autobot@gmail.com>
[Security] suggestion Integer overflow in allocation size computation. [C++ Design] must fix AtomicQueue declares a destructor managing resources (m_slots, m_bufferMemory, m_notFullSem) but does not delete or define copy/move operations (CPP-17). [C++ Design] must fix PriorityMemQueueHandle holds owning pointers (m_atomicQueues, m_notEmptySem, m_highWaterMarks) but defines neither a destructor nor deleted copy/move operations (CPP-17). [C++ Design] could fix reinterpret_cast from void* where static_cast is well-defined and preferred (CPP-10). Co-authored-by: lestarch-autobot <lestarch+autobot@gmail.com>
|
Zephyr CI has been acting up in the past few days - failures here can be ignored. |
Change Description
This PR adds:
The code in this PR depends on #5204 and cannot be merged until that PR is complete
Rationale
F' allows a component to mark some ports as drop and some ports as assert on full. This allows "drop" ports to fill up the message queue and cause "assert" ports to fail to send a message leading to an assert. E.g. If the queue is filled up by messages that could be dropped, and then a single message comes in that cannot be dropped, ComQueue triggers an assertion:
Adding a message queue with per priority memory allocation resolves that
Separating messages by priority is also useful for ports with the same drop behavior when the different messages have different priorities (e.g. it would be bad for EventManager to drop a WARNING HI event because DIAGNOSTIC events consumed memory pool).
Testing/Review Recommendations
There's unit tests and PriorityMemQueue has been tested in a private deployment. I'd like to add it to a deployment in fprime-community but I'm not sure which
Future Work
F' components should be updated to use different priorities when sending messages with different urgencies or with different queue full behavior.
AI Usage (see policy)
Claude code - Sonnet 4.5. Used to assist with code generation under human supervision (careful review for the flight code, less careful review for the unit test code) and with documentation