Skip to content

Commit

Permalink
Fix nasa#297, CCSDS sec cmd hdr consistancy
Browse files Browse the repository at this point in the history 
Implemented CCSDS command secondary header such that it
is endian agnostic.
  • Loading branch information
@skliper
skliper committed Mar 25, 2020
1 parent 5408523 commit a8c24ea
Showing 1 changed file with 12 additions and 10 deletions.
22 changes: 12 additions & 10 deletions fsw/cfe-core/src/inc/ccsds.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,12 @@ typedef struct {

typedef struct {

uint16 Command; /* command secondary header */
/* bits shift ------------ description ---------------- */
/* 0x00FF 0 : checksum, calculated by ground system */
/* 0x7F00 8 : command function code */
/* 0x8000 15 : reserved, set to 0 */
uint8 FunctionCode; /* Command Function Code */
/* bits shift ---------description-------- */
/* 0x7F 0 Command function code */
/* 0x80 7 Reserved */

uint8 Checksum; /* Command checksum (all bits, 0xFF) */

} CCSDS_CmdSecHdr_t;

Expand Down Expand Up @@ -326,14 +327,14 @@ typedef CCSDS_TelemetryPacket_t CCSDS_TlmPkt_t;
(((phdr).Length[1] = ((value) - 7) & 0xff)) )

/* Read function code from command secondary header. */
#define CCSDS_RD_FC(shdr) CCSDS_RD_BITS((shdr).Command, 0x7F00, 8)
#define CCSDS_RD_FC(shdr) CCSDS_RD_BITS((shdr).FunctionCode, 0x7F, 0)

This comment has been minimized.

Sign in to view

Copy link
@yashi

yashi Apr 3, 2020

What is the point of using the macros, CCSD_RD_BITS and CCSDS_WR_BITS, on 8 bit field?

This comment has been minimized.

Sign in to view

Copy link
@skliper

skliper Apr 3, 2020

Author Owner

No point. The macro reduces to the same thing as ((shdr).Functioncode & 0x7F) so I left it.

This comment has been minimized.

Sign in to view

Copy link
@yashi

yashi Apr 4, 2020

But you don't use a macro in case of CCSDS_RD_CHECKSUM() down below even though it reduced to be none. Having a macro looks consistent but from the maintenance point of view we should not use it where it's not needed, because it's easier to see what's used where. We can remove unused macros.

This comment has been minimized.

Sign in to view

Copy link
@skliper

skliper Apr 6, 2020

Author Owner

CCSDS_RD_CHECKSUM doesn't need a mask or shift. FunctionCode needed a mask, so we could have gone either way (use full macro, or do a one-off mask). It's not the only use of the macro so removing use here wouldn't allow removal of the macro.

This comment has been minimized.

Sign in to view

Copy link
@yashi

yashi Apr 9, 2020

I really don't want to be a pain for you but CCSDS_RD_BITS and CCSDS_WR_BITS aren't up to the task because it's not endian aware. Using them likely cause yet another trouble in near future. With this in mind I said it's over kill to use it just for uint8.

What's wrong with shdr.FunctionCode & 0x7F? it's simple, clear, and removes one more (broken) abstraction layer.

If we don't use the macros here, there are only 4 users:

#define CCSDS_SID_APID(sid)   CCSDS_RD_BITS(sid, 0x07FF, 0)
#define CCSDS_SID_SHDR(sid)   CCSDS_RD_BITS(sid, 0x0800, 11)
#define CCSDS_SID_TYPE(sid)   CCSDS_RD_BITS(sid, 0x1000, 12)
#define CCSDS_SID_VERS(sid)   CCSDS_RD_BITS(sid, 0xE000, 13)

This comment has been minimized.

Sign in to view

Copy link
@skliper

skliper Apr 9, 2020

Author Owner

Wow, those CCSDS_SID_XXXX macros are bad. Apps typically should avoid accessing CCSDS directly but better to use CCSDS_RD_SID/CCSDS_WR_SID which at first glance looks like it handles the endianness correctly. Instead use the abstracted functions: CFE_SB_InitMsg, CFE_SB_GetMsgId, CFE_SB_SetMsgId, etc.

The CCSDS_RD_BITS is just a mask and shift, it's just being used incorrectly in the example above (the macro would be wrong even if you did just a mask and shift). That said, I'd much rather just see the mask and shift than this abstraction.

See nasa#597

This comment has been minimized.

Sign in to view

Copy link
@jphickey

jphickey Apr 9, 2020

The input value to CCSDS_RD_BITS is supposed to already be in native endianness. The macros which read the fields from the header should be endian-neutral and then this macro can be used.

I concur its confusing/not ideal, I'm not sure its worth fussing over, because in a future version the endian issues can be solved in a better way.

This comment has been minimized.

Sign in to view

Copy link
@yashi

yashi Apr 10, 2020

If CCSDS_RD_BITS and co. are not accessing CCSDS, can we just rename them to more generic name, like BITS_READ16() or more generic BITS_GET()?
/* Write function code to command secondary header. */
#define CCSDS_WR_FC(shdr,value) CCSDS_WR_BITS((shdr).Command, 0x7F00, 8, value)
#define CCSDS_WR_FC(shdr,value) CCSDS_WR_BITS((shdr).FunctionCode, 0x7F, 0, value)

/* Read checksum from command secondary header. */
#define CCSDS_RD_CHECKSUM(shdr) CCSDS_RD_BITS((shdr).Command, 0x00FF, 0)
#define CCSDS_RD_CHECKSUM(shdr) ((shdr).Checksum)
/* Write checksum to command secondary header. */
#define CCSDS_WR_CHECKSUM(shdr,val) CCSDS_WR_BITS((shdr).Command, 0x00FF, 0, val)
#define CCSDS_WR_CHECKSUM(shdr,val) ((shdr).Checksum = (val))

This comment has been minimized.

Sign in to view

Copy link
@yashi

yashi Apr 3, 2020

This will generate a warning if the given val is not uint8 and -Wconversion is present.

This comment has been minimized.

Sign in to view

Copy link
@skliper

skliper Apr 3, 2020

Author Owner

As it should? I'd rather not cast inside the macro...

This comment has been minimized.

Sign in to view

Copy link
@yashi

yashi Apr 4, 2020

So, you want all the call site to use uint8 or cast to it?

This comment has been minimized.

Sign in to view

Copy link
@skliper

skliper Apr 6, 2020

Author Owner

User choice. Cast minimizes other code changes but impacts of either choice should be assessed by the user (overflow, rollover, etc).

/* Define additional APID Qualifier macros. */

Expand Down Expand Up @@ -378,7 +379,8 @@ typedef CCSDS_TelemetryPacket_t CCSDS_TlmPkt_t;

/* Clear command secondary header. */
#define CCSDS_CLR_CMDSEC_HDR(shdr) \
( (shdr).Command = (CCSDS_INIT_CHECKSUM << 0) | (CCSDS_INIT_FC << 8) )
( (shdr).FunctionCode = CCSDS_INIT_FC,\
(shdr).Checksum = CCSDS_INIT_CHECKSUM )


#define CCSDS_WR_SEC_HDR_SEC(shdr, value) shdr.Time[0] = ((value>>24) & 0xFF), \
Expand Down

0 comments on commit a8c24ea

Please sign in to comment.