From cefc41ea3afc78b8fcdae0dc3770f59d8ddddab4 Mon Sep 17 00:00:00 2001 From: Joseph Hickey Date: Tue, 11 Jan 2022 08:44:33 -0500 Subject: [PATCH] Fix #63, remove macros within C code Reworks the CF_CmdGetSetParam to be clearer in its implementation, not require the use of endian-specific conditionally-compiled code. --- fsw/src/cf_cmd.c | 150 ++++++++++++++++++++++----------- fsw/src/cf_cmd.h | 2 +- fsw/src/cf_msg.h | 25 +++++- unit-test/cf_cmd_tests.c | 24 +++--- unit-test/stubs/cf_cmd_stubs.c | 4 +- 5 files changed, 141 insertions(+), 64 deletions(-) diff --git a/fsw/src/cf_cmd.c b/fsw/src/cf_cmd.c index 54748e711..a66f2f042 100644 --- a/fsw/src/cf_cmd.c +++ b/fsw/src/cf_cmd.c @@ -911,43 +911,72 @@ int CF_CmdValidateMaxOutgoing(uint32 val, uint8 chan_num) *************************************************************************/ /* combine getset into a single function with a branch to avoid wasted memory footprint with duplicate * logic for finding the parameter */ -void CF_CmdGetSetParam(uint8 is_set, uint8 param_id, uint32 value, uint8 chan_num) +void CF_CmdGetSetParam(uint8 is_set, CF_GetSet_ValueID_t param_id, uint32 value, uint8 chan_num) { - /* These macros define entries into the paramater array. The mapping of the array is - * ground parameter to configuration parameter. This logic allows a simple access - * of the configuration parameter or a check on validity of the parameter and then - * access. */ -#define SPTR(x) \ - { \ - &CF_AppData.config_table->x, sizeof(CF_AppData.config_table->x), NULL \ - } -#define SPTRFN(x, fn) \ - { \ - &CF_AppData.config_table->x, sizeof(CF_AppData.config_table->x), fn \ - } -#define CPTRFN(x, fn) \ - { \ - &CF_AppData.config_table->chan[chan_num].x, sizeof(CF_AppData.config_table->chan[chan_num].x), fn \ - } - const struct + CF_ConfigTable_t *config; + int acc; + bool valid_set; + struct { void *ptr; uint32 size; int (*fn)(uint32, uint8 chan_num); - } items[CF_NUM_CFG_PACKET_ITEMS] = { - SPTR(ticks_per_second), SPTR(rx_crc_calc_bytes_per_wakeup), - SPTR(ack_timer_s), SPTR(nak_timer_s), - SPTR(inactivity_timer_s), SPTRFN(outgoing_file_chunk_size, CF_CmdValidateChunkSize), - SPTR(ack_limit), SPTR(nak_limit), - SPTR(local_eid), CPTRFN(max_outgoing_messages_per_wakeup, CF_CmdValidateMaxOutgoing)}; + } item; - int acc = 1; /* 1 means to reject */ + acc = 1; /* 1 means to reject */ + valid_set = false; + config = CF_AppData.config_table; + memset(&item, 0, sizeof(item)); -#if ENDIAN == _EB - static const int shift_map[5] = {0, 24, 16, 8, 0}; -#endif + switch (param_id) + { + case CF_GetSet_ValueID_ticks_per_second: + item.ptr = &config->ticks_per_second; + item.size = sizeof(config->ticks_per_second); + break; + case CF_GetSet_ValueID_rx_crc_calc_bytes_per_wakeup: + item.ptr = &config->rx_crc_calc_bytes_per_wakeup; + item.size = sizeof(config->rx_crc_calc_bytes_per_wakeup); + break; + case CF_GetSet_ValueID_ack_timer_s: + item.ptr = &config->ack_timer_s; + item.size = sizeof(config->ack_timer_s); + break; + case CF_GetSet_ValueID_nak_timer_s: + item.ptr = &config->nak_timer_s; + item.size = sizeof(config->nak_timer_s); + break; + case CF_GetSet_ValueID_inactivity_timer_s: + item.ptr = &config->inactivity_timer_s; + item.size = sizeof(config->inactivity_timer_s); + break; + case CF_GetSet_ValueID_outgoing_file_chunk_size: + item.ptr = &config->outgoing_file_chunk_size; + item.size = sizeof(config->outgoing_file_chunk_size); + item.fn = CF_CmdValidateChunkSize; + break; + case CF_GetSet_ValueID_ack_limit: + item.ptr = &config->ack_limit; + item.size = sizeof(config->ack_limit); + break; + case CF_GetSet_ValueID_nak_limit: + item.ptr = &config->nak_limit; + item.size = sizeof(config->nak_limit); + break; + case CF_GetSet_ValueID_local_eid: + item.ptr = &config->local_eid; + item.size = sizeof(config->local_eid); + break; + case CF_GetSet_ValueID_chan_max_outgoing_messages_per_wakeup: + item.ptr = &config->chan[chan_num].max_outgoing_messages_per_wakeup; + item.size = sizeof(config->chan[chan_num].max_outgoing_messages_per_wakeup); + item.fn = CF_CmdValidateMaxOutgoing; + break; + default: + break; + }; - if (param_id >= CF_NUM_CFG_PACKET_ITEMS) + if (item.size == 0) { CFE_EVS_SendEvent(CF_EID_ERR_CMD_GETSET_PARAM, CFE_EVS_EventType_ERROR, "CF: invalid configuration parameter id %d received", param_id); @@ -963,11 +992,9 @@ void CF_CmdGetSetParam(uint8 is_set, uint8 param_id, uint32 value, uint8 chan_nu if (is_set) { - int valid_set = 0; - - if (items[param_id].fn) + if (item.fn) { - if (!items[param_id].fn(value, chan_num)) + if (!item.fn(value, chan_num)) { valid_set = 1; } @@ -984,26 +1011,55 @@ void CF_CmdGetSetParam(uint8 is_set, uint8 param_id, uint32 value, uint8 chan_nu if (valid_set) { - CFE_EVS_SendEvent(CF_EID_INF_CMD_GETSET1, CFE_EVS_EventType_INFORMATION, - "CF: setting parameter id %d to %d", param_id, value); -#if ENDIAN == _EB - CF_Assert((items[param_id].size > 0) && (items[param_id].size < 5)); - value <<= shift_map[items[param_id].size]; -#endif - memcpy(items[param_id].ptr, &value, items[param_id].size); acc = 0; + + CFE_EVS_SendEvent(CF_EID_INF_CMD_GETSET1, CFE_EVS_EventType_INFORMATION, + "CF: setting parameter id %d to %lu", param_id, (unsigned long)value); + + /* Store value based on its size */ + if (item.size == sizeof(uint32)) + { + *((uint32 *)item.ptr) = value; + } + else if (item.size == sizeof(uint16)) + { + *((uint16 *)item.ptr) = value; + } + else if (item.size == sizeof(uint8)) + { + *((uint8 *)item.ptr) = value; + } + else + { + /* unimplemented store; this is a SW configuration error! */ + acc = 1; + } } } else { - memcpy(&value, items[param_id].ptr, items[param_id].size); -#if ENDIAN == _EB - CF_Assert((items[param_id].size > 0) && (items[param_id].size < 5)); - value >>= shift_map[items[param_id].size]; -#endif - CFE_EVS_SendEvent(CF_EID_INF_CMD_GETSET2, CFE_EVS_EventType_INFORMATION, "CF: parameter id %d = %d", param_id, - value); acc = 0; + + /* Read value depending on its size */ + if (item.size == sizeof(uint32)) + { + value = *((const uint32 *)item.ptr); + } + else if (item.size == sizeof(uint16)) + { + value = *((const uint16 *)item.ptr); + } + else if (item.size == sizeof(uint8)) + { + value = *((const uint8 *)item.ptr); + } + else + { + acc = 1; + } + + CFE_EVS_SendEvent(CF_EID_INF_CMD_GETSET2, CFE_EVS_EventType_INFORMATION, "CF: parameter id %d = %lu", param_id, + (unsigned long)value); } err_out: diff --git a/fsw/src/cf_cmd.h b/fsw/src/cf_cmd.h index 8fec7a60a..d655d0c96 100644 --- a/fsw/src/cf_cmd.h +++ b/fsw/src/cf_cmd.h @@ -116,7 +116,7 @@ void CF_CmdWriteQueue(CFE_SB_Buffer_t *msg); void CF_CmdSendCfgParams(CFE_SB_Buffer_t *msg); int CF_CmdValidateChunkSize(uint32 val, uint8 chan_num /* ignored */); int CF_CmdValidateMaxOutgoing(uint32 val, uint8 chan_num); -void CF_CmdGetSetParam(uint8 is_set, uint8 param_id, uint32 value, uint8 chan_num); +void CF_CmdGetSetParam(uint8 is_set, CF_GetSet_ValueID_t param_id, uint32 value, uint8 chan_num); void CF_CmdSetParam(CFE_SB_Buffer_t *msg); void CF_CmdGetParam(CFE_SB_Buffer_t *msg); void CF_CmdEnableEngine(CFE_SB_Buffer_t *msg); diff --git a/fsw/src/cf_msg.h b/fsw/src/cf_msg.h index 4026e616b..5f9646deb 100644 --- a/fsw/src/cf_msg.h +++ b/fsw/src/cf_msg.h @@ -117,8 +117,7 @@ typedef struct CF_ConfigPacket uint8 nak_limit; /* number of times to retry NAK before giving up (resets on a single response */ CF_EntityId_t local_eid; -/* must #define the number of data items in this struct for command processing */ -#define CF_NUM_CFG_PACKET_ITEMS 10 + } CF_ConfigPacket_t; /**************************************** @@ -183,6 +182,27 @@ typedef struct CF_UnionArgs_Payload_t data; } CF_UnionArgsCmd_t; +/** + * @brief Parameter IDs for use with Get/Set param messages + * + * Specifically these are used for the "key" field within CF_GetParamCmd_t and + * CF_SetParamCmd_t message structures. + */ +typedef enum +{ + CF_GetSet_ValueID_ticks_per_second, + CF_GetSet_ValueID_rx_crc_calc_bytes_per_wakeup, + CF_GetSet_ValueID_ack_timer_s, + CF_GetSet_ValueID_nak_timer_s, + CF_GetSet_ValueID_inactivity_timer_s, + CF_GetSet_ValueID_outgoing_file_chunk_size, + CF_GetSet_ValueID_ack_limit, + CF_GetSet_ValueID_nak_limit, + CF_GetSet_ValueID_local_eid, + CF_GetSet_ValueID_chan_max_outgoing_messages_per_wakeup, + CF_GetSet_ValueID_MAX +} CF_GetSet_ValueID_t; + typedef struct CF_GetParamCmd { CFE_MSG_CommandHeader_t cmd_header; @@ -230,4 +250,5 @@ typedef struct CF_TransactionCmd uint8 chan; /* if 254, use ts. if 255, all channels */ } CF_TransactionCmd_t; + #endif /* !CF_MSG_H */ diff --git a/unit-test/cf_cmd_tests.c b/unit-test/cf_cmd_tests.c index a9ef2fe4a..23c346680 100644 --- a/unit-test/cf_cmd_tests.c +++ b/unit-test/cf_cmd_tests.c @@ -4157,12 +4157,12 @@ void Test_CF_CmdValidateMaxOutgoing_WhenGiven_val_Is_0_And_sem_name_Is_NULL_Retu ** *******************************************************************************/ -void Test_CF_CmdGetSetParam_When_param_id_Eq_CF_NUM_CFG_PACKET_ITEMS_FailSendEventAndRejectCmd(void) +void Test_CF_CmdGetSetParam_When_param_id_Eq_CF_GetSet_ValueID_MAX_FailSendEventAndRejectCmd(void) { /* Arrange */ CF_ConfigTable_t dummy_config_table; uint8 arg_is_set = Any_uint8(); - uint8 arg_param_id = CF_NUM_CFG_PACKET_ITEMS; + uint8 arg_param_id = CF_GetSet_ValueID_MAX; uint32 arg_value = Any_uint32(); uint8 arg_chan_num = Any_uint8(); @@ -4186,14 +4186,14 @@ void Test_CF_CmdGetSetParam_When_param_id_Eq_CF_NUM_CFG_PACKET_ITEMS_FailSendEve "CF_AppData.hk.counters.err is %d and should be 1 more than %d", CF_AppData.hk.counters.err, initial_hk_err_counter); -} /* end Test_CF_CmdGetSetParam_When_param_id_Eq_CF_NUM_CFG_PACKET_ITEMS_FailSendEventAndRejectCmd */ +} /* end Test_CF_CmdGetSetParam_When_param_id_Eq_CF_GetSet_ValueID_MAX_FailSendEventAndRejectCmd */ -void Test_CF_CmdGetSetParam_When_param_id_AnyGreaterThan_CF_NUM_CFG_PACKET_ITEMS_FailSendEventAndRejectCmd(void) +void Test_CF_CmdGetSetParam_When_param_id_AnyGreaterThan_CF_GetSet_ValueID_MAX_FailSendEventAndRejectCmd(void) { /* Arrange */ CF_ConfigTable_t dummy_config_table; uint8 arg_is_set = Any_uint8(); - uint8 arg_param_id = Any_uint8_GreaterThan(CF_NUM_CFG_PACKET_ITEMS); + uint8 arg_param_id = Any_uint8_GreaterThan(CF_GetSet_ValueID_MAX); uint32 arg_value = Any_uint32(); uint8 arg_chan_num = Any_uint8(); @@ -4217,14 +4217,14 @@ void Test_CF_CmdGetSetParam_When_param_id_AnyGreaterThan_CF_NUM_CFG_PACKET_ITEMS "CF_AppData.hk.counters.err is %u and should be 1 more than %u", CF_AppData.hk.counters.err, initial_hk_err_counter); -} /* end Test_CF_CmdGetSetParam_When_param_id_AnyGreaterThan_CF_NUM_CFG_PACKET_ITEMS_FailSendEventAndRejectCmd */ +} /* end Test_CF_CmdGetSetParam_When_param_id_AnyGreaterThan_CF_GetSet_ValueID_MAX_FailSendEventAndRejectCmd */ void Test_CF_CmdGetSetParam_Given_chan_num_IsEqTo_CF_NUM_CHANNELS_ErrorOutAndCountError(void) { /* Arrange */ CF_ConfigTable_t dummy_config_table; uint8 arg_is_set = Any_uint8(); - uint8 arg_param_id = Any_uint8_LessThan(CF_NUM_CFG_PACKET_ITEMS); + uint8 arg_param_id = Any_uint8_LessThan(CF_GetSet_ValueID_MAX); uint32 arg_value = Any_uint32(); uint8 arg_chan_num = CF_NUM_CHANNELS; @@ -4255,7 +4255,7 @@ void Test_CF_CmdGetSetParam_Given_chan_num_IsGreaterThan_CF_NUM_CHANNELS_ErrorOu /* Arrange */ CF_ConfigTable_t dummy_config_table; uint8 arg_is_set = Any_uint8(); - uint8 arg_param_id = Any_uint8_LessThan(CF_NUM_CFG_PACKET_ITEMS); + uint8 arg_param_id = Any_uint8_LessThan(CF_GetSet_ValueID_MAX); uint32 arg_value = Any_uint32(); uint8 arg_chan_num = Any_uint8_GreaterThan(CF_NUM_CHANNELS); @@ -5344,12 +5344,12 @@ void add_CF_CmdValidateMaxOutgoing_tests(void) void add_CF_CmdGetSetParam_tests(void) { - UtTest_Add(Test_CF_CmdGetSetParam_When_param_id_Eq_CF_NUM_CFG_PACKET_ITEMS_FailSendEventAndRejectCmd, + UtTest_Add(Test_CF_CmdGetSetParam_When_param_id_Eq_CF_GetSet_ValueID_MAX_FailSendEventAndRejectCmd, cf_cmd_tests_Setup, cf_cmd_tests_Teardown, - "Test_CF_CmdGetSetParam_When_param_id_Eq_CF_NUM_CFG_PACKET_ITEMS_FailSendEventAndRejectCmd"); - UtTest_Add(Test_CF_CmdGetSetParam_When_param_id_AnyGreaterThan_CF_NUM_CFG_PACKET_ITEMS_FailSendEventAndRejectCmd, + "Test_CF_CmdGetSetParam_When_param_id_Eq_CF_GetSet_ValueID_MAX_FailSendEventAndRejectCmd"); + UtTest_Add(Test_CF_CmdGetSetParam_When_param_id_AnyGreaterThan_CF_GetSet_ValueID_MAX_FailSendEventAndRejectCmd, cf_cmd_tests_Setup, cf_cmd_tests_Teardown, - "Test_CF_CmdGetSetParam_When_param_id_AnyGreaterThan_CF_NUM_CFG_PACKET_ITEMS_FailSendEventAndRejectCmd"); + "Test_CF_CmdGetSetParam_When_param_id_AnyGreaterThan_CF_GetSet_ValueID_MAX_FailSendEventAndRejectCmd"); UtTest_Add(Test_CF_CmdGetSetParam_Given_chan_num_IsEqTo_CF_NUM_CHANNELS_ErrorOutAndCountError, cf_cmd_tests_Setup, cf_cmd_tests_Teardown, "Test_CF_CmdGetSetParam_Given_chan_num_IsEqTo_CF_NUM_CHANNELS_ErrorOutAndCountError"); diff --git a/unit-test/stubs/cf_cmd_stubs.c b/unit-test/stubs/cf_cmd_stubs.c index e3721df36..521778378 100644 --- a/unit-test/stubs/cf_cmd_stubs.c +++ b/unit-test/stubs/cf_cmd_stubs.c @@ -181,10 +181,10 @@ void CF_CmdGetParam(CFE_SB_Buffer_t *msg) * Generated stub function for CF_CmdGetSetParam() * ---------------------------------------------------- */ -void CF_CmdGetSetParam(uint8 is_set, uint8 param_id, uint32 value, uint8 chan_num) +void CF_CmdGetSetParam(uint8 is_set, CF_GetSet_ValueID_t param_id, uint32 value, uint8 chan_num) { UT_GenStub_AddParam(CF_CmdGetSetParam, uint8, is_set); - UT_GenStub_AddParam(CF_CmdGetSetParam, uint8, param_id); + UT_GenStub_AddParam(CF_CmdGetSetParam, CF_GetSet_ValueID_t, param_id); UT_GenStub_AddParam(CF_CmdGetSetParam, uint32, value); UT_GenStub_AddParam(CF_CmdGetSetParam, uint8, chan_num);