Struct not saving to EEProm Correctly

Hey Guys,

I've been struggling with this for a few hours now, I'm writing a program that needs to have a serial command interface, and accept and save a few different values to the EEProm.

The Temprature is saving correctly, but I'm not sure whats happening with the phone numbers.

I believe it may be because the phone numbers are a character array?

#include <SoftwareSerial.h> 
#include <SerialCommand.h>
#include <EEPROM.h>
#include "EEPROMAnything.h"

#define arduinoLED 13   // Arduino LED on board
#define EEPROMStart 32 // Start write and read for EEPROM

#define TX 1
#define RX 0
SoftwareSerial SoftSerial = SoftwareSerial(RX,TX);     // The SoftwareSerial Object
SerialCommand SCmd(SoftSerial);   // The demo SerialCommand object, using the SoftwareSerial Constructor

struct config_t
{
    int alertHigh;
    int alertLow;
    char* phone1;
    char* phone2;
} configuration;

void setup()
{ 
  //Read our last state from EEPROM
  EEPROM_readAnything(EEPROMStart, configuration);
  pinMode(arduinoLED,OUTPUT);      // Configure the onboard LED for output
  digitalWrite(arduinoLED,LOW);    // default to LED off

  Serial.begin(9600); 
  SoftSerial.begin(9600); 

  // Setup callbacks for SerialCommand commands   
  SCmd.addCommand("temp",setTemp);  // Converts two arguments to integers and echos them back 
  SCmd.addCommand("phone",setPhone);  // Converts two arguments to integers and echos them back 
  SCmd.addCommand("config",showConfig);  // Converts two arguments to integers and echos them back 
  SCmd.addCommand("default",clearEeprom);  // Converts two arguments to integers and echos them back 
  SCmd.addDefaultHandler(unrecognized);  // Handler for command that isn't matched  (says "What?") 
  
  showConfig();
    
  Serial.println("Ready"); 
 }

void loop()
{  
  SCmd.readSerial();     // We don't do much, just process serial commands
}


void setTemp()    
{ 
  char *arg; 

  arg = SCmd.next(); 
  if (arg != NULL) 
  {
    configuration.alertHigh=atoi(arg);    // Converts a char string to an integer
    Serial.print("HIGH: "); 
    Serial.println(configuration.alertHigh); 
  } 
  else {
    Serial.println("TEMP <HIGH> <LOW>"); 
    return;
  }

  arg = SCmd.next(); 
  if (arg != NULL) 
  {
    configuration.alertLow=atol(arg); 
    Serial.print("LOW: "); 
    Serial.println(configuration.alertLow); 
  } 
  else {
    Serial.println("TEMP <HIGH> <LOW>");
    return;
  }
  EEPROM_writeAnything(EEPROMStart, configuration);
}

void setPhone()    
{ 
  char *arg; 

  arg = SCmd.next(); 
  if (arg != NULL) 
  {
    configuration.phone1=arg;
    Serial.println(configuration.phone1); 
  } 
  else {
    Serial.println("phone <number1> <number2>"); 
    return;
  }

  arg = SCmd.next(); 
  if (arg != NULL) 
  {
    configuration.phone2=arg;
    Serial.println(configuration.phone2); 
  } 
  else {
    Serial.println("phone <number1> <number2>"); 
    return;
  }
  
  EEPROM_writeAnything(EEPROMStart, configuration);
}

void showConfig(){
  Serial.println("Reading Config from EEPROM:"); 
  EEPROM_readAnything(EEPROMStart, configuration);
  Serial.println("Current Config:"); 
  Serial.print("    Alert High: "); 
  Serial.println(configuration.alertHigh); 
  Serial.print("    Alert Low: "); 
  Serial.println(configuration.alertLow);
  Serial.print("    Phone 1: "); 
  Serial.println(configuration.phone1); 
  Serial.print("    Phone 2: "); 
  Serial.println(configuration.phone2);
}

void clearEeprom(){
   for (int i = 0 ; i < EEPROM.length() ; i++) {
    EEPROM.write(i, 0);
  }
  Serial.print("eepromCleared");
}


