Remove unnecessary buffer allocation/copy in FprimeRouter for file and unknown packets#30
Open
devin-ai-integration[bot] wants to merge 7 commits intodevelfrom
Open
Conversation
…d unknown packets Instead of allocating a new buffer and copying data for file and unknown packet types, pass the original buffer directly to the receiver. The buffer is not returned to the deframer via dataReturnOut until it comes back on fileBufferReturnIn, at which point an empty context is constructed for the return. This eliminates the need for bufferAllocate and bufferDeallocate ports, as well as the AllocationReason enum and AllocationError event. Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>
Author
Original prompt from michael.d.starch
|
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
…pologies Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>
Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>
Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…urn contract, document context discard Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>
Co-Authored-By: michael.d.starch <michael.d.starch@jpl.nasa.gov>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change Description
Svc::FprimeRouterpreviously allocated a new buffer and copied incoming data for both file and unknown packet types before forwarding. This was done so the original buffer could be immediately returned to the deframer viadataReturnOut.This PR removes that allocation/copy. Instead, the original buffer is passed directly to the receiver on
fileOut/unknownDataOut. The buffer is not returned to the deframer until it comes back onfileBufferReturnIn, at which pointdataReturnOutis invoked with an emptyComCfg::FrameContext.Specifically:
dataIn_handler: Each switch case now explicitly handles its own buffer return. The command case returns the buffer immediately after dispatching. The file and unknown cases pass the buffer through to the receiver when the port is connected (deferring return tofileBufferReturnIn), or return it immediately when the port is unconnected.fileBufferReturnIn_handler: Now callsdataReturnOut_outwith an empty context instead ofbufferDeallocate_out.bufferAllocate,bufferDeallocateoutput ports,AllocationReasonenum, andAllocationErrorevent.fprimeRouter.bufferAllocateandfprimeRouter.bufferDeallocateconnections from bothComFprime.fppandComCcsds.fpp.testAllocationFailureFileandtestAllocationFailureUnknown. Updated remaining tests to reflect thatdataReturnOutis no longer invoked during file/unknown routing, and thattestBufferReturnnow assertsdataReturnOutinstead ofbufferDeallocate.Rationale
The buffer copy was unnecessary — the router can simply pass the buffer through and defer returning it to the deframer. This simplifies the component, removes the dependency on a buffer allocator, and eliminates a potential allocation failure path.
Testing/Review Recommendations
Key areas for review:
fileOutorunknownDataOutmust return them viafileBufferReturnInor the buffers will never be returned to the deframer. Verify thatFileUplinkand any custom routers already follow this contract.fileBufferReturnIn_handlerconstructs a defaultComCfg::FrameContext— the original context fromdataInis discarded. Confirm no downstream consumer ofdataReturnOutrelies on context data.bufferAllocateorbufferDeallocateon this component, or referencingAllocationError, will need updating. The two in-tree subtopologies (ComFprime,ComCcsds) have been updated in this PR — check for any out-of-tree references.Unit tests were updated and pass locally (6/6).
Human Review Checklist
FileUplinkreturns buffers viafileBufferReturnIn(not some other path)bufferAllocate/bufferDeallocateportsdataReturnOutdepends on a populatedFrameContextFuture Work
fileBufferReturnInto something more generic (e.g.bufferReturnIn) since it now serves as the return path for both file and unknown buffers.AI Usage (see policy)
Generative AI (Devin / Claude) was used for code generation, test updates, and documentation updates under human direction.
Link to Devin session: https://nasa-jpl-demo.devinenterprise.com/sessions/bfc230d4f2ed448f8b88cbb457ca1456