Go Down

Topic: PROGMEM issues, again (Read 255 times) previous topic - next topic

dougp

In a quest to back away from a 60% ram usage message I finally investigated PROGMEM. After a lot of stumbling and assistance I managed to get some strings into PROGMEM and bring them out for display on my 16x2 LCD. The template to create them is copied from Nick Gammon's forum.

My next target is a list of error messages. The codes defining each type of error are fixed in the VL6180X TOF sensor. As can be seen below, these codes are not sequential. To relate error code to message text I created a 2D array (of structures) using the received error code as the argument for a search loop. It partly works -  the loop will retrieve, and I can print to the LCD, the correct error code. The lcd.print() however, shows nothing. My latest attempt, shown below, is lifted from the AVR user manual.

Creation of the data and a short bit to test the function call

Code: [Select]
//  LASER ERROR MESSAGE TEXT
//
typedef struct  {         // create structure
  int8_t errorCode;        // 6180 error code
  char* errorText[22];    // error description
} laserErr_t;

const char LasErrSt0[] PROGMEM = "System Error";
const char LasErrSt1[] PROGMEM = "ECE Failure";
const char LasErrSt2[] PROGMEM = "Convergence fail";
const char LasErrSt3[] PROGMEM = "Range Ignore";
const char LasErrSt4[] PROGMEM = "SNR Error";
const char LasErrSt5[] PROGMEM = "Raw Underflow";
const char LasErrSt6[] PROGMEM = "Raw Overflow";
const char LasErrSt7[] PROGMEM = "Range Underflow";
const char LasErrSt8[] PROGMEM = "Range Overflow";

const laserErr_t laserMsg[2][9] PROGMEM = {
  1, LasErrSt0,
  6, LasErrSt1,
  7, LasErrSt2,
  8, LasErrSt3,
  11, LasErrSt4,
  12, LasErrSt5,
  13, LasErrSt6,
  14, LasErrSt7,
  15, LasErrSt8,
  };