// This gets set as the default handler, and gets called when no other command matches. 
void unrecognized()
{
  Serial.println("Unrecognized"); 
}

In such I usually make a union of a structure and a byte array. Then dump the byte array to the EEPROM one byte at a time. The same for reading it back. If you are using an ESP8266 then there is a different behaviour because EEPROM is actually mapped to flash memory.

E.G.

struct {
    char eepromLayourVersion[5];  // magicString
    unsigned int runNumber ; // incremented on reboot
    bool networkMode ; // false = standalone.

    //screen calibration
    float xScale ;
    float xShift ;
    float yScale ;
    float yShift ;
    //wlan
    char ssid[32];
    char psk[64];
    char remoteHost[32] ;
    // number translation rules
    // char intl[2][6] ;  // eg intl[0] = "00" intl[1] = "+"  (applied first)
    // char local[2][6] ; // eg local[0] = "0" local[1] = "+41" (applied second)
    char ruleIntlFrom[6] ;  // "00"
    char ruleIntlTo[6] ;    // "+"
    char ruleLocalFrom[6] ; // "0"
    char ruleLocalTo[6] ;   // "+41"
    char phpRetrievePath[32] ;
    char phpStorePath[32] ;
    
  } ;
  byte store[EEPROMSIZE] ;
} config_t ;

extern config_t config ;


....
....

void ICACHE_FLASH_ATTR eepromFetch() {
  for ( int i = 0 ; i < EEPROMSIZE ; i++ ) {
    config.store[i] =  EEPROM.read(i);  // one byte at a time
  }
}


void ICACHE_FLASH_ATTR eepromDump() {
  for ( int i = 0 ; i < EEPROMSIZE ; i++ ) {
    EEPROM.put(i, config.store[i] );  // one byte at a time
  }
  EEPROM.commit();
}

When, for example, you want to store "Hello" to EEPROM, you have to store the 'H', the 'e', the 'l', another 'l' and so on.

Your struct has pointers to the strings. They are copied from a pointer on the stack "arg", and it was filled with the return value of "SCmd.next();", which returns a pointer to a buffer inside the SerialCommand SCmd object.

When you read the struct, you get pointers that point to something.

You need to store the actual string data with an array.
Do you know the maximum length of those strings ?

6v6gt:
In such I usually make a union of a structure and a byte array. Then dump the byte array to the EEPROM one byte at a time. The same for reading it back.

the library handles that; you can save an object to EEPROM in one command. No need to muck about with all that.

@Koepel
Thanks, that makes sense. Yes I do, they will be Australian phone numbers, so 0408xxxxxx (10 didgits)

MoonMan89:
@Koepel
Thanks, that makes sense. Yes I do, they will be Australian phone numbers, so 0408621398 (10 didgits)

right, so your struct would look like this, then:

struct config_t
{
    int alertHigh;
    int alertLow;
    char phone1[11];
    char phone2[11];
} configuration;

since you need to allow an extra byte for the null terminator of the phone number strings.

I was at that conclusion before, but I got compliation errors because of the type difference between the next() function and the char[11].

I assume I need to copy it from one to the other somehow, strncopy?

BulldogLowell:
the library handles that; you can save an object to EEPROM in one command. No need to muck about with all that.

Well, I have learned something there. It appears to be the put() and get() methods:

I guess I was caught out by the documentation: https://www.arduino.cc/en/Tutorial/EEPROMRead where the get method is oddly described.

Looks like the OP was using something else ( #include "EEPROMAnything.h" ) which appears to do the same thing. Maybe his explanation is similar to mine.

6v6gt:
Looks like the OP was using something else ( #include "EEPROMAnything.h" ) which appears to do the same thing. Maybe his explanation is similar to mine.

#include <EEPROM.h>
#include <Arduino.h>  // for type definitions

template <class T> int EEPROM_writeAnything(int ee, const T& value)
{
   const byte* p = (const byte*)(const void*)&value;
   unsigned int i;
   for (i = 0; i < sizeof(value); i++)
         EEPROM.write(ee++, *p++);
   return i;
}

template <class T> int EEPROM_readAnything(int ee, T& value)
{
   byte* p = (byte*)(void*)&value;
   unsigned int i;
   for (i = 0; i < sizeof(value); i++)
         *p++ = EEPROM.read(ee++);
   return i;
}

MoonMan89:
I was at that conclusion before, but I got compliation errors because of the type difference between the next() function and the char[11].

I assume I need to copy it from one to the other somehow, strncopy?

#include <SerialCommand.h>

I'm not familiar with that library... what is the source?

@MoonMan89, yes, copy it.
There was something about strncpy, I think it was unsafe/evil/weird/outdated. It is not doing what you think it is doing.

The strlcpy is preferred.
The strlcpy adds the zero terminator for you.
strlcpy( configuration.phone1, arg, sizeof( configuration.phone1));

I agree with BulldogLowell, the EEPROMAnything is not bad, but you can use the new EEPROM.put and EEPROM.get with the struct.

Koepel:
. . . but you can use the new EEPROM.put and EEPROM.get with the struct.

Ah. Are these commands new ? If so, that could well be (a) why I hadn't thought of using them and (b) why the OP had his home made solution.

void setPhone()    
{ 
  char arg[11];
  char phone1[11];
  char phone2[11];

  strncpy(phone1, SCmd.next(), 11);
  strncpy(phone2, SCmd.next(), 11);
  
 if((phone1 == NULL) || (phone2 == NULL)){
    Serial.println("phone <phone1> <phone2>");
    return;
  }
  
  strncpy(configuration.phone1, phone1, 11);
  strncpy(configuration.phone2, phone2, 11);
  
  EEPROM.put(EEPROMStart, configuration);
  showConfig();
}

Much gooder.
I was not aware of those commands :slight_smile:

As for the SerialCommand library:

So, now the only issue I have is varifying that there are two arguments given. All my if statements won't work, I'm assuming because the copy is filling with blanks?

strncpy(phone1, SCmd.next(), 11);

side note: if the two buffers are of the same size, strcpy() is perfectly safe...

MoonMan89:
So, now the only issue I have is varifying that there are two arguments given. All my if statements won't work, I'm assuming because the copy is filling with blanks?

by definition, the "phone" command means there are numbers following, yes? you can test the return to make sure they contain a certain number of digits... isdigit()
but here:

if((phone1 == NULL) || (phone2 == NULL)){

I think I see what you are trying to do there but you're doing it in a strange fashion. phone1 and phone2 are char arrays, their names respolve to a poiniter to the first element, so yeah if the first element is a null return...

but I'd

if(strlen(phone1) == 0 and strlen(phone2) == 0)
{

Now that I've gotten over that bit of the project, I've moved on to including the FONA library, and am having this issue:

Sketch uses 9,444 bytes (29%) of program storage space. Maximum is 32,256 bytes.
Global variables use 2,041 bytes (99%) of dynamic memory, leaving 7 bytes for local variables. Maximum is 2,048 bytes.
Low memory available, stability problems may occur.

I'm assuming that's why things are crashing. Where do I start to get that down? I don't have any extra libraries included :frowning:

#include <Adafruit_FONA.h>
#include <SoftwareSerial.h> 
#include <SerialCommand.h>
#include <EEPROM.h>


#define arduinoLED 13   // Arduino LED on board
#define EEPROMStart 513 // Start write and read for EEPROM

// FONA Settings
#define FONA_TX 2
#define FONA_RX 3
#define FONA_RST 9

// this is a large buffer for replies
char replybuffer[255];

#define TX 1
#define RX 0
SoftwareSerial SoftSerial = SoftwareSerial(RX,TX);     // The SoftwareSerial Object
SerialCommand SCmd(SoftSerial);   // The demo SerialCommand object, using the SoftwareSerial Constructor

SoftwareSerial fonaSS = SoftwareSerial(FONA_TX,FONA_RX);
SoftwareSerial *fonaSerial = &fonaSS;

Adafruit_FONA_3G fona = Adafruit_FONA_3G(FONA_RST);

uint8_t readline(char *buff, uint8_t maxbuff, uint16_t timeout = 0);

uint8_t type;

struct config_t
{
    int alertHigh;
    int alertLow;
    char phone1[11];
    char phone2[11];
} configuration;

void setup()
{ 
  //Read our last state from EEPROM
  EEPROM.get(EEPROMStart, configuration);
  pinMode(arduinoLED,OUTPUT);      // Configure the onboard LED for output
  digitalWrite(arduinoLED,LOW);    // default to LED off

  Serial.begin(9600); 
  SoftSerial.begin(9600); 
  
  Serial.println("Starting 3GAlarmPanel-NDM.guru V0001");
  Serial.println("Initializing....(May take 3 seconds)");

  fonaSerial->begin(4800);
  if (! fona.begin(*fonaSerial)) {
    Serial.println(F("Couldn't find FONA"));
    while (1);
  }
  type = fona.type();
  Serial.println(F("FONA is OK"));
  Serial.print(F("Found "));
  switch (type) {
    case FONA800L:
      Serial.println(F("FONA 800L")); break;
    case FONA800H:
      Serial.println(F("FONA 800H")); break;
    case FONA808_V1:
      Serial.println(F("FONA 808 (v1)")); break;
    case FONA808_V2:
      Serial.println(F("FONA 808 (v2)")); break;
    case FONA3G_A:
      Serial.println(F("FONA 3G (American)")); break;
    case FONA3G_E:
      Serial.println(F("FONA 3G (European)")); break;
    default: 
      Serial.println(F("???")); break;
  }

  // Print module IMEI number.
  char imei[15] = {0}; // MUST use a 16 character buffer for IMEI!
  uint8_t imeiLen = fona.getIMEI(imei);
  if (imeiLen > 0) {
    Serial.print("Module IMEI: "); Serial.println(imei);
  }

  // Setup callbacks for SerialCommand commands   
  SCmd.addCommand("temp",setTemp);  // Converts two arguments to integers and echos them back 
  SCmd.addCommand("phone",setPhone);  // Converts two arguments to integers and echos them back 
  SCmd.addCommand("config",showConfig);  // Converts two arguments to integers and echos them back 
  SCmd.addCommand("default",clearEeprom);  // Converts two arguments to integers and echos them back 
  SCmd.addDefaultHandler(unrecognized);  // Handler for command that isn't matched  (says "What?") 
  
  showConfig();
    
  Serial.println("Ready"); 
 }

void loop()
{  
  SCmd.readSerial();     // We don't do much, just process serial commands
}


void setTemp()    
{ 
  char *arg; 

  arg = SCmd.next(); 
  if (arg != NULL) 
  {
    configuration.alertHigh=atoi(arg);    // Converts a char string to an integer
    Serial.print("HIGH: "); 
    Serial.println(configuration.alertHigh); 
  } 
  else {
    Serial.println("TEMP <HIGH> <LOW>"); 
    return;
  }

  arg = SCmd.next(); 
  if (arg != NULL) 
  {
    configuration.alertLow=atol(arg); 
    Serial.print("LOW: "); 
    Serial.println(configuration.alertLow); 
  } 
  else {
    Serial.println("TEMP <HIGH> <LOW>");
    return;
  }
  EEPROM.put(EEPROMStart, configuration);
}

void setPhone()    
{ 
  char arg[11];
  char phone1[11];
  char phone2[11];

  strlcpy(phone1, SCmd.next(),11);
  strlcpy(phone2, SCmd.next(),11);
  
 if(strlen(phone1) == 0 or strlen(phone2) == 0){
    Serial.println("phone <phone1> <phone2>");
    return;
  }
  
  strlcpy(configuration.phone1, phone1, 11);
  strlcpy(configuration.phone2, phone2, 11);
  
  EEPROM.put(EEPROMStart, configuration);

  Serial.println("Phone numbers saved");
  showConfig();
}

void showConfig(){
  Serial.println("Reading Config from EEPROM:"); 
  EEPROM.get(EEPROMStart, configuration);
  Serial.println("Current Config:"); 
  Serial.print("    Alert High: "); 
  Serial.println(configuration.alertHigh); 
  Serial.print("    Alert Low: "); 
  Serial.println(configuration.alertLow);
  Serial.print("    Phone 1: "); 
  Serial.println(configuration.phone1); 
  Serial.print("    Phone 2: "); 
  Serial.println(configuration.phone2);
}

void clearEeprom(){
   for (int i = 0 ; i < EEPROM.length() ; i++) {
    EEPROM.write(i, 0);
  }
  Serial.print("eepromCleared");
}


// This gets set as the default handler, and gets called when no other command matches. 
void unrecognized()
{
  Serial.println("Commands:"); 
  Serial.println("  phone <number1> <number2>       # SMS Capable, mobiles"); 
  Serial.println("  temp <high> <low>               # 2 didgits"); 
  Serial.println("  config                          # Shows config"); 
  Serial.println("  default                         # Clears EEProm"); 
}

Use the F macro for all your strings:

Serial.print("    Alert High: "); 
etc.

Just discovered that one :smiley: Thanks. I've managed to add alot more, but I believe I can get rid of one of these serial connections. there are only really a need for 2, one to the PC and one to the FONA. However, I seem to have 3?

#include <Adafruit_FONA.h>
#include <SoftwareSerial.h> 
#include <SerialCommand.h>
#include <EEPROM.h>


#define arduinoLED 13   // Arduino LED on board
#define EEPROMStart 513 // Start write and read for EEPROM

// FONA Settings
#define FONA_TX 2
#define FONA_RX 3
#define FONA_RST 9

// this is a large buffer for replies
char replybuffer[64];

#define TX 1
#define RX 0
SoftwareSerial SoftSerial = SoftwareSerial(RX,TX);     // The SoftwareSerial Object
SerialCommand SCmd(SoftSerial);   // The demo SerialCommand object, using the SoftwareSerial Constructor

SoftwareSerial fonaSS = SoftwareSerial(FONA_TX,FONA_RX);
SoftwareSerial *fonaSerial = &fonaSS;

Adafruit_FONA_3G fona = Adafruit_FONA_3G(FONA_RST);

uint8_t readline(char *buff, uint8_t maxbuff, uint16_t timeout = 0);

struct config_t
{
    int alertHigh;
    int alertLow;
    char phone1[11];
    char phone2[11];
} configuration;

void setup()
{   
  //Read our last state from EEPROM
  EEPROM.get(EEPROMStart, configuration);
  pinMode(arduinoLED,OUTPUT);      // Configure the onboard LED for output
  digitalWrite(arduinoLED,LOW);    // default to LED off

  Serial.begin(9600); 
  SoftSerial.begin(9600); 
  
  Serial.println(F("Starting 3GAlarmPanel-NDM.guru V0001"));
  Serial.println(F("Initializing....(May take 3 seconds)"));

  fonaSerial->begin(4800);
  if (! fona.begin(*fonaSerial)) {
    Serial.println(F("Couldn't find FONA"));
    while (1);
  }
  
  // Setup callbacks for SerialCommand commands   
  SCmd.addCommand("temp",setTemp);  // Converts two arguments to integers and echos them back 
  SCmd.addCommand("phone",setPhone);  // Converts two arguments to integers and echos them back 
  SCmd.addCommand("config",showConfig);  // Converts two arguments to integers and echos them back 
  SCmd.addCommand("default",clearEeprom);  // Converts two arguments to integers and echos them back 
  SCmd.addDefaultHandler(unrecognized);  // Handler for command that isn't matched  (says "What?") 
  
  showConfig();
    
  Serial.println(F("Ready")); 
 }

void loop()
{  
  SCmd.readSerial();     // We don't do much, just process serial commands
}


void setTemp()    
{ 
  char *arg; 

  arg = SCmd.next(); 
  if (arg != NULL) 
  {
    configuration.alertHigh=atoi(arg);    // Converts a char string to an integer
    Serial.print(F("HIGH: ")); 
    Serial.println(configuration.alertHigh); 
  } 
  else {
    Serial.println(F("TEMP <HIGH> <LOW>")); 
    return;
  }

  arg = SCmd.next(); 
  if (arg != NULL) 
  {
    configuration.alertLow=atol(arg); 
    Serial.print(F("LOW: ")); 
    Serial.println(configuration.alertLow); 
  } 
  else {
    Serial.println(F("TEMP <HIGH> <LOW>"));
    return;
  }
  EEPROM.put(EEPROMStart, configuration);
}

void setPhone()    
{ 
  char arg[11];
  char phone1[11];
  char phone2[11];

  strlcpy(phone1, SCmd.next(),11);
  strlcpy(phone2, SCmd.next(),11);
  
 if(strlen(phone1) == 0 or strlen(phone2) == 0){
    Serial.println(F("phone <phone1> <phone2>"));
    return;
  }
  
  strlcpy(configuration.phone1, phone1, 11);
  strlcpy(configuration.phone2, phone2, 11);
  
  EEPROM.put(EEPROMStart, configuration);

  Serial.println(F("Phone numbers saved"));
  showConfig();
}

void showConfig(){
  Serial.println(F("Reading Config from EEPROM:")); 
  EEPROM.get(EEPROMStart, configuration);
  Serial.println(F("Current Config:")); 
  Serial.print(F("    Alert High: ")); 
  Serial.println(configuration.alertHigh); 
  Serial.print(F("    Alert Low: ")); 
  Serial.println(configuration.alertLow);
  Serial.print(F("    Phone 1: ")); 
  Serial.println(configuration.phone1); 
  Serial.print(F("    Phone 2: ")); 
  Serial.println(configuration.phone2);
}

void clearEeprom(){
   for (int i = 0 ; i < EEPROM.length() ; i++) {
    EEPROM.write(i, 0);
  }
  Serial.print(F("eepromCleared"));
}

void imei(){
  // Print module IMEI number.
  char imei[15] = {0}; // MUST use a 16 character buffer for IMEI!
  uint8_t imeiLen = fona.getIMEI(imei);
  if (imeiLen > 0) {
    Serial.print(F("Module IMEI: ")); Serial.println(imei);
  }
}

// This gets set as the default handler, and gets called when no other command matches. 
void unrecognized()
{
  Serial.println(F("Commands:")); 
  Serial.println(F("  phone <number1> <number2>       # SMS Capable, mobiles")); 
  Serial.println(F("  temp <high> <low>               # 2 didgits")); 
  Serial.println(F("  config                          # Shows config")); 
  Serial.println(F("  default                         # Clears EEProm")); 
}

These 2 use the same serial object so I don't think there is any duplication :

SoftwareSerial SoftSerial = SoftwareSerial(RX,TX);     // The SoftwareSerial Object
SerialCommand SCmd(SoftSerial);   // The demo SerialCommand object, using the SoftwareSerial Constructor

This looks odd, especially if you are experiencing crashes:

  char imei[15] = {0}; // MUST use a 16 character buffer for IMEI!
  uint8_t imeiLen = fona.getIMEI(imei);

Maybe make it 16 and then set the last char to 0 (null) by imei[15] = 0;

hmmmm....

SoftwareSerial SoftSerial = SoftwareSerial(RX,TX);     // The SoftwareSerial Object
SerialCommand SCmd(SoftSerial);   // The demo SerialCommand object, using the SoftwareSerial Constructor

SoftwareSerial fonaSS = SoftwareSerial(FONA_TX,FONA_RX);
SoftwareSerial *fonaSerial = &fonaSS;

The library has the following known limitations:

If using multiple software serial ports, only one can receive data at a time.

from here

it is hard to understand why you are creating two instances of SoftwareSerial and why you think you need so many pointers to it!