nrwiersma / ConfigManager

ESP8266 Wifi connection and configuration manager.
MIT License
197 stars 79 forks source link

ConfigManager::begin() changes value of int in struct #106

Closed d42ohpaz closed 3 years ago

d42ohpaz commented 3 years ago

Below is a simple ino that reproduces the issue.

I have a struct that defines my configuration - it has an array of a struct of char * and a single int. Both the struct and ConfigManager are members of a class I'm writing. Before I call ConfigManager::begin() my int in the struct is correctly initialized to zero. However, after calling ConfigManager::begin() the value of the int becomes -1.

I have tried erasing my flash in case there was previous data being read, but that didn't change anything. I would appreciate any help and/or advice you have on this.

For testing purposes I am using Arduino 1.8.13 and ConfigManager 2.1.0. My device is an esp32 wroom 32D.

#include <ConfigManager.h>

const int CALENDARS = 8;

struct configuration_t {
  int calendars;
};

struct metadata_t {
  int version;
};

class Config {
  public:
  Config(): manager(new ConfigManager()) {
    manager->addParameter("calendars", &config.calendars);
    manager->addParameter("version", &meta.version);
  }

  void begin() {
    Serial.printf("Config::begin(): config.calendars = %d\n", config.calendars);
    Serial.printf("Config::begin(): meta.version = %d\n", meta.version);
    manager->begin(config);
    Serial.printf("Config::begin(): config.calendars = %d\n", config.calendars);
    Serial.printf("Config::begin(): meta.version = %d\n", meta.version);
  }

  private:
  configuration_t config{};
  metadata_t meta{};
  ConfigManager * manager;
};

void setup() {
  Serial.begin(115200);

  Config * config = new Config();
  config->begin();  
}

void loop() {
}

Edit: Updated demo code to remove unnecessary code and to add config parameters as in the example code.

cgmckeever commented 3 years ago

are you adding parameters like the example?

In your setup function start the manager

configManager.setAPName("Demo");
configManager.setAPFilename("/index.html");
configManager.addParameter("name", config.name, 20);
configManager.addParameter("enabled", &config.enabled);
configManager.addParameter("hour", &config.hour);
configManager.addParameter("password", config.password, 20, set);
configManager.addParameter("version", &meta.version, get);
configManager.begin(config);
d42ohpaz commented 3 years ago

@cgmckeever In my haste I didn't add the config initialization to my demo code. I've updated it now to reflect calling addParameter.

Weird thing is, when I add meta.version it initializes to zero, and stays zero. So it's only affecting my configuration_t struct.

16:24:27.263 -> Config::begin(): config.calendars = 0
16:24:27.263 -> Config::begin(): meta.version = 0
16:24:28.357 -> Config::begin(): config.calendars = -1
16:24:28.357 -> Config::begin(): meta.version = 0
cgmckeever commented 3 years ago

FWIW meta isnt save to the EEPROM, so it not getting clobbered the same way as the config makes sense -- although why the config is being set to -1 is baffling me. Im trying to parse back into the code ;)

d42ohpaz commented 3 years ago

meta isnt save to the EEPROM, so it not getting clobbered the same way as the config makes sense

Agreed. I had the same thought, but I wanted to share the result nonetheless. Thanks for taking a look!

cgmckeever commented 3 years ago

Have you tried to update the value AFTER you do the begin, and then restart the device to see if the value is preserved - or if it is getting clobbered? I faintly remember something about the initialized value like this, but I may be making this up

cgmckeever commented 3 years ago

ok, I seem to now think ints are set to -1 ... at least that is what my code for setting defaults is telling me https://github.com/cgmckeever/arduino-sketch/blob/master/esp-alexa-relay/relay-a/v2/main/main.ino#L71 .. see L:80

d42ohpaz commented 3 years ago

If I save the values to eeprom, it reads them fine after a reset. This at least gets me moving forward again.

cgmckeever commented 3 years ago

I think .. not sure .. its because NULL is treated as -1 .. this is the only hint I have https://github.com/nrwiersma/ConfigManager#clearsettingsbool-reboot

d42ohpaz commented 3 years ago

I thought NULL is the same as zero.

cgmckeever commented 3 years ago

