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.

Sign up for GitHub

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

Fix reception header reading mistakes in case of buffer loop, fix #234 #426

Merged
merged 1 commit into from
Mar 28, 2023
Merged

Fix reception header reading mistakes in case of buffer loop, fix #234 #426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 9 additions & 40 deletions network/robus/src/msg_alloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,6 @@ volatile msg_t *oldest_msg = NULL; /*!< The oldest message among all
volatile msg_t *used_msg = NULL; /*!< Message curently used by luos loop. */
volatile uint8_t mem_clear_needed; /*!< A flag allowing to spot some msg space cleaning operations to do. */

// Allocator task stack
volatile header_t *copy_task_pointer = NULL; /*!< This pointer is used to perform a header copy from the end of the msg_buffer to the begin of the msg_buffer. If this pointer if different than NULL there is a copy to make. */

// msg interpretation task stack
volatile msg_t *msg_tasks[MAX_MSG_NB]; /*!< ready message table. */
volatile uint16_t msg_tasks_stack_id; /*!< Next writen msg_tasks id. */
Expand Down Expand Up @@ -160,10 +157,9 @@ void MsgAlloc_Init(memory_stats_t *memory_stats)
memset((void *)luos_tasks, 0, sizeof(luos_tasks));
tx_tasks_stack_id = 0;
memset((void *)tx_tasks, 0, sizeof(tx_tasks));
copy_task_pointer = NULL;
used_msg = NULL;
oldest_msg = (msg_t *)INT_MAX;
mem_clear_needed = false;
used_msg = NULL;
oldest_msg = (msg_t *)INT_MAX;
mem_clear_needed = false;
if (memory_stats != NULL)
{
mem_stat = memory_stats;
Expand Down Expand Up @@ -214,29 +210,6 @@ void MsgAlloc_loop(void)
******************************************************************************/
_CRITICAL void MsgAlloc_ValidDataIntegrity(void)
{
// Check if we have to make a header copy from the end to the begin of msg_buffer.
if (copy_task_pointer != NULL)
{
// copy_task_pointer point to a header to copy at the begin of msg_buffer
// Copy the header at the begining of msg_buffer
//
// msg_buffer init state
// +--------------------------------------------------------+
// |-------------------------------| Header | Datas to be received |
// +------------------------------------------^-------------+ ^
// | |
// Need to copy to buffer beginning data_end_estimation
// as an overflows will occur
//
// msg_buffer ending state
// +--------------------------------------------------------+
// |Header|-------------------------------------------------|
// +--------------------------------------------------------+
//
memcpy((void *)&msg_buffer[0], (void *)copy_task_pointer, sizeof(header_t));
// reset copy_task_pointer status
copy_task_pointer = NULL;
}
// Do we have to check data dropping?
LuosHAL_SetIrqState(false);
if (mem_clear_needed == true)
Expand Down Expand Up @@ -440,10 +413,6 @@ _CRITICAL void MsgAlloc_InvalidMsg(void)
data_ptr = (uint8_t *)current_msg;
data_end_estimation = (uint8_t *)(&current_msg->stream[sizeof(header_t) + CRC_SIZE]);
LUOS_ASSERT((uintptr_t)data_end_estimation < (uintptr_t)&msg_buffer[MSG_BUFFER_SIZE]);
if (current_msg == (volatile msg_t *)&msg_buffer[0])
{
copy_task_pointer = NULL;
}
}
/******************************************************************************
* @brief Valid the current message header by preparing the allocator to get the message data
Expand Down Expand Up @@ -472,13 +441,13 @@ _CRITICAL void MsgAlloc_ValidHeader(uint8_t valid, uint16_t data_size)
//
// msg_buffer ending state :
// +-------------------------------------------------------------+
// |------------------------------------| Header | Datas to be received |
// +----------^-------------------------^------------------------+
// | | |
// current_msg data_ptr copy_task_pointer
// | Header | Datas to be received |----------------------------
// +----------^--------------------------------------------------+
// | |
// current_msg data_ptr
//
// Create a task to copy the header at the begining of msg_buffer
copy_task_pointer = (header_t *)&current_msg->header;
// Copy the header at the begining of msg_buffer
memcpy((void *)&msg_buffer[0], (void *)&current_msg->header, sizeof(header_t));
// Move current_msg to msg_buffer
current_msg = (volatile msg_t *)&msg_buffer[0];
// move data_ptr after the new location of the header
Expand Down
31 changes: 11 additions & 20 deletions test/test_msg_alloc/unit_test_mem_alloc_generic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ extern volatile uint8_t *data_end_estimation;
extern volatile msg_t *oldest_msg;
extern volatile msg_t *used_msg;
extern volatile uint8_t mem_clear_needed;
extern volatile header_t *copy_task_pointer;
extern volatile msg_t *msg_tasks[MAX_MSG_NB];
extern volatile uint16_t msg_tasks_stack_id;
extern volatile luos_task_t luos_tasks[MAX_MSG_NB];
Expand Down Expand Up @@ -244,18 +243,17 @@ void unittest_MsgAlloc_ValidHeader()
//
// msg_buffer ending state :
// +-------------------------------------------------------------+
// |------------------------------------| Header | Datas to be received |
// +----------^-------------------------^------------------------+
// | | |
// current_msg data_ptr copy_task_pointer
// || Header | Datas to be received |--------------------------
// +----------^--------------------------------------------------+
// | |
// current_msg data_ptr
//

uint8_t valid = true;
uint16_t data_size = DATA_SIZE;
current_msg = (msg_t *)&msg_buffer[MSG_BUFFER_SIZE - 1];
header_t *expected_copy_task_pointer = (header_t *)current_msg;
uint8_t *expected_data_ptr = (uint8_t *)&msg_buffer[0] + sizeof(header_t);
uint8_t *expected_data_end = expected_data_ptr + data_size + CRC_SIZE;
uint8_t valid = true;
uint16_t data_size = DATA_SIZE;
current_msg = (msg_t *)&msg_buffer[MSG_BUFFER_SIZE - 1];
uint8_t *expected_data_ptr = (uint8_t *)&msg_buffer[0] + sizeof(header_t);
uint8_t *expected_data_end = expected_data_ptr + data_size + CRC_SIZE;

MsgAlloc_ValidHeader(valid, data_size);
NEW_STEP("Check mem cleared flag is raised");
Expand All @@ -264,8 +262,6 @@ void unittest_MsgAlloc_ValidHeader()
TEST_ASSERT_EQUAL(expected_data_ptr, data_ptr);
NEW_STEP("Check \"data end estimation\" has been computed");
TEST_ASSERT_EQUAL(expected_data_end, data_end_estimation);
NEW_STEP("Check \"copy task pointer\" points to message to copy");
TEST_ASSERT_EQUAL(expected_copy_task_pointer, copy_task_pointer);
}

NEW_TEST_CASE("There is enough space : save the end position and raise the clear flag");
Expand Down Expand Up @@ -405,14 +401,11 @@ void unittest_MsgAlloc_InvalidMsg()
TEST_ASSERT_EQUAL(current_msg, data_ptr);
NEW_STEP("Check \"data end estimation\" is correctly computed");
TEST_ASSERT_EQUAL(data_end_estimation, ((uint8_t *)current_msg + DATA_END));
NEW_STEP("Check no copy is needed : \"copy task pointer\" is reseted");
TEST_ASSERT_NULL(copy_task_pointer);

// Init variables
//---------------
current_msg = (msg_t *)&msg_buffer[100];
data_ptr = &msg_buffer[110];
copy_task_pointer = (header_t *)data_ptr;
current_msg = (msg_t *)&msg_buffer[100];
data_ptr = &msg_buffer[110];

// Call function
//---------------
Expand All @@ -424,8 +417,6 @@ void unittest_MsgAlloc_InvalidMsg()
TEST_ASSERT_EQUAL(current_msg, data_ptr);
NEW_STEP("Check \"data end estimation\" is correctly computed");
TEST_ASSERT_EQUAL(data_end_estimation, ((uint8_t *)current_msg + DATA_END));
NEW_STEP("Check \"copy task pointer\" is not cleared");
TEST_ASSERT_NOT_NULL(copy_task_pointer);
}

NEW_TEST_CASE("Clean memory");
Expand Down
48 changes: 2 additions & 46 deletions test/test_msg_alloc/unit_test_mem_alloc_static.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,12 +514,9 @@ void unittest_OldestMsgCandidate(void)

void unittest_ValidDataIntegrity(void)
{
NEW_TEST_CASE("No copy needed");
NEW_TEST_CASE("Check data integrity");
MsgAlloc_Init(NULL);
{
// copy_task_pointer is NULL :
// So there is no need to copy header to begin of msg_buffer
//
// msg_buffer init state
// +-------------------------------------------------------------+
// |-------|Header|----------------------------------------------|
Expand All @@ -532,8 +529,7 @@ void unittest_ValidDataIntegrity(void)
//

uint8_t expected_msg_buffer[MSG_BUFFER_SIZE];
mem_clear_needed = 0;
copy_task_pointer = NULL;
mem_clear_needed = 0;

memset((void *)&msg_buffer[0], 0xAA, MSG_BUFFER_SIZE);
memset((void *)&expected_msg_buffer[0], 0xAA, MSG_BUFFER_SIZE);
Expand All @@ -546,46 +542,6 @@ void unittest_ValidDataIntegrity(void)
TEST_ASSERT_EQUAL_MEMORY(expected_msg_buffer, msg_buffer, MSG_BUFFER_SIZE);
}

NEW_TEST_CASE("Copy header to begin of message buffer");
MsgAlloc_Init(NULL);
{
// Tx message size is greater than Rx size currently received.
// Tx message doesn't fit in msg buffer.
// There is already a Task , function should return FAILED
//
// msg_buffer init state
// +-------------------------------------------------------------+
// |-------|Header|----------------------------------------------|
// +-------------------------------------------------------------+
//
//
// msg_buffer ending state
// +-------------------------------------------------------------+
// |Header|------------------------------------------------------|
// +-------------------------------------------------------------+
//

uint8_t expected_msg_buffer[MSG_BUFFER_SIZE];
mem_clear_needed = 0;
copy_task_pointer = (header_t *)&msg_buffer[sizeof(msg_t)];

memset((void *)&msg_buffer[0], 0, MSG_BUFFER_SIZE);
memset((void *)&expected_msg_buffer[0], 0, MSG_BUFFER_SIZE);
memset((void *)&msg_buffer[sizeof(msg_t)], 1, sizeof(header_t));
memset((void *)&expected_msg_buffer[0], 1, sizeof(header_t));

RESET_ASSERT();
MsgAlloc_ValidDataIntegrity();

NEW_STEP("Check NO assert has occured");
TEST_ASSERT_FALSE(IS_ASSERT());
NEW_STEP("Check \"copy task pointer\" is NULL");
TEST_ASSERT_NULL(copy_task_pointer);
// TEST_ASSERT_EQUAL(copy_task_pointer, NULL);
NEW_STEP("Check header is copied to beginning of buffer");
TEST_ASSERT_EQUAL_MEMORY(expected_msg_buffer, msg_buffer, sizeof(header_t));
}

NEW_TEST_CASE("Verify memory cleaning");
MsgAlloc_Init(NULL);
{
Expand Down