technocreatives / dbc-codegen

Generate Rust structs for messages from a dbc (CAN bus definition) file.
Apache License 2.0
43 stars 23 forks source link

Add enum matching for floating values #1

Closed marcelbuesing closed 3 years ago

marcelbuesing commented 3 years ago

Ran this against a dbc and noticed it generates the following issue, so I added this minimal breaking example. The issue originates here so I assume since matching against f32 is not possible it probably would make sense here to cast to an integer type here.

Error:

error[E0308]: mismatched types
   --> testing/can-messages/src/messages.rs:389:13
    |
388 |         match self.five_raw() {
    |               --------------- this expression has type `f32`
389 |             2048 => DolorFive::Dolor,
    |             ^^^^
    |             |
    |             expected `f32`, found integer
    |             help: use a float literal: `2048.0
killercup commented 3 years ago

Thanks for the report! Can't promise that we'll have time to fix it soon…

Do you think comparing the byte representation would be a good fix?

marcelbuesing commented 3 years ago

Honestly, I don't know. It might work.

marcelbuesing commented 3 years ago

I think the issue with bitwise comparison would be the inaccuracy of the floating point calculation of the signal value. I checked what the recommended way is to compare floats and apparently the consensus seems to be to compare the absolute difference. Controversial is probably the epsilon to compare against.

marcelbuesing commented 3 years ago

Just tested building for thumbv6m-none-eabi and ran into this. I assumed the fns in the primitives would be available. Not sure what the alternative is here.

cargo build --target thumbv6m-none-eabi --no-default-features

error[E0599]: no function or associated item named `abs` found for type `f32` in the current scope
   --> testing/can-messages/src/messages.rs:732:23
    |
732 |             x if f32::abs(x - 6_f32) < f32::EPSILON * 3.0 => DolorOneFloat::Dolor2,
    |                       ^^^ function or associated item not found in `f32`
killercup commented 3 years ago

Thanks for testing! @fry recommends using https://docs.rs/micromath/1.1.0/micromath/ for math on embedded.

Do you think we can do this in two stages? First the "bad match", then the proper comparison?

marcelbuesing commented 3 years ago

Depends I mean merging this would basically mean no_std wont work until its fixed. On the other hand right now there are compiler errors if you have enum and floats. Micromath looks good. Does it make sense to generate a differents code here for no_std and std or is it not worth the complication?

killercup commented 3 years ago

Do you think we can do this in two stages?

Sorry I meant this: match on the value as bytes, early rerturn if successful, then match using proper float comparison with an epsilon value. But disregard this for now, it's just a premature optimization idea :)

Yeah, micromath is gonna shadow the methods from std. A smaller alternative might be float-cmp. I have never used both.

marcelbuesing commented 3 years ago

float-cmp looks good to me and seems widely used, will probably go with it.

marcelbuesing commented 3 years ago

Ok reading the docs of the float-cmp crate I think it make actually make sense to basically rely on it all the time. E.g. that part about comparing of negative and positive numbers:

(consider comparing -0.0000000028 to +0.00000097).

Just added the implementation.

marcelbuesing commented 3 years ago

Already rebased on master. I also added float-cmp and arbitrary to the list of dependencies in the readme, not sure if there is a better way to make this more visible, I agree though it's one of the things that is easily missed and might be confusing otherwise.

andresv commented 3 years ago

Can we merge that one?

killercup commented 3 years ago

Looks good! Thanks for the quick updates!