Skip to content

Refactor SerializeBufferBase to LinearBufferBase/SerialBufferBase (Phases 1-2)#29

Open
devin-ai-integration[bot] wants to merge 5 commits intodevelfrom
devin/1775768714-serial-buffer-base-refactor
Open

Refactor SerializeBufferBase to LinearBufferBase/SerialBufferBase (Phases 1-2)#29
devin-ai-integration[bot] wants to merge 5 commits intodevelfrom
devin/1775768714-serial-buffer-base-refactor

Conversation

@devin-ai-integration
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration bot commented Apr 9, 2026

Related Issue(s) SerialBufferBase abstraction refactoring (Phases 1–2)
Has Unit Tests (y/n) n (existing tests cover these utilities; no behavior change)
Documentation Included (y/n) y (fpp-codegen-changes-needed.md added)
Generative AI was used in this contribution (y/n) y

Change Description

Replace all uses of the Fw::SerializeBufferBase alias with the appropriate explicit type based on what the function body actually requires. SerializeBufferBase is a type alias for LinearBufferBase. Each usage is disambiguated into:

  • Fw::SerialBufferBase& — for parameters/variables where the function only calls serialize/deserialize API methods (serializeFrom(), deserializeTo(), resetSer(), getSize(), etc.). This is the abstract buffer interface with no pointer-access methods.
  • Fw::LinearBufferBase& — for parameters/variables where the function (or a callee) calls getBuffAddr() or other pointer-returning methods.

Phase 1: Svc/CmdSequencer/test/ut/SequenceFiles/ (50 files)

Changed to SerialBufferBase& (4 function groups):

  • AMPCS::Headers::serialize() (both overloads) — only calls serializeFrom()
  • AMPCS::Records::serialize()dest parameter in all 3 overloads — only calls serializeFrom()
  • FPrime::Headers::serialize() — only calls serializeFrom()
  • FPrime::Records::serialize()destBuffer in all 3 overloads — only calls serializeFrom()

Kept as LinearBufferBase& (everything else):

  • AMPCS::CRCs::computeCRC()/createFile() — calls buffer.getBuffAddr()
  • AMPCS::Records::serialize() cmdField param — calls cmdField.getBuffAddr()
  • FPrime::CRCs::serialize() — calls destBuffer.getBuffAddr()
  • Buffers::write() — calls buffer.getBuffAddr()
  • FileBuffer class — concrete buffer, inheritance changed from alias to canonical LinearBufferBase
  • File base class virtual methods — must match derived signatures
  • All 17 derived file classes (BadCRCFile, ImmediateFile, etc.) — override virtual methods from File

Phase 2: Framework, Drv, and test utility files (13 files changed)

Changed to SerialBufferBase&:

  • TmFramer::fill_with_idle_packet() — only calls getSize() and serializeFrom()
  • AosFramer::serialize_idle_spp_packet() — only calls serializeFrom()
  • GenericHubTester::random_fill() — only calls resetSer() and serializeFrom()
  • AsyncByteStreamBufferAdapterTester::random_fill() — only calls serializeFrom()
  • ByteStreamBufferAdapterTester::random_fill() — only calls serializeFrom()
  • AmpcsEvrLogPacket::deserializeFrom() header declaration — fixes mismatch with .cpp (which already used SerialBufferBase&); body only calls deserializeTo() and getDeserializeSizeLeft()
  • DpContainerTester::isDataBufferEmpty() — local variable only calls getSize() and getDeserializeSizeLeft()

Changed to explicit LinearBufferBase&:

  • DpContainerHeader::checkDeserialAtOffset() — calls getBuffAddr() and getBuffAddrLeft()

Evaluated, no change needed:

  • AmpcsEvrLogPacket::serializeTo() — already SerialBufferBase&
  • ConstStringBase serialize/deserialize methods — already SerialBufferBase&
  • FprimeFrameDetector.cpp — only comments mentioning SerializeBufferBase, no parameter types
  • SerializeBufferBaseTester — accesses LinearBufferBase private members (m_serLoc, m_deserLoc) via friend class; must stay SerializeBufferBase&

Skipped (FPP codegen required):

  • InputPortBase::invokeSerial(), InputSerializePort::invokeSerial(), OutputPortBase::invokeSerial(), CompFuncPtr typedef
  • serialIn_handler / from_serialOut_handler signatures in GenericHub and tester
  • All tracked in new fpp-codegen-changes-needed.md

New documentation: fpp-codegen-changes-needed.md

Tracks locations where LinearBufferBase/SerializeBufferBase could become SerialBufferBase but are blocked by FPP code generator dependencies. Includes justifications and a summary table.

Rationale

This is part of a broader effort to use the abstract SerialBufferBase interface wherever possible, enabling future circular buffer implementations to be used interchangeably. The SerializeBufferBase alias obscures whether code actually needs linear/contiguous buffer access or just the serialize API.

Testing/Review Recommendations

Key items to verify:

  1. Phase 1 — Virtual method consistency: File::serializeFPrime()/serializeAMPCS() are declared as LinearBufferBase&. All 17 derived class overrides must match exactly.
  2. Phase 1 — SerialBufferBase correctness: Verify that AMPCS::Headers::serialize() and FPrime::Records::serialize() truly never call getBuffAddr().
  3. Phase 2 — Serialize-only verification: Confirm the following functions don't call pointer-access methods on their buffer parameters:
    • TmFramer::fill_with_idle_packet() (TmFramer.cpp:108–129)
    • AosFramer::serialize_idle_spp_packet() (AosFramer.cpp:343–364)
    • GenericHubTester::random_fill() (GenericHubTester.cpp:84–90)
    • AsyncByteStreamBufferAdapterTester::random_fill() (AsyncByteStreamBufferAdapterTester.cpp:46–51)
    • ByteStreamBufferAdapterTester::random_fill() (ByteStreamBufferAdapterTester.cpp:45–51)
  4. ⚠️ AmpcsEvrLogPacket header/impl mismatch: The .hpp declared deserializeFrom(SerializeBufferBase&) while the .cpp already used SerialBufferBase&. Since SerializeBufferBase is an alias for LinearBufferBase (which is a subclass of SerialBufferBase), this was a type mismatch between declaration and definition. Verify the fix is correct and the function compiles.
  5. ⚠️ Completeness: The Drv/ tester files were found reactively during review and were not in the original Phase 2 file list. A repo-wide grep has been performed, but reviewers should verify no other test utilities were missed.
  6. FPP tracking doc: Review fpp-codegen-changes-needed.md for accuracy of the deferred change analysis.
  7. Build verification: These changes have not yet been CI-verified. Compilation of affected components should be checked.

Future Work

  • Phase 3: Evaluate remaining Svc/ files (CmdSequencer, FpySequencer, PrmDb, TlmPacketizer, ComLogger, ComQueue, BufferLogger)
  • Phase 4: Document files that must stay LinearBufferBase
  • Phase 5: Address SerialBufferBase's own LinearBufferBase references (design decision)
  • Phase 6–7: Documentation updates, full build/test, alias cleanup

AI Usage (see policy)

AI (Devin) was used to systematically analyze each function body for getBuffAddr() calls, determine the correct replacement type, perform the mechanical edits, and produce the fpp-codegen-changes-needed.md tracking document. Each change decision was based on examining whether the function body (and its callees) access buffer data pointers.

Link to Devin session: https://nasa-jpl-demo.devinenterprise.com/sessions/913162b97ea340479b60953cdb2d3443

…fferBase/SerialBufferBase

Phase 1 of SerialBufferBase refactoring. Replace SerializeBufferBase alias
with the appropriate concrete type in Svc/CmdSequencer test utility files:

- Functions that call getBuffAddr() on buffer parameters use LinearBufferBase&
- Functions that only use serialize/deserialize API use SerialBufferBase&
- Virtual method overrides in derived classes match base class signatures

Changed to SerialBufferBase& (no pointer access needed):
- AMPCS/Headers: serialize() functions only call serializeFrom()
- FPrime/Headers: serialize() only calls serializeFrom()
- FPrime/Records: all serialize() overloads only call serializeFrom()
- AMPCS/Records: dest parameter only calls serializeFrom()

Kept as LinearBufferBase& (pointer access required):
- AMPCS/Records: cmdField parameter calls getBuffAddr()
- AMPCS/CRCs: computeCRC()/createFile() call getBuffAddr()
- FPrime/CRCs: serialize() calls getBuffAddr()
- Buffers: write() calls getBuffAddr(); FileBuffer IS a LinearBufferBase
- File base class: virtual methods must match derived implementations
- All derived file classes: override virtual methods from File base class

