1
\$\begingroup\$

I am working with a fairly large data structure that needs to be maintained in RAM in two forms: a standard version and an inverted version. Whenever an element is updated, it must be written to the standard memory and bitwise inverted into the second memory. Each read operation must verify both memories to ensure that one value is the bitwise inverse of the other.

The straightforward approach to achieve this involves implementing getter and setter functions for each field. However, to make the implementation more generic, I utilized macros and the offsetof macro from stddef.h.

I would appreciate a review to ensure the sanity of this code.

#include <stdint.h>
#include <stdbool.h>
#include <stddef.h>

typedef struct __attribute__ ((aligned (4))) double_ram_mem_s
{
    uint8_t test_val_u8;
    struct inner_struct_s
    {
        uint8_t test_val_s_u8;
    } inner_struct;
} double_ram_mem_t;

double_ram_mem_t double_ram_mem;
double_ram_mem_t double_ram_memi;

bool double_ram_read_u8(uint16_t offset8, uint8_t *value)
{
    if (offset8 >= sizeof(double_ram_mem_t))
        return false;

    uint8_t mem_val = ((uint8_t *)&double_ram_mem)[offset8];
    uint8_t memi_val_inv = ~((uint8_t *)&double_ram_memi)[offset8];

    if (mem_val != memi_val_inv)
        return false;

    *value = mem_val;
    return true;
}

bool double_ram_write_u8(uint16_t offset8, uint8_t value)
{
    if (offset8 >= sizeof(double_ram_mem_t))
        return false;

    ((uint8_t *)&double_ram_mem)[offset8] = value;
    ((uint8_t *)&double_ram_memi)[offset8] = ~value;

    return true;
}

#define DOUBLE_RAM_READ_U8(m_ident, p_value)  \
            double_ram_read_u8((offsetof(double_ram_mem_t, m_ident)), (p_value))
#define DOUBLE_RAM_WRITE_U8(m_ident, value)   \
            double_ram_write_u8((offsetof(double_ram_mem_t, m_ident)), (value))

Example how this code could be used:

bool safety_mem_example(void)
{
    // init both normal and inversed memories
    memset(&double_ram_mem, 0x00, sizeof(double_ram_mem_t));
    memset(&double_ram_memi, 0xFF, sizeof(double_ram_mem_t));

    // write example
    uint8_t write_val = 0xC3;
    bool success = DOUBLE_RAM_WRITE_U8(inner_struct.test_val_s_u8, 0xC3);
    if (!success)
        return success;

    // read example
    uint8_t read_val;
    success = DOUBLE_RAM_READ_U8(inner_struct.test_val_s_u8, &read_val);
    if (!success)
        return success;

    if (read_val != write_val)
        return false;

    return true;
}

I'm open for completely different approaches and suggestions.


Edit: answering some of the questions in the comments

Not sure what OS you are working on, but on Ubuntu 22.04 the gcc version of stddef.h does not contain the macro to define offsetof. This would mean that the code isn't portable.

Portability isn't a real concern in this case. I'm compiling this code for a relatively small ARM Cortex-M4 controller by Texas Instruments. I'm using stddef.h provided by TI for the microcontroller I'm using.

It isn't clear from the code provide if the first block of code is in a header file or a C source file.

It's not the real code; just a minimalistic sample to demonstrate the concept of what I'm doing.

Suggestion: Objective makes little sense. If detecting data corruption on the level of struct members is required, you need better storage. What will the program do if/when it detects a mis-matching byte? What can it do but give up? Research "CRCs" and data error detection/correction methods. Plan before writing code...

This code runs on a motor driver. The RAM validation is just a minor part of the whole. Whenever the system detects data mis-match, it would issue a Safe Torque Off (STO), which puts the motor into a safe state.

I've also done some tests using CRC (CRC-CCITT with polynomial 0x1021), but that took far too long to compute (27.2 μs) for my purposes.

\$\endgroup\$
4
  • 2
    \$\begingroup\$ Not sure what OS you are working on, but on Ubuntu 22.04 the gcc version of stddef.h does not contain the macro to define offsetof. This would mean that the code isn't portable. \$\endgroup\$ Commented Nov 29, 2024 at 15:35
  • 1
    \$\begingroup\$ It isn't clear from the code provide if the first block of code is in a header file or a C source file. If it is in a header file than if it is include in multiple C source files the code will run into linking errors because the functions will be defined in multiple files. If it is in a C source file that limits the usefulness of the struct and the macros. \$\endgroup\$ Commented Nov 29, 2024 at 15:46
  • 2
    \$\begingroup\$ Suggestion: Objective makes little sense. If detecting data corruption on the level of struct members is required, you need better storage. What will the program do if/when it detects a mis-matching byte? What can it do but give up? Research "CRCs" and data error detection/correction methods. Plan before writing code... \$\endgroup\$ Commented Nov 29, 2024 at 22:08
  • 1
    \$\begingroup\$ If this isn't the real code, the question is off-topic and should be closed. We only review the real code on this site. Please read How do I ask a good question? \$\endgroup\$ Commented Nov 30, 2024 at 2:46

1 Answer 1

1
\$\begingroup\$

Code needs an example to inject failed double_ram_mem, double_ram_memi pairing.

Consider coding multiple-byte read and write functions

Maybe also a complete record check.

Consider forming some_name.h and some_name.c files to clearly express and segregate the interface and implementation of your idea.


Minor stuff:

Unneeded alignment

Why typedef struct __attribute__ ((aligned (4))) double_ram_mem_s and not simply typedef struct double_ram_mem_s.

Avoid adding in extraneous requirements unless they fulfill a specified programming need.

Why uint16_t in uint16_t offset8?

offsetof returns a size_t. IMO, avoid unneeded type changes. Use size_t offset8 and avoid quiet size truncations.

Unneeded type

I see no need for uint16_t anywhere in this code.

Be direct: use size of object

memset(&double_ram_mem, 0x00, sizeof(double_ram_mem_t)); obliges a reviewer to validate &double_ram_mem is a double_ram_mem_t. Elimante that need, future maintenance and simply use the size of the object.

memset(&double_ram_mem, 0x00, sizeof double_ram_mem);

Why 0xC3 twice?

Magic numbers are best coded only once.

uint8_t write_val = 0xC3;
// bool success = DOUBLE_RAM_WRITE_U8(inner_struct.test_val_s_u8, 0xC3);
bool success = DOUBLE_RAM_WRITE_U8(inner_struct.test_val_s_u8, write_val);

Subtle: Extract the value without manipulation

Should the compare fail, it is useful to report the original 2 values.

Rather than:

uint8_t memi_val_inv = ~((uint8_t *)&double_ram_memi)[offset8];
if (mem_val != memi_val_inv)

Consider:

uint8_t memi_val_inv = ((uint8_t *)&double_ram_memi)[offset8];
if (mem_val ^ memi_val_inv != 0xFF)

Also instead of 0xFF, consider a macro of ((uint8_t)(~0u))

\$\endgroup\$
0