Skip to content

Add CFDP support to Fprime#11

Open
pepepr08 wants to merge 185 commits intomonarchfrom
cfdp_manager
Open

Add CFDP support to Fprime#11
pepepr08 wants to merge 185 commits intomonarchfrom
cfdp_manager

Conversation

@pepepr08
Copy link
Copy Markdown
Collaborator

@pepepr08 pepepr08 commented Apr 15, 2026

Related Issue(s)
Has Unit Tests (y/n) y
Documentation Included (y/n) y
Generative AI was used in this contribution (y/n) y

Change Description

This change adds CfdpManager, a component that provides CFDP support to Fprime for acknowledged file transfers.

Rationale

CCSDS File Delivery Protocol provides class 2, acknowledged transactions with guaranteed delivery.

Testing/Review Recommendations

Future Work

AI Usage (see policy)

Comment thread .github/actions/spelling/expect.txt Fixed
Comment thread .github/actions/spelling/expect.txt Fixed
Comment thread Os/File.hpp
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you do a pass to make sure that there's no diff where there doesn't need to be?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@Brian-Campuzano Any chance you remember why it was needed to fix the comment offsets? 2668bab

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pretty sure it was just the inconsistency triggering my OCD.

Copy link
Copy Markdown

@github-advanced-security github-advanced-security AI left a comment

Choose a reason for hiding this comment

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

check-spelling found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

...because another more general variant is also in expect.
@pepepr08 pepepr08 marked this pull request as ready for review April 17, 2026 13:01
@pepepr08
Copy link
Copy Markdown
Collaborator Author

There is a CFDP directory here that we don't touch in this PR. It can be confusing for people to find CfdpManager elsewhere. I'm not saying we added CfdpManager to the wrong place, but is there anything we can do to avoid this confusion?

@Brian-Campuzano
Copy link
Copy Markdown
Member

There is a CFDP directory here that we don't touch in this PR. It can be confusing for people to find CfdpManager elsewhere. I'm not saying we added CfdpManager to the wrong place, but is there anything we can do to avoid this confusion?

I agree that it is confusing, however I was unable to move the checksum sub-directory into CFDP manager as it is used in the Framework FilePacket. One potential long term solution would be to move the checksum utility class into a framework folder. In the interim we could add a README.md file to the CFDP folder explaining its usecases.

Comment thread Os/File.hpp
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pretty sure it was just the inconsistency triggering my OCD.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@pepepr08 does this need to be updated based on your ComQueue integration updates?

| Name | Type | Port Type | Description |
|------|------|-----------|-------------|
| dataOut | output array[N] | `Fw.BufferSend` | Send encoded CFDP PDU data buffers to downstream components. One port (`N`) per CFDP channel. |
| dataReturnIn | async input array[N] | `Svc.ComDataWithContext` | Receive buffers previously sent via `dataOut` after downstream processing is complete. One port per CFDP channel. |
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this need to be updated for the ComQueue integration updates?

@Brian-Campuzano
Copy link
Copy Markdown
Member

I have also reviewed the changes in 6634aac

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.

4 participants