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

incorrect bits used to determine b's sign - leads to wildly off readings #4

Merged
merged 2 commits into from
Jan 16, 2024

Conversation

richardstephens
Copy link
Contributor

While testing on a Dell PowerEdge R630, we noticed temperature readings that were wildly off:
Screenshot 2024-01-16 at 1 24 10 pm

We observed that the value of B used in the reading conversion formula was 896, but the Full sensor record SDR defines B as a 10-bit signed integer. We tracked it down to FullSensorRecord::parse(...) using one of the accuracy bits to determine the sign of B.

With this change, we see sensible numbers that are in line with what we get from ipmitool (debug prints added). I'm still investigating why ipmi-rs gives us a bogus value for lower non-recoverable.
Screenshot 2024-01-16 at 1 44 18 pm
Screenshot 2024-01-16 at 1 44 41 pm

@richardstephens richardstephens force-pushed the temp-bug-pr branch 2 times, most recently from 5d7da22 to a161892 Compare January 16, 2024 13:04
@richardstephens
Copy link
Contributor Author

I wasn't sure the best place to put the tests.

@datdenkikniet
Copy link
Owner

Good catch, thank you! Please fix the comment and I will merge + release :)

I'm still investigating why ipmi-rs gives us a bogus value for lower non-recoverable.

The bogus value for the lower-non-recoverable value seems to be the lowest value that is supported by the sensor overall. I cannot find where it is defined clearly, but I assume that the minimum value is supposed to signal that the threshold is actually not supported.

src/lib.rs Outdated Show resolved Hide resolved
@richardstephens
Copy link
Contributor Author

Would prefer if we could put this test in a...with a separate file

Done, I think we'll probably want to add a few more tests too. I might also look at the broken ones, we're likely going to need to make some changes to RCMP as well.

@datdenkikniet datdenkikniet merged commit cac3f9e into datdenkikniet:main Jan 16, 2024
@datdenkikniet
Copy link
Owner

Awesome, thanks! Merged, will cook up a release :)

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