Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't treat signals as floats if factor is a whole number #60

Merged
merged 3 commits into from
Dec 18, 2023

Conversation

hulthe
Copy link
Contributor

@hulthe hulthe commented Nov 20, 2023

Before this change, any signal with offset != 0.0 and factor != 1.0 would be represented as a float in the generated Rust.

This change checks if factor and offset are integers. If so, the generated rust type is set to the smallest integer type that fits the value (accounting for offset and factor).

So a signal like this (8 bits, factor=2, offset=13) would be a u16:

  SG_ MySignal : 0|8@1+ (2,13) [1|256] "" Vector__XXX

Unresolved Questions

  • How to handle signals that, when accounting for offset and factor, exceed the capacity of u64s?
    Currently they willl always be truncated to fit in a u64
  • How to handle invalid values provided to set_<signal> methods.
    I.e., what should happen if you provide 0 to a signed integer signal with offset 1.

@hulthe hulthe requested a review from killercup November 20, 2023 15:24
@hulthe hulthe self-assigned this Nov 20, 2023
@killercup
Copy link
Member

How to handle signals that, when accounting for offset and factor, exceed the capacity of u64s?
Currently they willl always be truncated to fit in a u64

Why not fall back to f64? Might produce a warning if desired.

Copy link
Member

@killercup killercup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! lgtm with ci fixed/clippy appeased

src/lib.rs Show resolved Hide resolved
/// - Max: 256
/// - Unit: ""
/// - Receivers: Vector__XXX
#[inline(always)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memo to self: review those :D

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@hulthe hulthe force-pushed the integer-factors-and-offsets branch from c48b36b to 869cd28 Compare December 18, 2023 10:54
@hulthe
Copy link
Contributor Author

hulthe commented Dec 18, 2023

Did some tests with overflowing u64s, and it looks like dbc-codegen doesn't handle those well for other reasons too x)

I'm thinking we leave it. Would like to get this merged.

@hulthe hulthe merged commit 13edfa1 into main Dec 18, 2023
2 checks passed
@hulthe hulthe deleted the integer-factors-and-offsets branch December 18, 2023 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants