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

calibrate can cause value overflow #38

Closed
INemesisI opened this issue Jan 17, 2019 · 3 comments
Closed

calibrate can cause value overflow #38

INemesisI opened this issue Jan 17, 2019 · 3 comments
Assignees
Labels

Comments

@INemesisI
Copy link

INemesisI commented Jan 17, 2019

calibrate() checks if the local variable int8_t trim goes out of bound of 130 and -130.
However a 8bit value cannot exceed [128,-127], so checking for an out of bound condition at this point is too late, since it already happened.

Affected lines of code:
https://github.com/SV-Zanshin/MCP7940/blob/0678a66569f9ea81a9d959c8c6cce5247a6916eb/src/MCP7940.cpp#L497 [...]
https://github.com/SV-Zanshin/MCP7940/blob/0678a66569f9ea81a9d959c8c6cce5247a6916eb/src/MCP7940.cpp#L511-L516

Fix:
I think the trim value should be changed to a uint16_t. calibrate(uint8_t) needs to be fixed accordingly too.

@SV-Zanshin SV-Zanshin self-assigned this Jan 18, 2019
@SV-Zanshin SV-Zanshin added the bug label Jan 18, 2019
@SV-Zanshin
Copy link
Collaborator

Good catch.

The MCP7940 stores the TRIM in the OSCTRIM register, which is only 8-bit. This means the range is -127 to +127 (it doesn't use excess-128, just a sign bit, so the + and the - ranges are identical). So I just need to make the temporary register to compute the new trim 16-bit and adjust the trigger ranges. I've made those changes.
I will have to look around for a MCP7940 in my collection to test the changes, unless you can do so.

SV-Zanshin pushed a commit that referenced this issue Jan 18, 2019
The calculation of the "trim" value during calibration had an error with overflow which has been addressed
SV-Zanshin pushed a commit that referenced this issue Jan 18, 2019
Corrected excess-128 trim value range
@INemesisI
Copy link
Author

INemesisI commented Jan 18, 2019

These changes look fine to me, Thank you!
I could have a look, sure. However I am not sure, how to test these changes the best. Maybe just by setting a frequency that should exceed the threshold and reading the register back after that?

I am not using the calibrate function at the moment and just noticed it along the way of using the library.

@SV-Zanshin
Copy link
Collaborator

OK, in that case don't bother - I'll test this change once I get my system setup with a MCP7940.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants