Thread: Could you review my code?

Hi guys,

I want to read an IMU and some pressure sensors and store them together with a counter into a buffer. This is a subtask of my main firmware development. I wrote a working code chunk, but since I want to improve my coding skills, I would like your comments to improve, optimize, or even write the following code in a more elegant way. All suggestions are welcomed.

This is the main.c

Code:

//shift + alt + f : Auto formating
#include <stdio.h>
#include "main.h"


struct s_sampling sampling = {.force_sensor = true, .pressure_sensors = 8, .sensors_counter = 8, .packet_counter = 2000000, .index = 0};


int main()
{
    saadc_sample(&sampling);
    return 0;
}


void saadc_sample(struct s_sampling *sampling)
{
    sampling->packet_counter;
    sampling->payload[14] = sampling->packet_counter >> 16;
    sampling->payload[15] = sampling->packet_counter;


    //Read the IMU
    IMU_read(sampling);


    // Read the flexi force sensors
    while (sampling->force_sensor)
    {
        sampling->sensors_counter--;


        read_flexiforce(sampling);


        if (!sampling->sensors_counter)
        {
            break;
        }
    }


    for (uint8_t i = 0; i < 16; i++)
    {
        printf("Payload[%i] = %d\n", i, sampling->payload[i]);
    }
}


void IMU_read(struct s_sampling *sampling)
{
    //bmi160_get_sensor_data((BMI160_ACCEL_SEL | BMI160_GYRO_SEL | BMI160_TIME_SEL), &s_accel, &s_gyro, &sensor); //This funtion is not implemented here
    sampling->payload[0] = 1001; // Dummy value for now. It will be IMU s_accel.x 
    sampling->payload[1] = 1002; // Dummy value for now. It will be IMU s_accel.y 
    sampling->payload[2] = 1003; // Dummy value for now. It will be IMU s_accel.z 
    sampling->payload[3] = 1004; // Dummy value for now. It will be IMU s_gyro.x 
    sampling->payload[4] = 1005; // Dummy value for now. It will be IMU s_gyro.y 
    sampling->payload[5] = 1006; // Dummy value for now. It will be IMU s_gyro.z 
}


void read_flexiforce(struct s_sampling *sampling)
{
    static uint16_t dummy_value = 1007; // / Dummy value. It will be pressure sensror reading


    sampling->payload[sampling->index + 6] = dummy_value++;


    if (sampling->index == sampling->pressure_sensors - 1)
    {
        sampling->index = 0;
    }
    else
    {
        sampling->index++;
    }
}

This is the main.h

Code:

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


struct s_sampling
{
    int16_t payload[16];     // 32bytes buffer to store the payload
    uint8_t index;
    int32_t packet_counter;
    bool force_sensor;
    uint8_t sensors_counter;
    uint8_t pressure_sensors; // number of pressure sensors


};


void saadc_sample(struct s_sampling *sampling);
void IMU_read(struct s_sampling *sampling);
void read_flexiforce(struct s_sampling *sampling);

Originally Posted by stahta01

Avoid magic numbers/constants in programming.

Magic number (programming) - Wikipedia

Tim S.

Thank you Tim!! That was just for the example code chunk I prepared..

Thank you Salem!!

Originally Posted by Salem

>


Perhaps your payload would be better as

Code:

struct s_sampling
{
    struct {
        int16_t    imu[6];
        int16_t    pressure[8];
        int16_t    counter[2];
    } payload;
    uint8_t index;  // maybe rename to pressure_index
    int32_t packet_counter;
    bool force_sensor;
    uint8_t sensors_counter;
    uint8_t num_sensors;
};

When the payload is ready it is assigned to a pointer. This pointer belongs to an SDK provided my the manufacturer of the Bluetooth IC I use.

Code:

ble_gatts_hvx_params_t hvx_params;

 hvx_params.p_data = sampling->payload;

Code:

typedef struct
{
  uint16_t          handle;             /**< Characteristic Value Handle. */
  uint8_t           type;               /**< Indication or Notification, see @ref BLE_GATT_HVX_TYPES. */
  uint16_t          offset;             /**< Offset within the attribute value. */
  uint16_t         *p_len;              /**< Length in bytes to be written, length in bytes written after return. */
  uint8_t const    *p_data;             /**< Actual data content, use NULL to use the current attribute value. */
} ble_gatts_hvx_params_t;

So, if I do the payload as you suggested, how could I assign the payload to the p_data pointer?

Originally Posted by Salem

> But aside from that, you could do this.
hvx_params.p_data = sampling->payload.imu;

I must assign the whole payload to p_data, otherwise just the imu will be transmitted (bluetooth application)

Originally Posted by Salem

> hvx_params.p_data = sampling->payload;
Well in your code, this would be an error to begin with.
p_data is a uint8_t*, but payload is an array of int16_t

Should I typecast the payload?

Code:

hvx_params.p_data = (uint8_t*)sampling->payload // don't know if the syntax is valid

In function: saadc_sample(struct s_sampling *sampling)

Code:

sampling->packet_counter;

Remove this line, or change the code to support its use. It appears to do nothing, and I do not see any use of the volatile keyword that would indicate that it is secretly a memory-mapped I/O port or some other device where accessing the data creates a side-effect.

Code:

sampling->payload[14] = sampling->packet_counter >> 16;
sampling->payload[15] = sampling->packet_counter;

This right here convinces me that your structure definition is bad. Why are you moving data from one place to another inside the same structure? Use a union!

Code:

// Read the IMU
IMU_read(sampling);

With that function name, the comment is useless.

The function name is bad, however. Why is this IMU_read and not imu_read. Or perhaps saadc_read_imu. Or maybe static void read_imu()? Sadly, you declare all your functions in main.h, which suggests that they are all part of the public interface. Why would you do this? Your module should have a very small API, and the rest of the functions should be static by default.

Code:

// Read the flexi force sensors
while (sampling->force_sensor) {
    sampling->sensors_counter--;

Another bad comment. Just get rid of it. Or possibly replace the entire block of code with a function. (In general, if you find yourself writing a comment to "explain what step you are on" -- which is totally legit -- then turn the comment into a function call.)

This loop starts with no initialization, but runs a fixed number of times depending on the contents of the sampling->sensors_counter variable. That's a for loop in disguise!

Also, why are you using a structure member as a loop counter? Your code does not appear to be multi-tasking, or multi-threading. There is no "separate logic" that needs to see this counter value. Just use a local variable instead:

Code:

for (int i = sampling->num_sensors; i != 0; --i) {

(And, obviously, make the structure member changes I recommend elsewhere...)

Code:

for (uint8_t i = 0; i < 16; i++) {
    printf("Payload[%i] = %d\n", i, sampling->payload[i]);
}

This code is just debug helper stuff. Make your debug helpers formal! Don't just write stupid values out. If you find yourself doing this, you know you'll do it again. Write a formal function that does all the things and maybe put it in an #if !RELEASE block or something.

Code:

#if !RELEASE

// This needs to be at top of file.
void _saadc_dump_sensors(struct saadc_samples * sp);

#   define DUMP_SENSORS(sp)    _saadc_dump_sensors(sp)

void 
_saadc_dump_sensors(
    struct saadc_samples * sp)
{
    DPRINTF("...", ...); // Of course you have a DPRINTF macro, right? 
}

#else
#    define DUMP_SENSORS(sp)    EMPTY
#endif

In function: IMU_read(struct s_sampling *sampling)

First, the name. If this is an "IMU function" then name it imu_read. But if it is a saadc-public-API function, name it saadc_read_imu. And if it is a private function of whatever module, it doesn't need a prefix at all, so rename it static void read_imu or some such.

By default, all of your functions should be static. Only when you are forced to do so should you give a function external linkage. And you should push back on it! Try really hard to not make functions public. Once something is public, you will think of another use for it, and then you have to write test cases and stuff. Lots of work. Easier to just keep the functions private. (If someone needs a task done, provide a function that just does the task with few parameters.)

Code:

// bmi160_get_sensor_data((BMI160_ACCEL_SEL | BMI160_GYRO_SEL | BMI160_TIME_SEL), &s_accel, &s_gyro, &sensor); //This funtion is not implemented here
    sampling->payload[0] = 1001;  // Dummy value for now. It will be IMU s_accel.x
    sampling->payload[1] = 1002;  // Dummy value for now. It will be IMU s_accel.y

It appears that you are going to create some kind of temporary variables (s_accel, s_gyro, sensor) and then fetch sensor data into them. Then you plan to copy the temporary variables into the payload array. That is totally bogus. Never copy data if you can redirect the arrival of the data. Instead, use structs or unions or arrays or pointer casting to organize your payload data in a way that is compatible with the incoming sensor data.

Code:

    sampling->payload[0] = 1001;  // Dummy value for now. It will be IMU s_accel.x
    sampling->payload[1] = 1002;  // Dummy value for now. It will be IMU s_accel.y
    sampling->payload[2] = 1003;  // Dummy value for now. It will be IMU s_accel.z

Instead of this, make that:

Code:

    sp->accel_x = ...
    sp->accel_y = ...
    sp->accel_z = ...

Unless you are writing "generic networking" code, there's no reason not to provide a useful name to store/fetch your values with. If you want to loop over the parts, you can wrap everything in a union.

In function: read_flexiforce(struct s_sampling *sampling)

This is a good name. Now you should make the function static, if possible. This is weird code because you are obviously taking some steps to not provide "boring" constant fake data. It's also weird because I sort of expected it to modify the force_sensor field. (Because of the implied relationship in the saadc_sample function.)

I suggest moving your loop/reset code to the top of the function. It will enable you to get rid of the - 1 in the comparison, which should make things clearer, at the expense of dividing your update:

Code:

if (sampling->index == sampling->num_sensors)
     sampling->index = 0;

sampling->ff_pressure[sampling->index++] = dummy_value++;

However, when reading this in conjunction with the saadc_sample function, it's clear you are doing a lot of work to avoid just passing in a parameter. Don't do that -- just pass in the parameter:

Code:

// In saadc_sample:

for (int i = 0; i < sampling->num_sensors; ++i) {
     read_flexiforce(sampling, i);
}


// In read_flexiforce:

sampling->ff_pressure[i] = dummy_value++;

If you pass in the parameter, you don't have to have the awkward code that is duplicating the effect of the outer loop index.


In main.h:

Reorganize your struct:

Code:

struct s_sampling {
     int16_t payload[16];  // 32bytes buffer to store the payload
     uint8_t index;
     int32_t packet_counter;
     bool    force_sensor;
     uint8_t sensors_counter;
     uint8_t pressure_sensors;  // number of pressure sensors
};

You have objects of mixed sizes next to each other. Most compilers will "pad" items so that the offset of the item is a multiple of the size of the item -- so that 16-bit items are offset on a 2-byte boundary, and 32-bit items are offset on a 4-byte boundary. As such, it behooves you to group things by size. Here is FMTYEWTK about The Lost Art of Structure Packing.

Code:

struct s_sampling {
     int32_t packet_counter;
     uint8_t index;
     uint8_t sensors_counter;
     uint8_t pressure_sensors;
     bool    force_sensor;          // 1 byte, same as uint8_t
     int16_t payload[16];        // Big, but no padding after
};

Remove items from your struct that don't need to be there! Based on the code you have presented:

• the index member just mimics the loop index in saadc_sample()
• the pressure_sensors might be a constant value
• the sensors_counter is the loop index in saadc_sample() (which moots the whole index member anyway) and should be local
• the force_sensor is always true, but might be independent
• the packet_counter might be a constant, or might be a module-static variable

Which makes it seem like every initialized field in your structure is possibly unnecessary. Awkward!

Alternatively, maybe this part of the structure is really a "global variables" structure, and you've got two structs - the "config data" and the "readings"?

Give names to the elements in payload. Instead of using comments, instead of using an enum of array indices, just name the elements. If you want to be able to iterate over them in byte or word form, use a union:

Code:

enum { NUM_FLEXIFORCE_SENSORS=8 };

// Note: I am using an "anonymous" struct member here. This not C89.
    
typedef 
struct {
    int16_t is_valid;

    union {
        uint8_t as_bytes[1];       // Does not include `is_valid`
         uint16_t as_words[1];     // Does not include `is_valid`

         struct {
             struct {
                  int16_t x;
                  int16_t y;
                  int16_t z;
              } accel;

            struct {
                int16_t x;
                 int16_t y;
                int16_t z;
            } gyro;

            int16_t ff_pressure[NUM_FLEXIFORCE_SENSORS];
            uint32_t packet_num;
        };

         struct {
             uint16_t error_code;
            const char * error_msg;
        };
    };
} SaadcSample;

Now you can do something like:

Code:

extern struct bmi160_dev * Sensor;  // Don't know where this is.

#define THERE_WAS_AN_ERROR(...) (random() % 4 == 0)   // 75:25 shot

void 
saadc_sample(
    SaadcSample * sp)
{
     static packet_counter = 0; 
             
    init_samples(sp);
    read_imu(sp);
    read_ff_bank(sp);

    if (sp->is_valid)
        sp->packet_num = packet_counter++;

    DUMP_SAMPLES(sp);
}

void
init_samples(
    SaadcSample * sp)
{
    memset(sp, 0, sizeof(*sp));
    sp->is_valid = 1;
}

void
read_imu(
    SaadcSample * sp)
{
    enum { 
        ACCEL, 
         GYRO,
        NUM_SENSORS,
         WHICH_SENSORS = (BMI160_ACCEL_SEL | BMI160_GYRO_SEL)
    };

    if (!sp->is_valid)
        return;

    struct bmi160_sensor_data bmi_data[NUM_SENSORS];

    bmi160_get_sensor_data(WHICH_SENSORS, &bmi_data[ACCEL],            &bmi_data[GYRO], Sensor);

    if (THERE_WAS_AN_ERROR(Sensor)) {
       sp->is_valid = 0;
      sp->error_code = ...;
      sp->error_message = "Error reading bmi160 sensors";
       return;
    }

    sp->accel.x = bmi_data[ACCEL].x;
    sp->accel.y = bmi_data[ACCEL].y;
    sp->accel.z = bmi_data[ACCEL].z;

    sp->gyro.x = bmi_data[GYRO].x;
    sp->gyro.y = bmi_data[GYRO].y;
    sp->gyro.z = bmi_data[GYRO].z;
}

void
read_ff_bank(
    SaadcSample * sp,
    int sample)
{
    if (!sp->is_valid)
        return;

    for (int i = 0; i < NUM_FLEXIFORCE_SENSORS; ++i) {

        read_ff_pressure(sp, i);

         if (THERE_WAS_AN_ERROR()) {
             sp->is_valid = 0;
          sp->error_code = ...;
          sp->error_message = "Error reading Flexiforce sensors";
            return;
        }
    }
}

void
read_ff_pressure(
    SaadcSample * sp,
    int i)
{
#define DUMMY_FF_READING(i)                                         \
    (((offsetof(SaadcSample, ff_pressure)                            \
    - offsetof(SaadcSample, accel)) / sizeof (sp->ff_pressure[0]))\
              + 1000 + (i))

    sp->ff_pressure[i] = DUMMY_FF_READING(i);

#undef DUMMY_FF_READING
}

Originally Posted by aghast

A lot of information!! I will read your suggestions carefully and revert if I have questions!! Thanks a lot

Originally Posted by Salem

A pointer to the whole structure is the same (value-wise) as a pointer to the first member of the structure.
The only thing that changes is the TYPE of that pointer, and whether you need to do any casting or not.

I just picked the one which may have worked without the cast.

So long as you tell the transmitter that the pointer is pointing to 32 bytes, it won't know or care that you chose to declare it as

Code:

uint8_t bytes[32];

or
uint16_t words[16];

or
struct {
    int16_t    imu[6];
    int16_t    pressure[8];
    int16_t    counter[2];
} payload;

Or any other way you choose to represent your data.
So long as it ends up as 32 consecutive bytes, it's all good man.

Thank you very much Salem!!

I have a lot of questions but let's start from a simple one

Originally Posted by aghast

Code:

enum { NUM_FLEXIFORCE_SENSORS=8 };

Why you use anonymous enum instead of:

Code:

#define NUM_FLEXIFORCE_SENSORS 8 // OR
const uint8_t num_flexiforce_sensors = 8;

Originally Posted by Nikosant03

I have a lot of questions but let's start from a simple one


Why you use anonymous enum instead of:

Code:

#define NUM_FLEXIFORCE_SENSORS 8 // OR
const uint8_t num_flexiforce_sensors = 8;

Like a lot of things, it's mainly from habit. ;-)

I don't use variables (such as const uint8_t num_flexiforce_sensors = 8;) because they are variables, not constants. You cannot use a variable to size an array at all in pre-VLA versions of C, and even after VLAs there are restrictions on where they can be used. File scope is right out.

I prefer enums over preprocessor symbols because historically, using enum created entries in debug records, and because enums could be types, which made it possible for the compiler to warn you if you made certain kinds of mistakes. (Not relevant in this situation, since I was just using the enum to get a named constant.)

The part about enums creating debug records is important because if you see a value in memory, or in a register, and the value is "17" it doesn't tell you anything. "errno is 17, I wonder what that is?" Whereas, if you can get the debugger or dumper to show you "errno = EEXIST" that saves a lot of time and curiousity.