search

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

Fix start and end bit of big endian encoding #20

Closed marcelbuesing closed 3 years ago

marcelbuesing commented 3 years ago

So currently there is a bug where signals such as the following will panic with a subtraction overflow (signal size is greater than signal start). So I have looked into cantools again and found this. I actually thought I had understood how it works but apparently I don't. Looks like it was basically a coincidence that the previous tests worked when comparing the packing results between dbc-codegen and cantools. I have added a new message/signal that would otherwise fail without the fix.

I assume @bitbleep can judge best on this fix.

SG_ OneFloat : 0|12@0+ (1,0) [0.00|130.00] "" Vector__XXX
killercup commented 3 years ago

I trust this is correct purely based on the green checkmark 😅

bitbleep commented 3 years ago

I too am confused by this and unsure if I have misinterpreted the information when it comes to big endian signals. But my understanding is that for big endian signals the start bit in the signal definition (0 in your example above) points to the MSB of the signal ie. the signal definition for OneFloat above is invalid (you would need to have a start bit of at least 11 to be able to extract 12 bits out of the bit array).

Not on my normal computer and missing the DBC tidbits I've collected but the section "Bit positions" on this page seems to back up my theory.

bitbleep commented 3 years ago

Also, if what I wrote is actually true that also means that the panic you got to begin with would actually be correct right?

bitbleep commented 3 years ago

No, giving this another round of thought this makes sense. The MSB of a big endian signal would be in the first byte, NOT where I placed it in my head while I was writing the things I wrote above. Never mind me, I'm obviously too tired to think about smart things atm.

It looks good!

marcelbuesing commented 3 years ago

Thanks for the link given the "Bit position" doc it seems to make sense.

bitbleep commented 3 years ago

Yes, agree. Nice find @marcelbuesing and thanks for putting time and effort into this!

killercup commented 3 years ago

Happy to see you're both less confused and more convinced of the implemenation now :) I'll merge this now.

Random other idea I just had: Should we prohibit accidental overflowing and return proper errors instead?

marcelbuesing commented 3 years ago

Happy to see you're both less confused and more convinced of the implemenation now :) I'll merge this now.

Random other idea I just had: Should we prohibit accidental overflowing and return proper errors instead?

Thanks for merging it. Yes I think this might actually be a good idea especially for the generated code.