new returnvalues, RETURN_FAILED is 0xFFFF now #120

muellerr · 2020-06-23T13:04:02+02:00

muellerr commented

2020-06-23 13:04:02 +02:00

Include guard updated

muellerr added the

feature

label 2020-06-23 13:04:02 +02:00

mohr commented

2020-06-23 13:48:02 +02:00

I am not sure if you went for 0xffff exactly or for an integer of all ones. If the latter, I suggest using -1 as the initializer, which will produce all ones, regardless of the size of the datatype. That would make the definition more robust against redefinition of ReturnValue_t. The point of typedef'ing ReturnValue_t instead of using uint16_t directly is to be able to change the underlying datatype later on.

I am not sure if you went for 0xffff exactly or for an integer of all ones. If the latter, I suggest using -1 as the initializer, which will produce all ones, regardless of the size of the datatype. That would make the definition more robust against redefinition of `ReturnValue_t`. The point of typedef'ing `ReturnValue_t` instead of using `uint16_t` directly is to be able to change the underlying datatype later on.

muellerr commented

2020-06-23 15:09:58 +02:00

Will implement that.

mohr commented

2020-06-24 10:27:03 +02:00

And two more thoughts:

The MAKE_RETURN_CODE macro defines the upper Byte to be the interface ID. Having RETURN_FAILED be -1 moves into the -1 interface ID while RETURN_OK is still in the default 0 interface ID.
Why do we need to change the numeric value of RETURN_FAILED? As far as I am concerned, the actual value is used only at this one point, where it is defined. When parsing the TM on ground, the same approach should be used having it defined once and then never use the numeric value again.

When defining the current values, RETURN_OK was decided to be 0, so one can easier spot if something is wrong (ie != 0x0000) in raw TM, but that is about as far as I would go in tailoring the numeric values.

And two more thoughts: 1. The `MAKE_RETURN_CODE` macro defines the upper Byte to be the interface ID. Having `RETURN_FAILED` be -1 moves into the -1 interface ID while `RETURN_OK` is still in the default 0 interface ID. 2. Why do we need to change the numeric value of `RETURN_FAILED`? As far as I am concerned, the actual value is used only at this one point, where it is defined. When parsing the TM on ground, the same approach should be used having it defined once and then never use the numeric value again. When defining the current values, `RETURN_OK` was decided to be 0, so one can easier spot if something is wrong (ie != 0x0000) in raw TM, but that is about as far as I would go in tailoring the numeric values.

muellerr commented

2020-06-24 11:29:50 +02:00

This is something I found in the ISIS drivers:

/*!
 * @file	boolean.h
 * @brief	Exposes some boolean data types.
 *
 * Values like 0xFFFFFFFF and 0xFF are used instead of 1 for reliability as this increases  the
 * number of bit-flips that turn a TRUE into FALSE. Preference is placed within ISIS drivers
 * to use comparisons with FALSE.
 *
 * @verbatim
 	 // Instead of
 	 if(flag == TRUE) {
 	 	 takeAction();
 	 }
 	 // We use
 	 if(flag != FALSE) {
 	 	 takeAction();
 	 }
 	 // Or
	 if(flag) {
 	 	 takeAction();
 	 }
 * @endverbatim
 *
 * @date	Aug 03, 2012
 * @author	Akhil Piplani
 */

#ifndef BOOLEAN_H_
#define BOOLEAN_H_

typedef unsigned int  Boolean;
typedef unsigned char Boolean8bit;

#define TRUE		0xFFFFFFFF
#define FALSE		0

#define TRUE_8BIT	0xFF
#define FALSE_8BIT	0

#endif /* BOOLEAN_H_ */

They used the all-one value for true though..

I added a static makeReturnCode function inside the interface as well, similar to the makeEvent function for events.

This is something I found in the ISIS drivers: ```cpp /*! * @file boolean.h * @brief Exposes some boolean data types. * * Values like 0xFFFFFFFF and 0xFF are used instead of 1 for reliability as this increases the * number of bit-flips that turn a TRUE into FALSE. Preference is placed within ISIS drivers * to use comparisons with FALSE. * * @verbatim // Instead of if(flag == TRUE) { takeAction(); } // We use if(flag != FALSE) { takeAction(); } // Or if(flag) { takeAction(); } * @endverbatim * * @date Aug 03, 2012 * @author Akhil Piplani */ #ifndef BOOLEAN_H_ #define BOOLEAN_H_ typedef unsigned int Boolean; typedef unsigned char Boolean8bit; #define TRUE 0xFFFFFFFF #define FALSE 0 #define TRUE_8BIT 0xFF #define FALSE_8BIT 0 #endif /* BOOLEAN_H_ */ ``` They used the all-one value for true though.. I added a static makeReturnCode function inside the interface as well, similar to the makeEvent function for events.

muellerr referenced this issue from a commit

2020-07-02 14:45:05 +02:00

Merge pull request 'new returnvalues, RETURN_FAILED is 0xFFFF now' (#120) from KSat/fsfw:mueller_returnvalues into master

muellerr closed this pull request

2020-07-02 14:45:06 +02:00

muellerr commented

2020-07-02 14:45:33 +02:00

RETURN_FAILED remains 1. That change was rather esoteric.

muellerr deleted branch mueller_returnvalues

2020-07-02 14:45:41 +02:00

Sign in to join this conversation.

No reviewers