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

PROGMEM/pgm_read cast to "normal" data types #3086

Closed
MichaDit opened this issue Mar 25, 2017 · 23 comments
Closed

PROGMEM/pgm_read cast to "normal" data types #3086

MichaDit opened this issue Mar 25, 2017 · 23 comments

Comments

@MichaDit
Copy link

Using PROGMEM/pgm_read and cast to normal types like byte, char, int, long seems not to work properly.

In my case I tried to port an Arduino-sketch that worked like a charm on UNO and derivates to ESP8266.
It compiles fine, but ESP8266 crashes at some point. In this project there are a bunch of (multidimensional) arrays put to PROGMEM with cast to byte, char, int, long.
There are warning about expanding macros for PROGMEM/pgm_read, wich led me to try if there is a difference in using casts to appropiate xintxx_t instead.
This gives no compiler warnings and sketch works like expected.

Is this behaviour known at this point?

Tested on newest Arduino IDE with ESP12E

@neu-rah
Copy link

neu-rah commented Mar 27, 2017

do you have a cast line example?
PROGMEM macros should be transparent on ESP.

@MichaDit
Copy link
Author

MichaDit commented Mar 27, 2017

Not drilled down to the root, but shows the error on ESP8266 (working fine with UNO):

#define LUT(a) (long)(pgm_read_word(&lut[a]))

const unsigned int lut[] PROGMEM = { //crash on ESP8266, ok on UNO
//const uint16_t lut[] PROGMEM = { //ok
16384, 16381, 16374, 16361, 16344, 16321, 16294, 16261, 16224, 16182, 16135, 16082, 16025, 15964, 15897, 15825, 15749, 15668, 15582, 15491, 15395, 15295, 15190, 15081, 14967, 14848, 14725, 14598, 14466, 14329, 14188, 14043, 13894, 13740, 13582, 13420, 13254, 13084, 12910, 12732, 12550, 12365, 12175, 11982, 11785, 11585, 11381, 11173, 10963, 10748, 10531, 10310, 10086, 9860, 9630, 9397, 9161, 8923, 8682, 8438, 8191, 7943, 7691, 7438, 7182, 6924, 6663, 6401, 6137, 5871, 5603, 5334, 5062, 4790, 4516, 4240, 3963, 3685, 3406, 3126, 2845, 2563, 2280, 1996, 1712, 1427, 1142, 857, 571, 285, 0
};

long SIN(unsigned int angle) {
angle += 90;
if (angle > 450) return LUT(0);
if (angle > 360 && angle < 451) return -LUT(angle-360);
if (angle > 270 && angle < 361) return -LUT(360-angle);
if (angle > 180 && angle < 271) return LUT(angle-180);
return LUT(180-angle);
}

void setup() {
Serial.begin(115200);
Serial.println("Starting...");
}

void loop() {
int i = 0;
String out;
while (i < 91) {
yield();
out = "#";
out += i;
out += ": ";
out += LUT(i);
Serial.println("before sin");
out += SIN(i); //here it crashes
Serial.println("after sin");
Serial.println(out);
i++;
}
}

@bebo-dot-dev
Copy link

I think that it's the attempt to use pgm_read_word in your LUT macro that's causing the crash here on an ESP8266 device because pgm_read_word returns a 2 byte (16-bit) word from the given address and ESP devices are 32-bit devices. This code is probably stomping on memory in use by other things causing the explosion.

Try using pgm_read_dword instead - this returns a 4 byte double word.

@MichaDit
Copy link
Author

MichaDit commented Apr 5, 2017

You are right, the difference on ESP8266 unsigned int is 32bit and uint16_t is 16bit, on ATMega the types are both 16bit.

But besides that, is this a correct behaviour of pgm_read_*?
Should it be a problem in general when reading less bits of a wider datatype - besides loss of bits?

Drilled it down and it behaves really strange - there have to be a lot of things present at the same time to let it crash (compiles without any warnings):

#define LUT(a) (unsigned int)(pgm_read_word(&lut[a])) //needed to crash
//#define LUT(a) (unsigned int)(pgm_read_dword(&lut[a])) //OK