yeah .. Im all confused myself right now .. my code seems to indicate that default ints are -1 and resetting the EEPROM sets things to null .. but ... I have no conclusive evidence outside of that

cgmckeever commented 3 years ago

talking more out my butt, BUT .. NULL for the EEPROM may be \xff https://stackoverflow.com/questions/28737829/whats-the-diffrence-between-xff-and-0xff

d42ohpaz commented 3 years ago

Looking at some EEPROM related discussions, it sounds like -1 (signed char) and 255 (unsigned char) - or 0xFF - is considered unwritten data. So it seems to be part of the EEPROM standard, which is what the emulated EEPROM follows, and your suggestion of checking the values of ints and saving the sane default may be the appropriate approach to handling things. I could be wrong.

At the very least, a warning in the documentation may be warranted.

nrwiersma commented 3 years ago

One question? When you see the issue, do you have WiFi credentials set in EEPROM? We should never read the config from EEPROM without the magic bytes set, which can only happen once you have actually saved your config before. This can be seen when enabling the debug log, it should print "Reading saved configuration".

Could you perhaps enable the debug logging and post the logs here? This would give us a really good idea of how this is happening.

d42ohpaz commented 3 years ago

When you see the issue, do you have WiFi credentials set in EEPROM?

Yes, I haven't tried this in AP mode.

Could you perhaps enable the debug logging and post the logs here?

MAC: 7C:9E:BD:ED:28:5C
Checking for magic initialization
Reading saved configuration
SSID: "nebakanezer"

Wifi connection attempt 1
Waiting for WiFi to connect
...
Connected to nebakanezer with 192.168.42.12
Station Mode
Webserver enabled on port: 80
Index page registered
Scan page registered
nrwiersma commented 3 years ago

Ok, i see the issue. When the initial wifi settings are written, the default config should also be written, but it no longer is.

nrwiersma commented 3 years ago

Can you give this branch a go: https://github.com/nrwiersma/ConfigManager/tree/store-init-config. You will need to start from a clean EEPROM.

d42ohpaz commented 3 years ago

I went through the nuclear option and erased my flash and reuploaded everything. It is doing the Right Thing. It appears the settings are written/read correctly - even in AP mode:

MAC: 7C:9E:BD:ED:28:5C
Checking for magic initialization
MagicBytes mismatch
Starting Access Point
AP Name: Desk Buddy Setup
AP IP address: 192.168.1.1
AP Api Mode
Webserver enabled on port: 80
Index page registered
Scan page registered
Config::begin(): config.calendars = 0 // A debug statement directly after calling ConfigManager::begin()
...
[E][WebServer.cpp:632] _handleRequest(): request handler not found
Unknown URL: captive.apple.com
Index Page: /wifi.html.gz
[E][WebServer.cpp:632] _handleRequest(): request handler not found
Unknown URL: captive.apple.com
Index Page: /wifi.html.gz
[E][WebServer.cpp:632] _handleRequest(): request handler not found
Unknown URL: captive.apple.com
Index Page: /wifi.html.gz
Storing WiFi Settings for SSID: "nebakanezer"
EEPROM committed: true
nrwiersma commented 3 years ago

What about on second boot, which i think is where you have the initial problem? Is that now fixed as well?

d42ohpaz commented 3 years ago

It comes back up as zero as expected.

MAC: 7C:9E:BD:ED:28:5C
Checking for magic initialization
Reading saved configuration
SSID: "nebakanezer"

Wifi connection attempt 1
Waiting for WiFi to connect
.......
Connected to nebakanezer with 192.168.42.12
Station Mode
Webserver enabled on port: 80
Index page registered
Scan page registered
Config::begin(): config.calendars = 0
nrwiersma commented 3 years ago

Awesome, then I will commit this tonight and cut a bug fix version.

cgmckeever commented 3 years ago

@nrwiersma Should that initial save be doen at the cold start https://github.com/nrwiersma/ConfigManager/blob/master/src/ConfigManager.cpp#L64

nrwiersma commented 3 years ago

@cgmckeever I am reluctant to as normally nothing is written to EEPROM until the WiFI is setup. It seems simpler to add the config at the same time as the Wifi.