byte noOfErrMsgs = (sizeof (laserMsg) / sizeof (laserErr_t)) / 2;
/*
    END VARIABLE DECLARATIONS  --  SETUP ROUTINE
*/
void setup() {
  Serial.begin(9600); // for debug only
  lcd.begin(16, 2);// open comms. with 1602 display unit
  lcd.createChar(0, ramAtTop);
  lcd.createChar(1, ramAtLoad);
  lcd.createChar(2, ramAtBottom);
  lcd.createChar(3, rotateCCW);
  lcd.createChar(4, rotateCW);
  // diagnostic_display();
  status = 13;
  displayLaserError(status);
  while (1);


Function to print error messages


Code: [Select]
// variable i will hold index to code and text

// print buffer is declared as: char lcdPrintBufr[22];

void displayLaserError(int errNum) {
  for ( int i = 0; i < noOfErrMsgs; i++) { // nine error messges currently
    if (errNum == pgm_read_byte(&laserMsg[0][i])) {
      memcpy_P(&lcdPrintBufr, (&laserMsg[1][i]), sizeof lcdPrintBufr);
      //(&(mydata[i][j]));
      break;
    }
    lcd.clear();
    lcd.print("Laser Err no. ");
    lcd.print(errNum);
    lcd.setCursor(0, 01);
   //  memcpy_P(&lcdPrintBufr, &laserMsg[1][1], sizeof lcdPrintBufr);
     lcd.print(lcdPrintBufr);
  }
}


No doubt it's something blindingly simple but I'm out of ideas. Throw me a bone?




Budvar10

I didn't study whole code, just after brief look: you have &lcdPrintBufr in memcpy but the pointer to the lcdPrintBufr[22] array is just lcdPrintBufr or &lcdPrintBufr[0].
Another problem is using the memcpy instead of strcpy. The progmem strings are defined with variable length, it means the string ends with the NULL character and it is not defined behind. You have to align buffered string separately.

Arduino clone with ATmega1284P   http://forum.arduino.cc/index.php?topic=277260.0

johnwasser

I'm pretty sure you don't want 22 character pointers in your structure.  One is sufficient.
Code: [Select]
typedef struct  {         // create structure
  int8_t errorCode;        // 6180 error code
  char* errorText;    // error description
} laserErr_t;


And why have two structures per message?
Code: [Select]
const laserErr_t laserMsg[2][9] PROGMEM = {
  1, LasErrSt0,
  6, LasErrSt1,
  7, LasErrSt2,
  8, LasErrSt3,
  11, LasErrSt4,
  12, LasErrSt5,
  13, LasErrSt6,
  14, LasErrSt7,
  15, LasErrSt8,
  };


Shouldn't this be more like:
Code: [Select]
const laserErr_t laserMsg[9] PROGMEM = {
  {1, LasErrSt0},
  {6, LasErrSt1},
  {7, LasErrSt2},
  {8, LasErrSt3},
  {11, LasErrSt4},
  {12, LasErrSt5},
  {13, LasErrSt6},
  {14, LasErrSt7},
  {15, LasErrSt8}
  };
Send Bitcoin tips to: 1G2qoGwMRXx8az71DVP1E81jShxtbSh5Hp

dougp

I didn't study whole code, just after brief look: you have &lcdPrintBufr in memcpy but the pointer to the lcdPrintBufr[22] array is just lcdPrintBufr or &lcdPrintBufr[0].
Another problem is using the memcpy instead of strcpy. The progmem strings are defined with variable length, it means the string ends with the NULL character and it is not defined behind. You have to align buffered string separately.
This is part of the already working code:

Code: [Select]
typedef struct {
  char stationtext[22];
} stationlist;
const stationlist turretPosnTextP[] PROGMEM = {"Crimp",
                                               "Bullet Seat",
                                               "Powder Drop",
                                               "Prime/Deprime",
                                               "Raise to Unload ",
                                               "Lower to Unload " //final space erases turret char
                                              };
int8_t turretPosnNoParms_s = sizeof(turretPosnTextP) / 2;
//
// and this correctly accesses and displays the above strings
//
memcpy_P (&lcdPrintBufr, &turretPosnTextP[textAndParms[screenNow].parmVal], sizeof lcdPrintBufr);
lcd.print(lcdPrintBufr);


I don't understand how this is different from the non-working stuff.

Also, this line:

Code: [Select]
strcpy_P(&lcdPrintBufr, (&laserMsg[1][i]), sizeof lcdPrintBufr);

Generates the error code:

laser_error:5: error: cannot convert 'char (*)[22]' to 'char*' for argument '1' to 'char* strcpy_P(char*, const char*)'

       strcpy_P(&lcdPrintBufr, (&laserMsg[1]), sizeof lcdPrintBufr);

                                                                     ^


johnwasser

You can .print() direct from PROGMEM.  Just cast the PROGMEM address as (__FlashStringHelper*)

Code: [Select]
void displayLaserError(int errNum) {
  char * message;  // PROGMEM address of message
  
  lcd.clear();
  lcd.print("Laser Err no. ");
  lcd.print(errNum);

  for ( int i = 0;  i < noOfErrMsgs;  i++) { // nine error messges currently
    if (errNum == pgm_read_byte(&laserMsg[i].errorCode)) {
      // Found the message that matches the error code
      message = pgm_read_word(&laserMsg[i].errorText);  // Get PROGMEM address from PROGMEM
      lcd.setCursor(0, 01);
      lcd.print( (__FlashStringHelper*) message);
      break;  // Don't bother looking for another match
    }
  }
}
Send Bitcoin tips to: 1G2qoGwMRXx8az71DVP1E81jShxtbSh5Hp

dougp

And why have two structures per message?
Because I'm easily confused.  :)  Come to think of it, that would be a good screen name.

The code is revised per your advice and will compile but still does not work.

Code: [Select]
//  LASER ERROR MESSAGE TEXT
//
typedef struct  {         // create structure
  int8_t errorCode;        // 6180 error code
  char* errorText[];    // error description
} laserErr_t;

const laserErr_t laserMsg[] PROGMEM = {
  1, "System Error",
  6, "ECE Failure",
  7, "Convergence fail",
  8, "Range Ignore",
  11, "SNR Error",
  12, "Raw Underflow",
  13, "Raw Overflow",
  14, "Range Underflow",
  15, "Range Overflow"
};
byte noOfErrMsgs = (sizeof (laserMsg) / sizeof (laserErr_t)) / 2;
/*
    END VARIABLE DECLARATIONS  --  SETUP ROUTINE
*/
void setup() {
  Serial.begin(9600); // for debug only
  lcd.begin(16, 2);// open comms. with 1602 display unit
  lcd.createChar(0, ramAtTop);
  lcd.createChar(1, ramAtLoad);
  lcd.createChar(2, ramAtBottom);
  lcd.createChar(3, rotateCCW);
  lcd.createChar(4, rotateCW);
  // diagnostic_display();
  status = 11;
  displayLaserError(status);
  while (1);


Now the search fails. With the added print statements the number 0-8 are printed on the serial monitor.
What I believe are the contents of the referenced memory locations are printed after the index.

Code: [Select]
void displayLaserError(int errNum) {
  char * message;  // PROGMEM address of message
 
  lcd.clear();
  lcd.print("Laser Err no. ");
  lcd.print(errNum);

  for ( int i = 0;  i < noOfErrMsgs;  i++) { // nine error messges currently
    Serial.print(i);
    Serial.print(" ");
    Serial.println(pgm_read_byte(&laserMsg[i].errorCode));
    if (errNum == pgm_read_byte(&laserMsg[i].errorCode)) {
      // Found the message that matches the error code
      message = pgm_read_word(&laserMsg[i].errorText);  // Get PROGMEM address from PROGMEM
      lcd.setCursor(0, 01);
      lcd.print( (__FlashStringHelper*) message);
      //iprint = i;
      lcd.setCursor(14, 0);
      lcd.print(i);
      break;  // Don't bother looking for another match
    }
  }



Additionally, including a number in the declaration of laserMsg yields this error message - too many initializers for 'const laserErr_t [9]'

Coding Badly

#6
Sep 13, 2017, 08:37 pm Last Edit: Sep 13, 2017, 08:37 pm by Coding Badly
Code: [Select]

  char* errorText[];    // error description

  - ? - 


dougp

 - ? -  
Brackets removed. Code now compiles and loop executes as desired. A serial monitor display from the loop with status assigned a value of '15' appears as follows:

index/value from table

0   1
1    6
2   7
3   8
4   11
5   12
6   13
7   14
8   15

Code: [Select]
//  LASER ERROR MESSAGE TEXT
//
typedef struct  {         // create structure
  int8_t errorCode;        // 6180 error code
  char* errorText;    // error description
} laserErr_t;

const laserErr_t laserMsg[] PROGMEM = {
  1, "System Error",
  6, "ECE Failure",
  7, "Convergence fail",
  8, "Range Ignore",
  11, "SNR Error",
  12, "Raw Underflow",
  13, "Raw Overflow",
  14, "Range Underflow",
  15, "Range Overflow"
};
byte noOfErrMsgs = (sizeof (laserMsg) / sizeof (laserErr_t));
/*
    END VARIABLE DECLARATIONS  --  SETUP ROUTINE
*/
void setup() {
  Serial.begin(9600); // for debug only
  lcd.begin(16, 2);// open comms. with 1602 display unit
  lcd.createChar(0, ramAtTop);
  lcd.createChar(1, ramAtLoad);
  lcd.createChar(2, ramAtBottom);
  lcd.createChar(3, rotateCCW);
  lcd.createChar(4, rotateCW);
  Serial.println();
  status = 15;
  displayLaserError(status);
  while (1);


Still, the result of the function...

Code: [Select]
void displayLaserError(int errNum) {
  char * message;  // PROGMEM address of message
 
  lcd.clear();
  lcd.print("Laser Err ");
  lcd.print(errNum);

  for ( int i = 0;  i < noOfErrMsgs;  i++) { // nine error messges currently
    Serial.print(i);
    Serial.print(" ");
    Serial.println(pgm_read_byte(&laserMsg[i].errorCode));
    if (errNum == pgm_read_byte(&laserMsg[i].errorCode)) {
      // Found the message that matches the error code
      message = pgm_read_word(&laserMsg[i].errorText);  // Get PROGMEM address from PROGMEM
      lcd.setCursor(0, 01);
      lcd.print( (__FlashStringHelper*) message);
      lcd.setCursor(14, 0);
      lcd.print(i);
      break;  // Don't bother looking for another match
    }
  }

}


is:

Laser Err 15 8
16 chrs of   !<#_&c@ 8j)-S`

johnwasser

The version below, with the changes I recommended, compiles fine for me after adding the stuff that your snippets didn't include.  I have not yet tried to run it.
Code: [Select]
// include the library code:
#include <LiquidCrystal.h>

// these constants won't change.  But you can change the size of
// your LCD using them:
const int numRows = 2;
const int numCols = 16;

// initialize the library with the numbers of the interface pins
LiquidCrystal lcd(12, 11, 5, 4, 3, 2);

const byte noOfErrMsgs = 9;

const char LasErrSt0[] PROGMEM = "System Error";
const char LasErrSt1[] PROGMEM = "ECE Failure";
const char LasErrSt2[] PROGMEM = "Convergence fail";
const char LasErrSt3[] PROGMEM = "Range Ignore";
const char LasErrSt4[] PROGMEM = "SNR Error";
const char LasErrSt5[] PROGMEM = "Raw Underflow";
const char LasErrSt6[] PROGMEM = "Raw Overflow";
const char LasErrSt7[] PROGMEM = "Range Underflow";
const char LasErrSt8[] PROGMEM = "Range Overflow";

typedef struct  {         // create structure
  int8_t errorCode;        // 6180 error code
  const char* errorText;    // error description
} laserErr_t;

const laserErr_t laserMsg[noOfErrMsgs] PROGMEM = {
  {1, LasErrSt0},
  {6, LasErrSt1},
  {7, LasErrSt2},
  {8, LasErrSt3},
  {11, LasErrSt4},
  {12, LasErrSt5},
  {13, LasErrSt6},
  {14, LasErrSt7},
  {15, LasErrSt8}
};

void displayLaserError(int errNum) {
  char * message;  // PROGMEM address of message

  lcd.clear();
  lcd.print("Laser Err no. ");
  lcd.print(errNum);

  for ( int i = 0;  i < noOfErrMsgs;  i++) { // nine error messges currently
    if (errNum == pgm_read_byte(&laserMsg[i].errorCode)) {
      // Found the message that matches the error code
      message = pgm_read_word(&laserMsg[i].errorText);  // Get PROGMEM address from PROGMEM
      lcd.setCursor(0, 01);
      lcd.print( (__FlashStringHelper*) message);
      break;  // Don't bother looking for another match
    }
  }
}

void setup() {}
void loop() {}
Send Bitcoin tips to: 1G2qoGwMRXx8az71DVP1E81jShxtbSh5Hp

dougp

#9
Sep 14, 2017, 03:28 am Last Edit: Sep 14, 2017, 03:34 am by dougp
The version below, with the changes I recommended, compiles fine for me after adding the stuff that your snippets didn't include.  I have not yet tried to run it.
Thank you! The code in post #9 works as desired.

For future reference, it seems my main error was not including curly braces around the array elements. Is this correct?

Code: [Select]
const laserErr_t laserMsg[noOfErrMsgs] PROGMEM = {
  {1, LasErrSt0},
  {6, LasErrSt1},
  {7, LasErrSt2},
  {8, LasErrSt3},
  {11, LasErrSt4},
  {12, LasErrSt5},
  {13, LasErrSt6},
  {14, LasErrSt7},
  {15, LasErrSt8}
};


Also, ram usage is now down to 41%. That should leave sufficient headroom from here on out.

johnwasser

For future reference, it seems my main error was not including curly braces around the array elements. Is this correct?
Your THREE main errors were:

  • Not putting brackets around the initializers for each structure.
  • Declaring a two by N array to hold N structures.
  • Declaring 'errorText' as an array of 22 character pointers.
Send Bitcoin tips to: 1G2qoGwMRXx8az71DVP1E81jShxtbSh5Hp

dougp

Bollixed it up but good, eh?

Again, many thanks.

johnwasser

Bollixed it up but good, eh?
Even with decades of experience I have made similar messes when trying to figure out why a construct that made sense to me did not make sense to the compiler.  We're always learning.
Send Bitcoin tips to: 1G2qoGwMRXx8az71DVP1E81jShxtbSh5Hp

johnwasser

If you don't mind all of the messages taking up the space of the longest message you can avoid having the string constants separate from the structure initialization.  (The compiler WILL generate a warning if an initializer string that is larger than the character array it is initializing.)


Code: [Select]
// include the library code:
#include <LiquidCrystal.h>

// these constants won't change.  But you can change the size of
// your LCD using them:
const int numRows = 2;
const int numCols = 16;

// initialize the library with the numbers of the interface pins
LiquidCrystal lcd(12, 11, 5, 4, 3, 2);

const byte noOfErrMsgs = 9;

typedef struct  {         // create structure
  int8_t errorCode;        // 6180 error code
  const char errorText[17];    // error description
} laserErr_t;

const laserErr_t laserMsg[noOfErrMsgs] PROGMEM = {
  {1, "System Error"},
  {6, "ECE Failure"},
  {7, "Convergence fail"},
  {8, "Range Ignore"},
  {11, "SNR Error"},
  {12, "Raw Underflow"},
  {13, "Raw Overflow"},
  {14, "Range Underflow"},
  {15, "Range Overflow"}
};

void displayLaserError(int errNum) {
  lcd.clear();
  lcd.print("Laser Err no. ");
  lcd.print(errNum);

  for ( int i = 0;  i < noOfErrMsgs;  i++) { // nine error messges currently
    if (errNum == pgm_read_byte(&laserMsg[i].errorCode)) {
      // Found the message that matches the error code
      lcd.setCursor(0, 01);
      lcd.print( (__FlashStringHelper*) &laserMsg[i].errorText);
      break;  // Don't bother looking for another match
    }
  }
}

void setup() {}
void loop() {}
Send Bitcoin tips to: 1G2qoGwMRXx8az71DVP1E81jShxtbSh5Hp

dougp

That's what I was trying to achieve in my initial attempts. A few extra bytes of PROGMEM don't bother me at this point. There's 32K and I've only used a third of that. I just like the look of things, with the error numbers aligned with their respective messages as literals as opposed to text contained within yet another variable.

Saved for later digestion.


Go Up