const unsigned int lut[] PROGMEM = { //needed to crash
//const uint16_t lut[] PROGMEM = { //OK
0, 1}; //needed to crash
//0}; //OK


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

void loop() {
  unsigned int i = 0; //needed to crash
  //int i = 0; //OK
  String out;
  while (i < 2) { //needed to crash
  //while (i < 1) { //OK
    //Serial.println(LUT(i++)); //OK
    out = LUT(i++); //CRASH (only if all needed elements are present, otherwise OK)
    Serial.println(out);
  }
}

@bebo-dot-dev
Copy link

To be perfectly honest I've not found a use for the pgm_read_word macro on an ESP device other than to cause a crash :)

That's not to say it's completely useless because it may well work for some scenarios that I'm not aware of..but I can say that I do use PROGMEM for storing an array of application strings in my code and when I was developing this, the only way I managed to get it working successfully was by way of using pgm_read_dword.

I believe this is related to the fact that all memory addresses on an ESP device are 32bit addresses regardless of the type of data that is actually stored at that address - in other words sizeof(char*) == 4 //an ESP8266 pointer is always 4 bytes

@MichaDit
Copy link
Author

MichaDit commented Apr 5, 2017

ESP is only one target in Arduino-world ;-)
On other targets you have to pay attention on RAM- and FLASH-consumption...

But that's not the thing, there is IMO a bug and I thought it is worth to publish it.
A workaround is already given in the first post, needless to point it anymore.
I think a discussion if it should be used is senseless - here is not the right place for this and does not get us any further on solving the bug.

@neu-rah
Copy link

neu-rah commented Apr 5, 2017

its very important if you want to run some existing arduino sketches on ESP

@davisonja
Copy link

davisonja commented Apr 5, 2017 via email

@bebo-dot-dev
Copy link

@igrr
Copy link
Member

igrr commented Apr 6, 2017

doh...
ptrdiff_t __offset = ((uint32_t)__local & 0x00000002); /* word aligned mask */ \

@milkpirate
Copy link

Ok are pgm_read_* (in paticular pgm_read_byte) now working? Because the only thing it produces are bootloops. Am I doing something wrong?!

Test code:

PROGMEM const char table[2][3] = {
  {0x7B,0x60,0x5D},
  {0x7D,0x48,0x3E}
};

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

void loop() {
  byte data = pgm_read_byte(&table[1][2]);
  Serial.println(data, HEX);
  delay(250);
}

Output:

Exception (3):
epc1=0x40201c0e epc2=0x00000000 epc3=0x00000000 excvaddr=0x4022e71d depc=0x00000000

ctx: cont
sp: 3ffef200 end: 3ffef3c0 offset: 01a0

>>>stack>>>
3ffef3a0:  feefeffe feefeffe feefeffe 40202098
3ffef3b0:  feefeffe feefeffe 3ffee3a0 40100114
<<<stack<<<

 ets Jan  8 2013,rst cause:2, boot mode:(3,6)

load 0x4010f000, len 1384, room 16
tail 8
chksum 0x2d
csum 0x2d
v09f0c112
~ld

@MichaDit
Copy link
Author

I hope you know what you are doing: storing 16bit in PROGMEM and read it via pgm_read_byte as 8bit.
Try PROGMEM const uint16_t and for 16bit reads: char data = pgm_read_word(&table[1][2]);.
char data is not on all targets 16bit, portability is restricted.
I do not know if there is a solution for the problem right now @igrr ?

@milkpirate
Copy link

milkpirate commented Apr 15, 2017

I modified my sketch:

PROGMEM const uint8_t table[2][3] = {
  {0x7B,0x60,0x5D},
  {0x7D,0x48,0x3E}
};

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

void loop() {
  uint8_t data = pgm_read_byte(&table[1][1]);
  Serial.println(data, HEX);
  delay(250);
}

Still no luck... Am I forecd to store and read my uint8_t's as uint16_t's?

@MichaDit
Copy link
Author

This seams to be another problem...
Putting the delay in front of Serial.println it works!?:

