Thread: Could you review my code?

  1. #1
    Registered User
    Join Date
    Feb 2019
    Posts
    89

    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);

  2. #2
    Registered User
    Join Date
    May 2009
    Posts
    4,098
    Avoid magic numbers/constants in programming.

    Magic number (programming) - Wikipedia

    Tim S.
    "...a computer is a stupid machine with the ability to do incredibly smart things, while computer programmers are smart people with the ability to do incredibly stupid things. They are,in short, a perfect match.." Bill Bryson

  3. #3
    and the hat of int overfl Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    38,904
    > // Read the flexi force sensors
    This loop should be in another function.

    > uint8_t pressure_sensors; // number of pressure sensors
    A better name would save a comment.

    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;
    };
    Also line 6 needs to have some newlines in it.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.

  4. #4
    Registered User
    Join Date
    Feb 2019
    Posts
    89
    Quote Originally Posted by stahta01 View Post
    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..

  5. #5
    Registered User
    Join Date
    Feb 2019
    Posts
    89
    Thank you Salem!!

    Quote Originally Posted by Salem View Post
    >


    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?

  6. #6
    and the hat of int overfl Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    38,904
    > 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

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

    Since the inner payload structure contains members of the same type, there will be no gaps between members.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.

  7. #7
    Registered User
    Join Date
    Feb 2019
    Posts
    89
    Quote Originally Posted by Salem View Post
    > 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)

    Quote Originally Posted by Salem View Post
    > 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

  8. #8
    Registered User
    Join Date
    Apr 2021
    Posts
    75
    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
    }

  9. #9
    and the hat of int overfl Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    38,904
    > I must assign the whole payload to p_data, otherwise just the imu will be transmitted (bluetooth application)
    It's just a pointer!
    hvx_params.p_data = sampling->payload.imu;
    hvx_params.p_data = (uint8_t*)&sampling->payload.imu[0];
    hvx_params.p_data = (uint8_t*)&sampling->payload;

    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.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.

  10. #10
    Registered User
    Join Date
    Feb 2019
    Posts
    89
    Quote Originally Posted by aghast View Post
    A lot of information!! I will read your suggestions carefully and revert if I have questions!! Thanks a lot

  11. #11
    Registered User
    Join Date
    Feb 2019
    Posts
    89
    Quote Originally Posted by Salem View Post


    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!!

  12. #12
    Registered User
    Join Date
    Feb 2019
    Posts
    89
    I have a lot of questions but let's start from a simple one
    Quote Originally Posted by aghast View Post

    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;

  13. #13
    Registered User
    Join Date
    Apr 2021
    Posts
    75
    Quote Originally Posted by Nikosant03 View Post
    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.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Please review my code
    By ari01 in forum C++ Programming
    Replies: 2
    Last Post: 02-20-2014, 01:01 PM
  2. How to get a code review
    By Richardcavell in forum C Programming
    Replies: 42
    Last Post: 03-19-2011, 10:36 PM
  3. review this code
    By KIBO in forum C Programming
    Replies: 12
    Last Post: 08-14-2007, 02:28 PM
  4. Code Review
    By Thantos in forum C Programming
    Replies: 8
    Last Post: 03-06-2004, 06:20 PM
  5. review code
    By absenta in forum C++ Programming
    Replies: 3
    Last Post: 04-09-2002, 02:13 PM

Tags for this Thread