-
Notifications
You must be signed in to change notification settings - Fork 33
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
Fix bug with unsigned types and negative factors #78
Merged
killercup
merged 21 commits into
technocreatives:main
from
cpctaylo:fix-negative-factor
Jun 26, 2024
Merged
Fix bug with unsigned types and negative factors #78
killercup
merged 21 commits into
technocreatives:main
from
cpctaylo:fix-negative-factor
Jun 26, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cpctaylo
changed the title
Fix negative factor (WIP)
Fix bug with unsigned types and negative factors (WIP)
May 21, 2024
cpctaylo
changed the title
Fix bug with unsigned types and negative factors (WIP)
Fix bug with unsigned types and negative factors
May 21, 2024
cplnixon
approved these changes
Jun 25, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented logic seems sound and test coverage looks thorough 👍
killercup
approved these changes
Jun 26, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I trust you guys!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a bug in how the library chooses Rust types to represent signals.
We tried a DBC with a 16 bit unsigned signal with a factor of -1. This means the range of valid values is
[-65535, 0]
. But this confused the functionscaled_signal_to_rust_int
which chosei8
when it should have pickedi32
.This PR will:
scaled_signal_to_rust_int
to look at the whole range of values for a signal and pick the smallest Rust type.min
andmax
values when deciding Rust types for a signal becauseConfig.check_ranges
can disable range checking. To be on the safe side, this picks types assuming the worst-case values for a signal.testing/can-messages/src/messages.rs
because it doesn't work on Windows. Nowtesting/can-messages/build.rs
copies the file fromcan-messages
tocan-embedded
.can-messages
goes beforecan-embedded
.UnsignedNegativeFactorSignal
inexample.dbc
will reproduce the bug.Happy to address any comments or change requests.