Co-Authored-By: cindy.h.oda <cindy.h.oda@jpl.nasa.gov>
@devin-ai-integration
Copy link
Copy Markdown
Author

Original prompt from cindy.h.oda

Repository: JPL-Devin/fprime (branch: devel)

#``# Goal
Change usages of LinearBufferBase (and its alias SerializeBufferBase) to SerialBufferBase wherever the code does NOT need direct pointer access to the buffer data (i.e., does not call getBuffAddr(), getBuffAddrLeft(), getBuffAddrSer(), or copyFrom()). This makes the code more abstract and allows future circular buffer implementations to be used interchangeably.

#``# Important Constraints

  1. SerialBufferBase must NEVER return a pointer to the data. Verify this is already the case (it is — confirmed in Fw/Types/Serializable.hpp lines 115-668). If any future changes are proposed that would add pointer-returning methods to SerialBufferBase, reject them.
  2. Concrete buffer class inheritance (e.g., class ComBuffer : public LinearBufferBase) must NOT change — these classes ARE linear buffers.
  3. Autocoded files and port handler signatures that pass buffers to getBuffAddr() downstream must stay LinearBufferBase.

#``# Process — Batched Review with Justifications
This refactoring should be done in small batches. For EACH change, provide:

  • The file and line(s) changed
  • What was changed (e.g., parameter type LinearBufferBase&amp;SerialBufferBase&amp;)
  • WHY: a brief justification explaining that the function body only uses serialize/deserialize API methods and does not call getBuffAddr() or similar pointer-access methods
  • Or WHY NOT: if a usage was evaluated and determined to need LinearBufferBase, explain why

After each batch, pause for user review. After several successful batches, ask the user if they want to reduce review frequency.

#``# Phase 0: Verify SerialBufferBase safety
Audit Fw/Types/Serializable.hpp lines 115-668 to confirm SerialBufferBase has no method that returns U8* or const U8* pointing to internal buffer data. Document this finding. (Already confirmed — it does not.)

#``# Phase 1: Evaluate and change Svc/CmdSequencer test utility files
These files in `Svc/CmdSequen... (4135 chars truncated...)

@devin-ai-integration
Copy link
Copy Markdown
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR that start with 'DevinAI' or '@devin'.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Copy Markdown
Author

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional findings.

Open in Devin Review

…Base/LinearBufferBase

- TmFramer::fill_with_idle_packet: SerializeBufferBase& -> SerialBufferBase& (serialize-only API)
- AosFramer::serialize_idle_spp_packet: SerializeBufferBase& -> SerialBufferBase& (serialize-only API)
- GenericHubTester::random_fill: SerializeBufferBase& -> SerialBufferBase& (serialize-only API)
- DpContainerHeader::checkDeserialAtOffset: SerializeBufferBase& -> LinearBufferBase& (calls getBuffAddr)
- AmpcsEvrLogPacket::deserializeFrom: fix header/impl mismatch, SerializeBufferBase& -> SerialBufferBase&
- Add fpp-codegen-changes-needed.md tracking FPP-blocked changes

Co-Authored-By: cindy.h.oda <cindy.h.oda@jpl.nasa.gov>
@devin-ai-integration devin-ai-integration bot changed the title Refactor CmdSequencer test utilities: SerializeBufferBase to LinearBufferBase/SerialBufferBase Refactor SerializeBufferBase to LinearBufferBase/SerialBufferBase (Phases 1-2) Apr 9, 2026
devin-ai-integration bot and others added 3 commits April 9, 2026 23:27
… in isDataBufferEmpty

Local variable only calls getSize() and getDeserializeSizeLeft() — no pointer access needed.

Co-Authored-By: cindy.h.oda <cindy.h.oda@jpl.nasa.gov>
…SerialBufferBase&

random_fill() only calls serializeFrom() — no pointer access needed.

Co-Authored-By: cindy.h.oda <cindy.h.oda@jpl.nasa.gov>
…lBufferBase&

random_fill() only calls serializeFrom() — no pointer access needed.

Co-Authored-By: cindy.h.oda <cindy.h.oda@jpl.nasa.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants