Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Saving config with wrong type wipes EEPROM memory #88

Open
cgmckeever opened this issue Aug 4, 2020 · 3 comments
Open

Saving config with wrong type wipes EEPROM memory #88

cgmckeever opened this issue Aug 4, 2020 · 3 comments

Comments

@cgmckeever
Copy link
Collaborator

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
Copy link
Owner

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
Copy link
Collaborator Author

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants