64K problems: pins_arduino.h _PGM arrays pushed out of range of pgm_read_byte()

[WestfW]

A simple program (say, Blink) that includes more than 64k of pgmspace data (for MEGA, MEGA2560, MEGA ADK, various 1284 boards) will fail to work correctly.
One of the reasons is that Arduino.h includes macro definitions like:
#define digitalPinToPort(P) ( pgm_read_byte( digital_pin_to_port_PGM + (P) ) )
Due to link order, the digital_pin_to_port_PGM[] array will be pushed AFTER the explicitly defined pgmspace variables, and it will no longer be readable by pgm_read_byte.

Using pgm_read_byte_far() seems like overkill.
It turns out that apparently gcc has a similar problem with pgmspace variables that IT uses, because the default linker map includes two entries for progmem data:

    /* For data that needs to reside in the lower 64k of progmem.  */
    *(.progmem.gcc*)
    *(.progmem*)

This means that MEGA and etc can be fixed by a relative simple patch to their pins_arduino.h file, putting the pin tables in section .progmem.gcc.arduinocore instead of the normal .progmem (as per the attached diff file)

[WestfW]

(Um. Can I not attach source and diff files to an issue?)

[matthijskooijman]

Probably not - I guess you could open a pullrequest, or include the diff in a comment (indent it by for spacs or put ``` before and after it to make it display as code).

As for the fix you suggest, I'm wondering why it works. AFAIU, any new sections introduced have to be manually listed in the linker script? Though perhaps due to -fdata-sections each variable already gets its own section and they are auto-joined? If so, I believe that this fix works because "arduinocore" sorts early?

[WestfW]

The linker script already has *(progmem.gcc*) in the appropriate place. The section names from -fdata-sections take the form of ".mainsection.variablename", so almost all of the linker script sections are wildcarded. (Hmm. I wonder why the wildcards didn't come out in my first post. I'm pretty sure they were in the COPY...)

[WestfW]

Grr. Removed them again. cursed markdown language!
(edited to fix?)

[matthijskooijman]

It seems that github/markdown is interpreting your asterisks as meaning italic text. You can probably escape them with *, or quite with `.

Thanks for clarifying, I can see why your fix works. I'm wondering if "arduinocore" is the best approach though - it sorts low, but if a user declares a big "aa" PROGMEM variable, things still break. I'm not sure what the rules for section names are, but if they're the same as variable names (start with a latter, contain letters, numbers and underscores IIRC), something like .progmem.gcc.A00_arduinocore might be good?

[WestfW]

--- pins_arduino.h  2014/08/04 06:42:23 1.1
+++ pins_arduino.h  2014/08/04 06:43:51
@@ -84,8 +84,9 @@
                                 0 ) ) ) ) ) )

 #ifdef ARDUINO_MAIN
+#define MYPROGMEM  __attribute__((section(".progmem.gcc.arduinocore")))

