search

Zanduino / MCP7940

Arduino Library to access the MCP7940M, MCP7940N and MCP7940x Real-Time chips
GNU General Public License v3.0
35 stars 22 forks source link

calibrate can cause value overflow #38

Closed INemesisI closed 5 years ago

INemesisI commented 5 years ago

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 commented 5 years ago

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.

INemesisI commented 5 years ago

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 commented 5 years ago

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