Round f32 when used as integers

[mvalenzuelamoixa]

I was finding problems with values that after scaling were not exact, such as 79.99999 being truncated to 79 instead of rounded to 80. I have added a test and the fix for this use case.

Let me know if this was a design choice because of performance and I have misunderstood it to be a bug.

[killercup]

Thanks! To me this makes sense -- @marcelbuesing do you have an opinion of this? How does it interact with min/max settings?

Looks like there is no round for our target, so approx might be needed after all :)

[mvalenzuelamoixa]

I just realised that the can-embedded crate exists, so yes, we would need something for no_std. Let me see what I can do.

[mvalenzuelamoixa]

I have implemented rounding with num-traits in this commit: mvalenzuelamoixa@43f759a

However, that adds a new dependency to users of the generated code. Any thoughts on that?

[marcelbuesing]

Thanks! To me this makes sense -- @marcelbuesing do you have an opinion of this? How does it interact with min/max settings?

Looks like there is no round for our target, so approx might be needed after all :)

Honestly I don't know. I think I would probably check this against the cantools implementation. So basically regenerate this part using the changes you added to the example.dbc @mvalenzuelamoixa . And look at their C code in comparison. Regeneration is described here

I just realised that the can-embedded crate exists, so yes, we would need something for no_std. Let me see what I can do.

Yes even better, embedded-hal just added alpha CAN support based on the can-embedded-hal experience.

[mvalenzuelamoixa]

Hi @marcelbuesing. As you recommended I added a test on the cantools-messages crate after regenerating the C code. It seems that without rounding the test breaks and with rounding it works. I have pushed it to mvalenzuelamoixa#1 for the time being.

However, rather than rounding, it seems that the main difference is in the floating point representation, with cantools using double instead of float:

uint16_t example_foo_inexact_voltage_encode(double value)
{
    return (uint16_t)(value / 0.001);
}

I've also tried changing the codegen to use f64 and this also makes the test pass.

[marcelbuesing]

Thanks you @mvalenzuelamoixa ! So if I understand correctly it would suffice to switch to f64 without rounding to make your new test pack_unpack_message_inexact_scale pass? In that case I think i would just replace all f32 usages with f64. Certainly makes the no_std part less troublesome.

[mvalenzuelamoixa]

So it does help for some specific inputs, but it seems like the truncation vs rounding problem still happens for some (I wrote a test that goes through all 2^16 inputs), which is not behaviour that I would expect. However, this is also the case for the cantools generated code.

For example (with f64 in the rust code):

input 65525 v 65.525 v_raw (can_tools) 65525 v_raw (rust) 65525 a 32.757000000000005 a_raw (can_tools) 32757 a_raw (rust) 32757
input 65526 v 65.526 v_raw (can_tools) 65525 v_raw (rust) 65525 a 32.757999999999996 a_raw (can_tools) 32757 a_raw (rust) 32757

With the .round() implementation:

input 65525 v 65.525 v_raw (can_tools) 65525 v_raw (rust) 65525 a 32.757000000000005 a_raw (can_tools) 32757 a_raw (rust) 32757
input 65526 v 65.526 v_raw (can_tools) 65525 v_raw (rust) 65526 a 32.757999999999996 a_raw (can_tools) 32757 a_raw (rust) 32758

To me, the behaviour below seems more sensible, and no_std support can be achieved via the num-traits crate or some other way. However, I understand if the crate wanted to keep behaviour close to cantools where possible.