Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #116 (plus others), separate logical vs. network PDU buffers #137

Merged
merged 2 commits into from
Jan 6, 2022

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Dec 17, 2021

Improves the distinction between PDU data being actively interpreted or created during the PDU receive or transmit process, and the encoded form of that data.

CF formerly treated the two as the same, directly referencing the encoded form of the data. This creates many repeated translations. Furthermore, it would sometimes write a modified value back to the packet in a partially-decoded form, so it was never clear what was in a given buffer at a given time (it could be native byte order or network byte order, in the same fields).

This introduces a "logical" buffer which correlates to the CFDP buffer, but is used for all in-work or temporary value storage.
All values in the logical buffer are normalized to the native machine, that is they are aligned properly and always in the
correct byte order for the host, so they can be used as normal values without any need for translation.

When it comes time to transmit data to/from the network, a dedicated Encode/Decode function is used, to translate the
entire content from its native form to the network form, or vice versa.

FSW should typically not access the encoded form of data, outside of the codec routines, except under very limited
circumstances with good reason (such as dynamically updating the total_length field in the base header after encode).

Fixes #116
Fixes #71
Fixes #68
Fixes #37
Fixes #35
Fixes #34
Fixes #33
Fixes #28

Also related to #61, #91, #95, #129, #109 - makes progress toward those goals but more work still needed.

@jphickey
Copy link
Contributor Author

This draft PR is intended to let folks get a preview of what's to come here. Important thing missing at this point is the unit tests, at this point I have unit tests disabled because they are NOT updated to work with these FSW changes yet. Probably will need to take a stab at #87 first, which should make it easier.

Happy to hear any feedback about the proposed changes, if anyone has time to review them, it would be helpful.

@jphickey jphickey force-pushed the fix-116-encode-decode-revamp branch 2 times, most recently from c074b39 to c295364 Compare December 17, 2021 21:14
@jphickey jphickey force-pushed the fix-116-encode-decode-revamp branch 3 times, most recently from 3ba734d to 9d0cdc8 Compare January 5, 2022 16:47
Improves the distinction between PDU data being actively interpreted
or created during the PDU receive or transmit process, and the encoded
form of that data.

CF formerly treated the two as the same, directly referencing the
encoded form of the data.  This creates many repeated translations.
Furthermore, it would sometimes write a modified value back to the
packet in a partially-decoded form, so it was never clear what
was in a given buffer at a given time (it could be native byte
order or network byte order, in the same fields).

This introduces a "logical" buffer which correlates to the CFDP
buffer, but is used for all in-work or temporary value storage.
All values in the logical buffer are normalized to the native
machine, that is they are aligned properly and always in the
correct byte order for the host, so they can be used as normal
values without any need for translation.

When it comes time to transmit data to/from the network, a
dedicated Encode/Decode function is used, to translate the
entire content from its native form to the network form, or
vice versa.

FSW should typically not access the encoded form of data,
outside of the codec routines, except under very limited
circumstances with good reason (such as dynamically updating
the total_length field in the base header after encode).
@jphickey jphickey force-pushed the fix-116-encode-decode-revamp branch from 9d0cdc8 to 98801ae Compare January 5, 2022 16:52
@jphickey
Copy link
Contributor Author

jphickey commented Jan 5, 2022

Updated for initial review (preview) ...

This is currently focused on the proposed FSW changes.

Right now this has the unit test build disabled because the FSW changes break it. Required unit test updates will be added on in an additional commit.

@astrogeco
Copy link
Contributor

astrogeco commented Jan 5, 2022

CCB:2022-01-05 APPROVED

  • Splitting up functionality helps unit testing organization
  • Important to keep "unrelated functionality" separated into different c file and that each c-file has a companion h file
  • Remove need to #include c files by removing static definitions.
    • Should we have private headers for some of these?
    • Should we have a naming convention for private functions?
    • Traditionally, public APIs are placed in an inc directory while private ones are placed in src
    • In apps "everything" is private. Symbols cannot be found
  • Missing a little bit of coverage that should be done soon. Need to figure out what to do with the "asserts".
  • Do we anticipate performance changes from this change?
    • Unlikely
  • Might want to "revive" the fuzz testing approach separately at a later moment

A significant cleanup and overhaul of CF unit tests
to follow patterns that are more consistent with
other CFE/OSAL test modules.

Since the FSW change in this PR requires significant test updates
to go with it, this replaces all the "cfdp" tests with new
implementations where there is a 1:1 ratio between test
functions and implementation functions.

Ths also does some minor cleanup on the FSW side where needed
for testability.

Note that all stub files in this version are now directly generated
using the tool provided with UtAssert.  These generated stubs should
not be modified.
@jphickey jphickey force-pushed the fix-116-encode-decode-revamp branch from 4137c78 to 3acc447 Compare January 6, 2022 18:35
@jphickey
Copy link
Contributor Author

jphickey commented Jan 6, 2022

Commit 3acc447 updates all the unit tests to work with the FSW changes here. Back up to 100%/100% function/line coverage for all modified modules. Noted some other issues along the way but those can be fixed separately (issues written).

This is done now....

@jphickey jphickey marked this pull request as ready for review January 6, 2022 18:37
@jphickey
Copy link
Contributor Author

jphickey commented Jan 6, 2022

The latest commit also added a fix for #33, as the (new) unit tests also revealed the fact that this did a null dereference here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment