CFE_SB_Default_Qos exposed globally, with internal defaults, passing structure by value

[skliper]

Is your feature request related to a problem? Please describe.
CFE_SB_Default_Qos is extern from cfe_sb.h (so any code including cfe_sb.h could change it's value!):

cFE/fsw/cfe-core/src/inc/cfe_sb.h

Lines 175 to 185 in c7363c8

	/** \brief Quality Of Service Type Definition
	**
	** Currently an unused parameter in #CFE_SB_SubscribeEx
	** Intended to be used for interprocessor communication only
	**/
	typedef struct {
	uint8 Priority;/**< \brief Specify high(1) or low(0) message priority for off-board routing, currently unused */
	uint8 Reliability;/**< \brief Specify high(1) or low(0) message transfer reliability for off-board routing, currently unused */
	}CFE_SB_Qos_t;
	extern CFE_SB_Qos_t CFE_SB_Default_Qos;/**< \brief Defines a default priority and reliabilty for off-board routing */

But the defines are "internal" to SB:

cFE/fsw/cfe-core/src/sb/cfe_sb_priv.h

Lines 86 to 87 in c7363c8

	#define CFE_SB_QOS_LOW_PRIORITY 0
	#define CFE_SB_QOS_LOW_RELIABILITY 0

Also structure passed by value:

cFE/fsw/cfe-core/src/inc/cfe_sb.h

Line 381 in c7363c8

CFE_Status_t CFE_SB_SubscribeEx(CFE_SB_MsgId_t MsgId, CFE_SB_PipeId_t PipeId, CFE_SB_Qos_t Quality, uint16 MsgLim);

There is no underlying implementation, so currently just a placeholder in the API.

Describe the solution you'd like
Possibly convert to bits in a uint32/16 or similar (structure is overkill), provide default and the other values publicly as defines, don't expose as a global variable.

Describe alternatives you've considered
None

Additional context
Found when working #1036, it's out of family since it's not at task global scope.

Requester Info
Jacob Hageman

[skliper]

@ejtimmon Note CFE_SB_Default_Qos is referenced by ds and hs (and in sch test utils)

[jphickey]

Passing by value is generally OK so long as the structure is small (i.e. 32 bits or less) in case the calling convention in use relies on register(s) to pass argument/return values. However if it grows larger it can become inefficient to pass by value.

IIRC the one I was worried about before was CFE_TIME_SysTime_t - this is passed by value and it is 64 bits in size, therefore could be inefficient on 32-bit machines depending on the calling convention and particularly the return value.

But this one is only 16 bits so its probably fine. But it would be a good idea to note in the structure/declaration comments that it is passed by value and therefore needs to be kept small for this reason - so fields shouldn't be added without careful consideration.

[skliper]

I understand it's OK in this case since it's small, but the extra caution/complexity and currently implementation really doesn't seem justifiable based on a lack of implementation. I'd think a generic uint and setting bits is a more flexible, future proof implementation that needs no special note. Could easily add implementations as modules and assign more bits (or have reserved/user defined bits), etc without having to redefine the elements in the structure (until bits run out).

[jphickey]

Sure, bit fields in an integer could work too - but then you lose the advantage of type safety protection from having it as a struct. That is, one has to declare their local variable as a CFE_SB_Qos_t or else they get a type mismatch when they try to pass it to the API. If it is uint32 then any integer will be accepted - regardless of whether the value is actually compatible or not (including short/long/signed/unsigned/literals - pretty much anything).

I definitely agree with moving the constants out of the "private" file and into a public header file - but I would hesitate to actually change the API at this time.