CPU hangs when using pgm_read_word with an odd address

[Jueff]

when running this code the CPU freezes after writing "word@1" to the serial port.

const PROGMEM unsigned char Config[] = { 0,1,2,3,4,5,6,7,8};
const unsigned char* ptr;


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

void loop() {
  delay(2500);
  ptr = &Config[0];
  Serial.print("word@0:");
  Serial.println(pgm_read_word(ptr));
  ptr = &Config[1];
  Serial.print("word@1:");
  Serial.println(pgm_read_word(ptr));
}

when changing
#define pgm_read_word(addr) (*(const unsigned short *)(addr))
to
#define pgm_read_word(addr) (pgm_read_byte(addr)+(pgm_read_byte(addr+1)<<8))

program works fine.

Maybe this is also an issue with

#define pgm_read_dword(addr)
#define pgm_read_float(addr)
#define pgm_read_ptr(addr) 

[Bodmer]

The problem is that a byte array has been created but then a 16 bit word access is being requested with an 8 bit pointer that may not be aligned to 16 bit boundaries. This is bad programming practice for FLASH which frequently requires type aligned data access pointers. So the data should always be accessed with the pointer type that it is stored in. So the bytes should be concatenated in the program.

The sketch below will work as the address of the first byte is forced to be on a 16 bit 32 bit word boundary. Edit: aligned parameter is in bytes (now 32 bits to play safe).

const PROGMEM unsigned char Config[] __attribute__ ((aligned (4))) = { 0,1,2,3,4,5,6,7,8};
const unsigned char* ptr;


void setup() {
  Serial.begin(115200);
  while (!Serial);
  uint16_t c = 0;
}

void loop() {
  ptr = &Config[0];
  Serial.print("word@0:");
  Serial.println(pgm_read_word(ptr));
  ptr = &Config[2];
  Serial.print("word@1:");
  Serial.println(pgm_read_word(ptr));
  delay(2000);
}

Note that &Config[1], being a byte address is now clearly not word aligned and thus will crash the Pico, hence the change to &Config[2]. Hence only even indexes are legal. Note also the word at &Config[8] will give an indeterminate value as there are an odd number of bytes in the array.

[Jueff]

Hi Bodmer,
thanks for your feedback, but that doesn't help forward.

concerning "This is bad programming practice for FLASH which frequently"
This is just a sample code - in real live code various dynamically created configuration structs are stored in an flash area. So the alignment of data can only be byte-wise (e.g ISO/IEC 7816 data structures)

concerning "only even indexes are legal"
Reading the documentation of the pgm_read_... functions I couldn't find any note that the given address must be aligned depending on the resulting data type.

concerning "will work as the address of the first byte is forced to..."
out of compatibility reason there must not be any "force" to change existing source code. The PGM_READ.... functions work with various platforms, I'm running it on ATMega328, ATMega4809 and ESP32, none of the platform has any problem with unaligned addresses.

concerning "...word aligned and thus will crash the Pico..."
Having an existing source code now running on Pico this issue is very hard to find. I know the problems with memory alignements, but even for me it wasn't clear that the PGM_READ functions cause this CPU lock-up. You need to first have a look on the platform source code.

concerning "Note also the word at &Config[8] will give an indeterminate"
Yes, I know, but in real live the code parsing the configuration structure will take care of it.

I would expect a compatible implementation of these functions.

[Bodmer]

I can understand the frustration, maybe this helps since the ESP8266 has exactly the same problem. Essentially AVR processors read the FLASH as 8 bits (instruction size) and 32 bit processors pull out 32bits at a time (this is a hardware level behaviour) even if the FLASH itself if byte wide.

Unfortunately #define macros do not do type checking so the compiler does not spot the problem.

There are ways around this of course, e.g. throw an error or (for strict legacy compatibility) assemble words/dwords byte by byte byte and accept the undesirable performance penalty.

[Jueff]

I'm not frustrated, I know how to,workaround this issue :-)
But a lot of hobby programmer may get frustrated caused by incompatibility and unexpected hang ups.

[Bodmer]

OK, but bear in mind the workaround then breaks the reading of 16 bit words. The results will be different according to the pointer type used (8, 16, 32 etc), so a new problem is created that will confuse anyone who tries to use that approach.

Type casting the pointer will fix this:

#define pgm_read_word(addr) (pgm_read_byte((const uint8_t*)(addr))+(pgm_read_byte((const uint8_t*)(addr)+1)<<8))

The Arduino environment does hide some problems from programmers and thus creates pitfalls later unfortunately.

I have added brackets around the (addr) since that may be a muliple parameter expression and hence the (const uint8_t*) type cast wouldotherwise only apply to the first parameter in that expression.

Unfortunately there is atill another problem introduced, the result will depend on the endianess (byte order) used by the processor...

[earlephilhower]

@Jueff we ran into this same issue on the ESP8266 Arduino core I help maintain, too. Thanks @Bodmer (also, FWIW the aligned pragma takes bytes, not bits, so your original example would align the array to a 16-byte boundary, not just a 16-bit one)

Basically on the ARM M0+ you can't read a value off of its natural alignment. shorts need to be on 2-byte boundaries and ints need to be on 4-byte boundaries. If not, the CPU doesn't know how to handle it and you get a hardware exception (not C++ exception).

Let me pull in the macros like you suggested. It's a little more complicated than your example because sometimes G++ is too smart for its own good and will do things like convert your nicely written 2-individual-byte-reads-and-shifts into a read-short. But we already solved it on the 8266 so I'll just steal that. :)

[Bodmer]

Noted. It certainly would be good to not just "hang" and need a hard reset.

[Bodmer]

I did not realise this had been fixed for the ESP8266 as discussed here, a good solution. I had avoided the problem as it existed for so long.

This also explains the performance drop I noticed at one point due to the default unaligned access, but just accepted it at the time. I can see there is an aligned access macro so I can now use that in future to get the performance back. Thanks, glad this came up!

Have corrected the aligned pragma, my brain was on a coffee break there!