nasa / nasa/CF

Use of globals to store ephemeral / in-transit data

Open
#91 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

enhancement
Dominant language
C
Stars
120
Forks
77
Avg merge
7d 18h
Merged PRs (30d)
3

Description

CF stores its current working pointers in a global variable called CF_AppData.engine:

In particular:
CF_AppData.engine.in.msg has a pointer to the buffer last received from SB
CF_AppData.engine.in.bytes_received has the size of that buffer (and is actually updated during the course of processing)
CF_AppData.engine.in.src and CF_AppData.engine.in.dst have the data extracted from the header of the most recent message.
and so forth...

Importantly: none of these values are supposed to be carried across wakeups. All values are reset in their entirety on every wakeup, and in fact with each channel. All data is ephemeral and is only valid while the CFDP app is actively processing that packet. As soon as processing of the current packet completes, the data is no longer valid. When the wakeup cycle completes, only the CF_AppData.engine.in.msg is actively cleared. All other fields will be left with whatever data was in them.

Also notable - there is a mixture of API calls where sometimes the pointer to the packet data is passed in directly, as it is here via the ph argument:

https://github.com/nasa/CF/blob/7b99b91cd50a347f8553fc68ea3b074ff0672251/fsw/src/cf_cfdp_r.c#L436-L447

The call to CF_CFDP_R_ProcessFd also needs the packet data, but it does not pass it along. Instead, this function grabs it from the global (theoretically the same packet):

https://github.com/nasa/CF/blob/7b99b91cd50a347f8553fc68ea3b074ff0672251/fsw/src/cf_cfdp_r.c#L221-L224

This inconsistency should be addressed. If the intent is to always store the current packet in a global, then code needing to access it should always retrieve it from the global. There should not be some APIs which pass a pointer to the structure, mixed with others that get it directly from the global (where they are supposed to be acting on the same data), as this creates the opportunity that they could diverge.

For ephemeral data, it is fine to pre-allocate a buffer in a global to avoid dynamic allocation, but the pointer to this data should be passed consistently down through the API where needed. This design allows for safer evolution, permitting the use of multiple buffers or even multiple threads should that become a requirement (e.g. create a child task per CFDP channel to make them more independent).

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in fsw/src/cf_cfdp_r.c at CF_CFDP_R_ProcessFd and the caller around lines 436-447, then trace how packet data moves through the CFDP processing APIs. Done means the packet data access strategy is consistent throughout these calls and ephemeral state is not implicitly mixed with passed pointers; no tests or additional files are identified in the issue.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
backend-api-design, embedded-iot
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.