[uart] Code review

[TheComet]

1

You've got such a nice macro VOLTAGE_LENGTH but you're manually specifying the buffer_str size.

    char buffer_str[7]; /* five digits, sign, '\0' terminator */
    short buffer_num;

    buffer_num = convert_milli(model_get_open_circuit_voltage(cell_id));
    str_nitoa(buffer_str, VOLTAGE_LENGTH, buffer_num);

Please use char buffer_str[VOLTAGE_LENGTH + 2]

2

struct data_t {
    union
    {
        struct {
            unsigned char selected_cell;  /* number representing the cell */
            unsigned int param;           /* holds numerical value
                                           * UNITS:
                                           * I: milli ampere
                                           * U: milli volt
                                           * T: 1/10ths degrees celsius
                                           * E: percent */
            unsigned char was_digit;      /* flag for indicating whether
                                           * received char was digit or not */
            unsigned int voltage;         /* millivolts */
            unsigned int current;         /* milliamperes */
            unsigned int temperature;     /* 1/10ths Kelvin */
            unsigned int exposure;        /* percent */
        } config_cell;
        struct {
            unsigned char selected_cell;
        } config_dump;
    };
};

• Use unsigned short
• voltage, current, temperature, and exposure all do the same thing as "param" and yet they're in the same struct. Suggest making a union called "param" and adding voltage, current, temperature and exposure to that union (since those items are all mutually exclusive when parsing). Then you can type param.voltage or param.current and save memory.

  3

You can refactor state_data.config_cell.was_digit = 0; out of the switch/case.

            switch (data)
            {
                case 'I':
                    state_data.config_cell.was_digit = 0;
                    state = STATE_CONFIG_SHORT_CIRCUIT_CURRENT;
                    break;

                case 'U':
                    state_data.config_cell.was_digit = 0;
                    state = STATE_CONFIG_OPEN_CIRCUIT_VOLTAGE;
                    break;

                case 'T':
                    state_data.config_cell.was_digit = 0;
                    state = STATE_CONFIG_TEMPERATURE;
                    break;

                case 'E':
                    state_data.config_cell.was_digit = 0;
                    state = STATE_CONFIG_EXPOSURE;
                    break;

                default:
                    state = STATE_IDLE;
                    break;
            }

4

Suggest making a macro is_number

#define is_number(x) \
        (x >= '0' && x <= '9')

5

This code is redundant

            } else if (data == 'a' && state_data.config_cell.was_digit == 1) {
                /*
                 * This is an error, revert back to idle state. All variables
                 * will be reset there as well.
                 */
                state = STATE_IDLE;
            } else {
                model_cell_remove(state_data.config_cell.selected_cell);
                state = STATE_IDLE;
            }

Can be shortened to:

            } else {
                model_cell_remove(state_data.config_cell.selected_cell);
                state = STATE_IDLE;
            }

6

No error checking is done on the returned value of

            /* First, create a new cell and get its ID */
            state_data.config_cell.selected_cell = model_cell_add();

If we failed to add the cell you need to send back a zero.

7

No error checking is done in general. Make sure to send back the return values we discussed.