Megunolink / MLP

Arduino library for sending data to MegunoLink visualisers and useful components
http://www.megunolink.com
GNU Lesser General Public License v3.0
46 stars 26 forks source link

wrong in MLP/EEPROMStore.h #8

Closed tinamore closed 3 years ago

tinamore commented 3 years ago

hi, i have found a error in code:

bool Load(CEEPROMData &Result)
  {
    eeprom_read_block(&Result, (const void *)&m_EEPROMData, sizeof(CEEPROMData));
    uint16_t uChecksum = CalculateChecksum(Result.m_UserData);
    return uChecksum == Result.m_uChecksum;
  }

when new arduino board => EEPROM is empty => uChecksum = 0 and Result.m_uChecksum = 0 => uChecksum == Result.m_uChecksum => is true => CEEPROMData set zero default value.

tinamore commented 3 years ago

edit the code:

bool Load(CEEPROMData &Result)
  {
    eeprom_read_block(&Result, (const void *)&m_EEPROMData, sizeof(CEEPROMData));
    uint16_t uChecksum = CalculateChecksum(Result.m_UserData);

    // if empty eeprom then false
    if(uChecksum == 0)
        return false;

    return uChecksum == Result.m_uChecksum;
  }
PaulMartinsen commented 3 years ago

Hi,

Thanks for your message. Which Arduino board are you using? The EEPROM on AVR boards reads 0xff (255) when reset or empty which won't match the data checksum in most (65535 out of 65536) cases. So the load implementation should work as expected.

Is it not working for you?

Have a good day Paul.

tinamore commented 3 years ago

Hi, I use board Arduino nano, and i use Arduino version 1.8.13 This is clear test code:

#include "EEPROMStore.h"

struct GreenhouseControllerConfiguration
{
  int TemperatureSetPoint;
  int HumiditySetPoint;

  void Reset()
  {
    // Set default values in case there is nothing 
    // stored in the eeprom yet. 
    TemperatureSetPoint = 10;
    HumiditySetPoint = 80;
  }
};

EEPROMStore<GreenhouseControllerConfiguration> Configuration;

void setup() {
  // put your setup code here, to run once:
  Serial.begin(115200);
  Serial.println("Hello world...");

// clear EEPROM
/*
 for (int i = 0 ; i < EEPROM.length() ; i++) {
    EEPROM.write(i, 0);
  }
*/

  Serial.print("Temperature set-point: "); 
  Serial.println(Configuration.Data.TemperatureSetPoint); //expect is 10 but show is 0
}

void loop() {
  // put your main code here, to run repeatedly:

}

Have a nice day and thank you to your library

tinamore commented 3 years ago

image

PaulMartinsen commented 3 years ago

Thanks for the test code. I believe I see the problem. Your code to clear the eeprom sets all values to 0, but a new device will have 0xFF stored in all values in the eeprom.

We use the CRC16 checksum with a seed of 0. Which calculates a checksum of 0 when given all zero input. Which would happen if you set the temperature and humidity setpoints to zero then save the configuration in the eeprom. Then calling reset if the checksum was zero would call the reset function returning the set points to 10°C and 80% RH. The current implementation will handle that correctly (because the checksum of an eeprom filled with 0xff is not 0xffff).

#include <EEPROM.h>

struct GreenhouseControllerConfiguration
{
  int TemperatureSetPoint;
  int HumiditySetPoint;

  void Reset()
  {
    // Set default values in case there is nothing
    // stored in the eeprom yet.
    TemperatureSetPoint = 10;
    HumiditySetPoint = 80;
  }
};

void setup() 
{
  // put your setup code here, to run once:
  Serial.begin(115200);
  Serial.println("Hello world...");

#if 0
  // zero EEPROM
  Serial.println(F("Zeroing eeprom"));
  for (int i = 0 ; i < EEPROM.length() ; i++) 
  {
    EEPROM.write(i, 0);
  }
#endif

#if 1
  // reset EEPROM to factory state
  Serial.println(F("Reset eeprom"));
  for (int i = 0 ; i < EEPROM.length() ; i++) 
  {
    EEPROM.write(i, 0xff);
  }
#endif

  // Normally this would be global. Placing it here 
  // lets us immediately see the effect of changing 
  // the eeprom above.
  EEPROMStore<GreenhouseControllerConfiguration> Configuration;

  Serial.print("Temperature set-point: ");
  Serial.println(Configuration.Data.TemperatureSetPoint);

  Serial.print("Humidity set-point: ");
  Serial.println(Configuration.Data.HumiditySetPoint); 
}

void loop() 
{
  // put your main code here, to run repeatedly:
}

image

tinamore commented 3 years ago

Great, I understood the problem. The library EEPROMStore.h uses is great for saving and read configs for AVR. Thank you very much