send_exception_msg() implicit signed conversion

[mrnoise-ko]

The send_exception_msg() will take an uint8_t exception argument but is used throughout the code with the nmbs_error enum. The enum is signed because values <=0 are library errors. So there is a implicit signed conversion happening.

I know that only values >0 should be send via send_exception_msg() because these values are known modbus error codes but I think it is bad design because it will break if someone changes the code without being aware of that.

I would change the function(untestet):

static uint8_t clamp_exception_msg(nmbs_error exception) {
    const int8_t except = (int8_t) exception;
    if (except < 0)
        return 0;
    return (uint8_t) except;
}

static nmbs_error send_exception_msg(nmbs_t* nmbs, nmbs_error exception) {
    nmbs->msg.fc += 0x80;
    put_msg_header(nmbs, 1);
    put_1(nmbs, clamp_exception_msg(exception));

    NMBS_DEBUG_PRINT("%d NMBS res -> address_rtu %d\texception %d", nmbs->address_rtu, nmbs->address_rtu, exception);

    return send_msg(nmbs);
}
´´´´

[debevv]

Yes, i think that preventing an (unlikely) implicit conversion is a good idea. However, clamping it to 0 will basically give the same result: the value will be altered unbeknownst to the user. I think a better approach would be printing a big giant error and then, maybe, stopping the program. The priority is letting the user know that she is sending a negative exception number.

[mrnoise-ko]

Yeah you are right! Maybe an assert() is the right way to handle this?

[debevv]

I think returning an NMBS_ERROR_INVALID_ARGUMENT could suffice, since you should check the return value anyway to be sure that the exception was actually sent.