-const uint16_t PROGMEM port_to_mode_PGM[] = {
+const uint16_t MYPROGMEM port_to_mode_PGM[] = {
    NOT_A_PORT,
    (uint16_t) &DDRA,
    (uint16_t) &DDRB,
@@ -101,7 +102,7 @@
    (uint16_t) &DDRL,
 };

-const uint16_t PROGMEM port_to_output_PGM[] = {
+const uint16_t MYPROGMEM port_to_output_PGM[] = {
    NOT_A_PORT,
    (uint16_t) &PORTA,
    (uint16_t) &PORTB,
@@ -117,7 +118,7 @@
    (uint16_t) &PORTL,
 };

-const uint16_t PROGMEM port_to_input_PGM[] = {
+const uint16_t MYPROGMEM port_to_input_PGM[] = {
    NOT_A_PIN,
    (uint16_t) &PINA,
    (uint16_t) &PINB,
@@ -133,7 +134,7 @@
    (uint16_t) &PINL,
 };

-const uint8_t PROGMEM digital_pin_to_port_PGM[] = {
+const uint8_t MYPROGMEM digital_pin_to_port_PGM[] = {
    // PORTLIST     
    // -------------------------------------------      
    PE  , // PE 0 ** 0 ** USART0_RX 
@@ -208,7 +209,7 @@
    PK  , // PK 7 ** 69 ** A15  
 };

-const uint8_t PROGMEM digital_pin_to_bit_mask_PGM[] = {
+const uint8_t MYPROGMEM digital_pin_to_bit_mask_PGM[] = {
    // PIN IN PORT      
    // -------------------------------------------      
    _BV( 0 )    , // PE 0 ** 0 ** USART0_RX 
@@ -283,7 +284,7 @@
    _BV( 7 )    , // PK 7 ** 69 ** A15  
 };

-const uint8_t PROGMEM digital_pin_to_timer_PGM[] = {
+const uint8_t MYPROGMEM digital_pin_to_timer_PGM[] = {
    // TIMERS       
    // -------------------------------------------      
    NOT_ON_TIMER    , // PE 0 ** 0 ** USART0_RX 

[WestfW]

I'm pretty sure that the progmem.gcc.* will always sort before the progmem.variablename values created by normal user pgmspace statements. Otherwise it wouldn't work for gcc internals, either.

[matthijskooijman]

Oh, right. I was assuming that variables would get progmem.gcc.variablename, which is apparently wrong. I'm wondering if it's ok to 'invade' the progmem.gcc namespace like this, but I guess it will work and as long as we don't push > 64k of data in it, it should be ok :-)

Not sure I like the "MYPROGMEM" name though. Wouldn't GCCPROGMEM be better? In either case, there should be a fat comment explaining why this is needed and why this works :-)

[oqibidipo]

You must also modify these lines in Arduino.h to make it work

extern const uint16_t PROGMEM port_to_mode_PGM[];
extern const uint16_t PROGMEM port_to_input_PGM[];
extern const uint16_t PROGMEM port_to_output_PGM[];

extern const uint8_t PROGMEM digital_pin_to_port_PGM[];
// extern const uint8_t PROGMEM digital_pin_to_bit_PGM[];
extern const uint8_t PROGMEM digital_pin_to_bit_mask_PGM[];
extern const uint8_t PROGMEM digital_pin_to_timer_PGM[];

[geez0x1]

Today we spent most of the day debugging why our Arduino immediately crashed when increasing data use in PROGMEM, even when that data was not being used. Spent ages trying to find problems with 32-bit pointers, etc. What made matters worse is that at some point disabling optimisations (-O0 instead of -Os) made it partially work, which we tried because the documentation for pgm_get_far_address suggests:

'var' has to be resolved at linking time as an existing symbol, i.e, a simple type variable name, an array name (not an indexed element of the array, if the index is a constant the compiler does not complain but fails to get the address if optimization is enabled), a struct name or a struct field name, a function identifier, a linker defined identifier,...

http://www.nongnu.org/avr-libc/user-manual/group__avr__pgmspace.html#ga8f4a87d8740f570871d7e041750efed3

Turns out the problem was the one mentioned in this thread. Disabling optimisations just resulted in different code which somehow made something work at some point. Very confusing.

The patches by WestfW (needs to be done manually though) and oqibidipo appear to completely solve the problem.

As of this moment these patches are not implemented in the Arduino mainline code. My Debian packages may be slightly outdated(?), but the other machine is running the (most) recent packages with AVR 1.6.18 etc.

Is there any particular reason these patches have not been mainlined? Should we file a pull request?

[information-security]

Hi,

Any update on this? I tested the pull request. It somehow fixed the issue but some strange behaviours came up instead. I am not sure if it was caused by the change or it is from my code!

[geez0x1]

Can you be more specific as to what issues you found?

[information-security]

I have two large arrays as well as a medium array:
extern imagedatatype gImage_img_header[];
extern imagedatatype gImage_img_sc1[];
extern imagedatatype gImage_img_sc2[];
As their names imply, these arrays contain image data. header is shown at top of TFT display, sc1 is shown below the header and sc2 is shown at bottom of the screen. When I reduce arrays' size (By reducing image quality) everything is shown as expected but when I switch to using large arrays considering the fact that I have applied the patch, sc1 and sc2 are fine but header is not shown and a cropped part of sc2 or sc1 (I can't recall which) is shown instead.
Note that without the patch sketch doesn't work at all.
Here is my sketch (excluded image data because they were too large):

#include <UTFT.h>
extern uint8_t SmallFont[];
UTFT myGLCD(HX8357B,38,39,40,41);
#define imagedatatype  unsigned int
extern imagedatatype gImage_img_header[]; // size: 14400 bytes
extern imagedatatype gImage_img_sc1[]; // size: 31200 bytes
extern imagedatatype gImage_img_sc2[]; // size: 31200 bytes

void setup() {
  myGLCD.InitLCD();
  myGLCD.setFont(SmallFont);
  myGLCD.clrScr();
}

void loop() {
  myGLCD.fillScr(255, 255, 255);
  myGLCD.setColor(255, 255, 255);
  myGLCD.drawBitmap(0, 0, 240, 30, gImage_img_header, 2);
  myGLCD.drawBitmap(0, 60, 240, 65, gImage_img_sc1, 2);
  myGLCD.drawBitmap(0, 190, 240, 65, gImage_img_sc2, 2);
  delay(5000);
}