Zanduino / INA

Combined Arduino library for reading multiple INA2xx power monitor devices
GNU General Public License v3.0
158 stars 41 forks source link

Hardcode EEPROM size #62

Closed fg89o closed 4 years ago

fg89o commented 4 years ago

I think there should be a posibility to set EEPROM size in begin() method. Before your last merged I did a test with an EEPROM_offset and works correctly but changing the line

EEPROM.begin(512).

For example, If I need 400 bytes for my main project and I want another 512 bytes for INA library the begin method should set EEPROM to 912. Right?. You can expose antoher variable to set EEPROM size and initializate to a default value to have backwards compatibility.

In the other hand, you calculate max_devices including the EEPROM_offset variable ,If the library is not allowed to use the offset, the maximum devices should be:

maxDevices = (EEPROM_size - _EEPROM_offset ) / sizeof(inaEE);

instead of

maxDevices = (_EEPROM_offset + EEPROM_size) / sizeof(inaEE);

I did all the tests with an ESP32. Hope something of that help you to improve the library,

SV-Zanshin commented 4 years ago

:thumbsup: I missed that, good catch and I've added the correct offset math to the code.

fg89o commented 4 years ago

Are you going to create the EEPROM size variable too?.

SV-Zanshin commented 4 years ago

I wanted to fix the actual bug first, before the new release was published.

I still need to think about changing the begin() method, which is why this issue is still open.

SV-Zanshin commented 4 years ago

What I will do is add another public uint16_t variable "_EEPROM_size" which is only valid for the ESP32 and ESP8266 platform. It defaults to 512 but can be changed before the begin() is called.

fg89o commented 4 years ago

Perfect! Thank you for your support.

SV-Zanshin commented 4 years ago

OK, that should work!