PROGMEM const char table[2][3] = {
  {0x7B,0x60,0x5D},
  {0x7D,0x48,0x3E}
};

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

void loop() {
  byte data = pgm_read_byte(&table[1][2]);
  delay(250);
  Serial.println(data, HEX);
}

@milkpirate
Copy link

Swapping Serial.println(data, HEX); and delay(250); worked. But:

  • Why?
  • How do I know where to place the delay?
  • What commands do cause that behaviour?
  • How do have to adjust my code to make it work?
  • ...finally WHY?!

@MichaDit
Copy link
Author

I suggest you to open a new issue for that.
This seems to be a bug...

@creeg2
Copy link

creeg2 commented Apr 19, 2017

@milkpirate I'm seeing the exact same error you're seeing. I encountered it in an encryption library that utilizes program memory to store a large array. Adding the delay after the pgm_read_byte call fixes the issue (but obviously makes the overall program run much slower. It seems that even a 1ms delay is sufficient to make the pgm_read_byte call successful. Have you made any progress finding a solution?

@milkpirate
Copy link

@creeg2 no. But your hint is a good start I guess. Have you tried an us delay? I guess it's related to the amount of time it takes to retrieve data from flash. Since the programming paradigm is different from the normal one. It relies on callbacks when an action is done instead of sequetial instructions. The Serial.print might access the data before read is done.

@igrr
Copy link
Member

igrr commented Apr 19, 2017

Maybe you could try changing the wrong mask in pgm_read_word macro,

ptrdiff_t __offset = ((uint32_t)__local & 0x00000001); /* word aligned mask */

(currently it is & 0x00000002 which doesn't make any sense)

@creeg2
Copy link

creeg2 commented Apr 20, 2017

@igrr the offset calculation is correct, for both word and byte read functions. The ESP8266 requires flash reads to be aligned in 32bit chunks. The offset calculation makes sure that the requested read occurs on the boundry of the closest 32bit chunk. It basically rounds down to the start of the nearest 32bit chunk. The read_byte function has mask 0x00...03 to grab the last 2 bits of the address. The read_word function has mask 0x00...02 to grab just the 2nd to last bit since words are 16 bits long. An offset error also would not account for the delay work around behavior.

@igrr
Copy link
Member

igrr commented Apr 20, 2017

You're right! Sorry, i was thinking we were trying to read an uint16_t variable which happens to be unaligned in flash.

I have tried running code fragments posted in this issue. All tests on a nodemcu v1 board, not that this should matter... Running git version.

  • latest fragment by @Fuchks with pgm_read_byte works even without delay
  • both fragments by @milkpirate work as posted
  • earlier code by @Fuchks doesn't work, because the array is declared as unsigned int. This makes it uint16_t on UNO but uint32_t on ESP. With the array type changed to uint16_t everything works fine.

Whether the original issue (access to unsigned int array using pgm_read_word) has to be fixed somehow is debatable, because in general it is not possible to make all the code which assumes exact size of int to be portable. If anything, I prefer the program to crash rather than continuing to work with loss of bytes, as this immediately indicates that there is a problem (and if you use exception decoder or GDBStub, even points to the place where it happens).

The issue raised in the last few comments (where pgm_read_byte) does not work is something i can not reproduce here. This may very well be because we are compiling with slightly different version of the core, which causes slightly different placement of data, which leads to some (?) alignment issue which doesn't happen in my setup. I'll poke around and try to make it crash somehow.

@milkpirate
Copy link

milkpirate commented Apr 20, 2017

Have a look here: #3140 ...strange behavior...
@igrr what core are you using? How can I get it? My compiles are done by 1.8.0 and 1.8.2.

@igrr
Copy link
Member

igrr commented Apr 20, 2017

I'm using git version a few commits older than master. I think the issue you are seeing is not related to this one. You are seeing a PhysicalLoadStoreError, load address is properly aligned, but the CPU fails to fetch data from the cache. I will check a few things and will reply in that new issue.

For this one, I think it can be closed.

@igrr igrr closed this as completed Apr 20, 2017
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

7 participants