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

I2C clock config uses uint16_t values #36

Closed hexeguitar closed 5 years ago

hexeguitar commented 5 years ago

The constants and the function arguments for setting the I2C clock speed are uint_16t, but the actual values are out of range (100000, 400000), hence they are truncated (compiler throws a warning).

Fix: in MCP7940.h line 72 & 73

#ifndef I2C_MODES                                                           // I2C related constants            //
  #define I2C_MODES                                                         // Guard code to prevent multiple   //
  const uint16_t I2C_STANDARD_MODE      =     100000;                       // Default normal I2C 100KHz speed  //
  const uint16_t I2C_FAST_MODE          =     400000;                       // Fast mode                        //
#endif  

line 185:

      bool     begin(const uint16_t i2cSpeed = I2C_STANDARD_MODE); 

in MCP7940.cpp line 231

bool MCP7940_Class::begin(const uint16_t i2cSpeed) {
...

change the uint16_t to uint32_t Arduino Wire library expects an uint32_t passed to the setClock method.

SV-Zanshin commented 5 years ago

Good catch - added fix Thanks!

davidlehrian commented 5 years ago

I just updated my library and got the fix that hexeguitar mentioned above, and while it corrected the truncation, I now get a warning, "MCP7940.h: 68:49: warning: large integer implicitly truncated to unsigned type" for both lines 68 and 69. The integers 100000 and 400000 need to be cast to (uint16_t) as:

const uint16_t I2C_STANDARD_MODE = (uint16_t)100000; // Default normal I2C 100KHz speed // const uint16_t I2C_FAST_MODE = (uint16_t)400000; // Fast mode //

and the warning goes away.

SV-Zanshin commented 5 years ago

I was wondering with which compiler you were getting this error, as I didn't see this on any of the builds I did. Then, looking at your code snippet, I realized that you still had the old "uint16_t" definition rather than the correct "uint32_t". If you change the datatypes (or download the newer library version), then you can get rid of the casts.

davidlehrian commented 5 years ago

Thanks for the reply. I should have caught that when I read this thread. The code he quoted was the code that he changed FROM not what he changed the code TO. And if I thought about it I should have realized that 16 bits only goes up to 65536. Anyway, apparently the version of the library that gets installed via the Arduino interface has this error in it. I don't run the Arduino interface much but I updated it the other day as a new project required an updated version and I also updated all my installed libraries. The MCP7940 library was updated to Version 1.1.1 and when I compiled my project I saw this error. I don't know how the code gets from GitHub to the Arduino Library Manager, but it needs to be updated.

SV-Zanshin commented 5 years ago

The most recent version of the library is 1.1.5 and I don't know why the library manager isn't picking up the newest (and most correct) version.

I've opend up a ticket for this at https://github.com/arduino/Arduino/issues/8450 and we'll see what might be causing the issue. If you don't want to wait for the correct version to show up in the IDE's library manager you could always download the most recent version and install it manually.

davidlehrian commented 5 years ago

Ah, interesting.

No worries, I fixed the warning in my version of the library. I'll watch for Version 1.1.5.

SV-Zanshin commented 5 years ago

It seems that I made my file changes in the wrong order and thus the library was tagged with an old version in the library manager. I've corrected that and now the MCP7940 library shows up with version 1.1.6 in the Arduino IDE.

davidlehrian commented 5 years ago

I just updated via the Arduino Library Manager and it indeed updated to version 1.1.6 and compiles w/o warning. Thanks for looking at this.