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

Default to 64 bit memory address/offset for all IO (cmd/tlm/tbl/binary dumps) #2093

Open
skliper opened this issue Apr 20, 2022 · 0 comments

Comments

@skliper
Copy link
Contributor

skliper commented Apr 20, 2022

Is your feature request related to a problem? Please describe.
CFE_ES_MemAddress_t/CFE_ES_MEMADDRESS_C and CFE_ES_MemOffset_t/CFE_ES_MEMOFFSET_C are used to convert cpuaddr into a fixed size for cmd/tlm/tbl/binary output. Currently they are both hard-coded to uint32:

/**
* @brief Type used for memory sizes and offsets in commands and telemetry
*
* For backward compatibility with existing CFE code this should be uint32,
* but all telemetry information will be limited to 4GB in size as a result.
*
* On 64-bit platforms this can be a 64-bit value which will allow larger
* memory objects, but this will break compatibility with existing control
* systems, and may also change the alignment/padding of messages.
*
* In either case this must be an unsigned type.
*/
typedef uint32 CFE_ES_MemOffset_t;
/*
* A converter macro to use when initializing a CFE_ES_MemOffset_t
* from an integer value of a different type.
*/
#define CFE_ES_MEMOFFSET_C(x) ((CFE_ES_MemOffset_t)(x))
/**
* @brief Type used for memory addresses in command and telemetry messages
*
* For backward compatibility with existing CFE code this should be uint32,
* but if running on a 64-bit platform, addresses in telemetry will be
* truncated to 32 bits and therefore will not be valid.
*
* On 64-bit platforms this can be a 64-bit address which will allow the
* full memory address in commands and telemetry, but this will break
* compatibility with existing control systems, and may also change
* the alignment/padding of messages.
*
* In either case this must be an unsigned type.
*
* FSW code should access this value via the macros provided, which
* converts to the native "cpuaddr" type provided by OSAL. This macro
* provides independence between the message representation and local
* representation of a memory address.
*/
typedef uint32 CFE_ES_MemAddress_t;
/*
* A converter macro to use when initializing a CFE_ES_MemAddress_t
* from a pointer value of a different type.
*
* @note on a 64 bit platform, this macro will truncate the address such
* that it will fit into a 32-bit telemetry field. Obviously, the resulting
* value is no longer usable as a memory address after this.
*/
#define CFE_ES_MEMADDRESS_C(x) ((CFE_ES_MemAddress_t)((cpuaddr)(x)&0xFFFFFFFF))

Problem is this basically is just wasted space on 64 bit and also requires querying a separate element to know if these values are valid. Also ends up reducing functionality (and not supporting some requirements on certain apps).

uint32 AddressesAreValid; /**< \cfetlmmnemonic \ES_ADDRVALID
\brief Indicates that the Code, Data, and BSS addresses/sizes are valid */
CFE_ES_MemAddress_t CodeAddress; /**< \cfetlmmnemonic \ES_CODEADDR
\brief The Address of the Application Code Segment*/
CFE_ES_MemOffset_t CodeSize; /**< \cfetlmmnemonic \ES_CODESIZE
\brief The Code Size of the Application */
CFE_ES_MemAddress_t DataAddress; /**< \cfetlmmnemonic \ES_DATAADDR
\brief The Address of the Application Data Segment*/
CFE_ES_MemOffset_t DataSize; /**< \cfetlmmnemonic \ES_DATASIZE
\brief The Data Size of the Application */
CFE_ES_MemAddress_t BSSAddress; /**< \cfetlmmnemonic \ES_BSSADDR
\brief The Address of the Application BSS Segment*/
CFE_ES_MemOffset_t BSSSize; /**< \cfetlmmnemonic \ES_BSSSIZE
\brief The BSS Size of the Application */
CFE_ES_MemAddress_t StartAddress; /**< \cfetlmmnemonic \ES_STARTADDR

There's also cases where validity isn't actually indicated within the element as in tbl tlm:

typedef struct CFE_TBL_TblRegPacket_Payload
{
CFE_ES_MemOffset_t Size; /**< \cfetlmmnemonic \TBL_SIZE
\brief Size, in bytes, of Table */
uint32 Crc; /**< \cfetlmmnemonic \TBL_CRC
\brief Most recently calculated CRC of Table */
CFE_ES_MemAddress_t ActiveBufferAddr; /**< \cfetlmmnemonic \TBL_ACTBUFADD
\brief Address of Active Buffer */
CFE_ES_MemAddress_t InactiveBufferAddr; /**< \cfetlmmnemonic \TBL_IACTBUFADD
\brief Address of Inactive Buffer */
CFE_ES_MemAddress_t ValidationFuncPtr; /**< \cfetlmmnemonic \TBL_VALFUNCPTR
\brief Ptr to Owner App's function that validates tbl contents */
CFE_TIME_SysTime_t TimeOfLastUpdate; /**< \cfetlmmnemonic \TBL_TIMELASTUPD
\brief Time when Table was last updated */
uint32 FileCreateTimeSecs; /**< \cfetlmmnemonic \TBL_FILECSECONDS
\brief File creation time from last file loaded into table */
uint32 FileCreateTimeSubSecs; /**< \cfetlmmnemonic \TBL_FILECSUBSECONDS
\brief File creation time from last file loaded into table */
bool TableLoadedOnce; /**< \cfetlmmnemonic \TBL_LOADEDONCE
\brief Flag indicating whether table has been loaded once or not */
bool LoadPending; /**< \cfetlmmnemonic \TBL_UPDATEPNDNG
\brief Flag indicating an inactive buffer is ready to be copied */
bool DumpOnly; /**< \cfetlmmnemonic \TBL_DUMPONLY
\brief Flag indicating Table is NOT to be loaded */
bool DoubleBuffered; /**< \cfetlmmnemonic \TBL_DBLBUFFERED
\brief Flag indicating Table has a dedicated inactive buffer */
char Name[CFE_MISSION_TBL_MAX_FULL_NAME_LEN]; /**< \cfetlmmnemonic \TBL_NAME
\brief Processor specific table name */
char LastFileLoaded[CFE_MISSION_MAX_PATH_LEN]; /**< \cfetlmmnemonic \TBL_LASTFILEUPD
\brief Filename of last file loaded into table */
char OwnerAppName[CFE_MISSION_MAX_API_LEN]; /**< \cfetlmmnemonic \TBL_OWNERAPP
\brief Name of owning application */
bool Critical; /**< \cfetlmmnemonic \TBL_CRITICAL
\brief Indicates whether table is Critical or not */
uint8 ByteAlign4; /**< \cfetlmmnemonic \TBL_SPARE4
\brief Spare byte to maintain byte alignment */
} CFE_TBL_TblRegPacket_Payload_t;

Note MM, CS, and MD also all have memory addresses in IO.

Describe the solution you'd like
Default to 64 bit everywhere and reorganize IO to avoid implicit padding where possible based on the change. This is a breaking change (IO change) so probably appropriate to either provide backwards compatibility or target a major release.

This does waste space on 32-bit systems, but makes it such that the values are always valid (no longer need a special flag as long as the underlying functionality is still valid). None of the uses are in what would typically be high rate telemetry so should be minimal impact. If really need we could support configurability, or projects could locally modify if really needed.

Describe alternatives you've considered
None

Additional context
Discussed at 20220420 CCB

Requester Info
Jacob Hageman - NASA/GSFC

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

No branches or pull requests

1 participant