nrwiersma / ConfigManager

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

Saving config with wrong type wipes EEPROM memory #88

Open cgmckeever opened 4 years ago

cgmckeever commented 4 years ago

Discussed here

When putting the wrong type in a config parameter and saving, the memory allocation gets screwy and gets wiped. Device will no longer have Wifi credentials/etc at next boot.

To recreate

config.someIntParam = random(300); // where the parameter is an int
configManager.save();

** This may be an odd type cast I came across and doesnt apply to other scenarios.

Suggestion

A better setter for parameters via ConfigManager.

This would particularly be useful since setting a string is not straightforward

strncpy(config.deviceName, "fesp-muppet", DEVICENAMELEN);

Update method

Something like:

template <typename T>
void updateParameter(name, T* value) {
    // call to the configParameter->update();
  }

void updateParameter(name, char* value) {
    // call to the configParameter->update();
  }

I had zero luck figuring out how to identify a parameter by name from the std::list<BaseParameter*> and a tripped over the same trap of Arduino not having std::unordered_map at least twice now ;)

Explicit parameter objects for the main project

I also tried having the addParameter call return the ConfigParameter object and the main project code could track/index it. That got complicated, and compiler errors in the type returns/etc

void loop() {

...

ConfigParameter = configManager.addParameter("randomNumber", &config.randomNumber, get);

Doing something bad ...

Finally .. I came up with this monster that exploits ConfigManagers use of JSON for updating ... worked, but felt icky

 template <typename T>
  void updateParameter(const char* name, T value) {
    std::list<BaseParameter*>::iterator it;
    for (it = parameters.begin(); it != parameters.end(); ++it) {
      if ((*it)->getName() == name) {
        DynamicJsonDocument doc(1024);
        JsonObject json = doc.createNestedObject();
        json[name] = value;
        (*it)->fromJson(&json);
        break;
nrwiersma commented 4 years ago

So the first question I have is, how does C++ even allow the assignment. Its a strong type language, I would have thought it would be a compile error.

In any case, if it allows it with direct assignment, it likely will not kick up a fuss if we try coerce the type. I agree that the long -> json -> int will work, but I dont love it as a solution.

I think, should my brain cooperate for long enough, we can tackle this in v3?

cgmckeever commented 4 years ago

In any case, if it allows it with direct assignment, it likely will not kick up a fuss if we try coerce the type. I agree that the long -> json -> int will work, but I dont love it as a solution.

Its literally GROSS

we can tackle this in v3?

There is no pressing need, it was something that I stumbled into not knowing better. A cleaner update would be a nice to have. depending on how you think about approaching it, I feel if its doable and there is bandwidth, outside of v3 would be simpler as there would be less churn in general -- of course, if it is pegged on features of v3, then well -- delaying it would make sense

how does C++ even allow the assignment.

That is a mystery beyond my google-fu

nrwiersma commented 4 years ago

I think I have a partial answer this this conundrum.

https://www.arduino.cc/reference/en/language/variables/data-types/int/

At the bottom here it points to the dangers of overflowing an int. I must wonder if the assignment of long is overflowing the int and affecting other memory. I also think it might be an Arduino thing to allow the assignment, or my mind is stuck in Go mode and this is always allowed in C++.

This is my best guess at the moment.