pgm_read_*: cast address to const void* #4619
Conversation
| @@ -112,8 +112,13 @@ static inline uint16_t pgm_read_word_inlined(const void* addr) { | |||
| } | |||
|
|
|||
| // Make sure, that libraries checking existence of this macro are not failing | |||
| #ifdef __PROG_TYPES_COMPAT__ | |||
There was a problem hiding this comment.
(deleted an earlier comment after seeing the reference to __PROG_TYPES_COMPAT__ in the original issue).
Looking at https://www.nongnu.org/avr-libc/user-manual/group__avr__pgmspace.html, __PROG_TYPES_COMPAT__ is used for different purpose (to enable some typedefs) than used here. Therefore suggest using a different name.
There was a problem hiding this comment.
Since the esp8266 pgmspace.h always creates all the deprecated AVR pgmspace types that this define enables in the AVR version, this name seems ok for this usage to me. i.e. from a user perspective setting this define ensures you get the deprecated progmem types as well as the old behavior that was present when those types existed.
i.e. it provides full AVR compatibility including the old types.
I guess technically to be fully compatible with the AVR version the deprecated AVR pgmspace types should also be removed in the esp8266 version if the define is not set but currently the deprecated types always exist in the esp8266 version - which is a difference but is likely to be pretty harmless.
There was a problem hiding this comment.
@bperrybap I guess you made it through, merge is coming.
There was a problem hiding this comment.
I've been following along. Everything seems reasonable for very close compatibility.
It may inadvertently trip up some existing esp8266 users that were depending on the deprecated progmem types, but overall it seems great.
The only last potential remaining item for complete AVR pgmspace.h emulation is the location of the header file itself since with avr-gcc pgmspace.h is in a subdirectory named "avr" since some Arduino code include
#include <avr/pgmspace.h>
On my test/development system I have 80 Arduino libraries installed.
about 20 of those 80 include <avr/pgmspace.h>
Here is grep search for them on my system:
(several may have other esp8266 issues, but still there is significant percentage of libraries using this location)
ffft/ffft.h:#include <avr/pgmspace.h>
glcd/glcd.h:#include <avr/pgmspace.h>
I2C_graphical_LCD_display/I2C_graphical_LCD_display.h:#include <avr/pgmspace.h>
LCD12864/LCD12864.h:#include <avr/pgmspace.h>
Newliquidcrystal_1.3.5/LCD.h:#include <avr/pgmspace.h>
PCF8574_I2C_LCD/PCF8574_I2C_LCD.h: #include <avr/pgmspace.h>
PCF8574_I2C_LCD/PCF8574_I2C_LCD.h: #include <pgmspace.h>
RGB-matrix-Panel/gamma.h:#include <avr/pgmspace.h>
SdFat/SdBaseFile.h:#include <avr/pgmspace.h>
TTS/english.h:#include <avr/pgmspace.h>
WaveHC/WaveUtil.h:#include <avr/pgmspace.h>
Adafruit_GFX_Library/Adafruit_GFX.cpp: #include <avr/pgmspace.h>
Adafruit_GFX_Library/Adafruit_GFX.cpp: #include <pgmspace.h>
Adafruit_GFX.sav/Adafruit_GFX.cpp: #include <avr/pgmspace.h>
Adafruit-MCP23017/Adafruit_MCP23017.cpp:#include <avr/pgmspace.h>
Adafruit_SSD1306/Adafruit_SSD1306.cpp:#include <avr/pgmspace.h>
DHT22/DHT22.cpp:#include <avr/pgmspace.h>
glcd/glcd.cpp:#include <avr/pgmspace.h>
glcd/gText.cpp:#include <avr/pgmspace.h>
LCD12864/LCD12864.cpp:#include <avr/pgmspace.h>
MD_MAX72xx/MD_MAX72xx_font.cpp:#include <avr/pgmspace.h>
RGB-matrix-Panel/RGBmatrixPanel.cpp: "#include <avr/pgmspace.h>\n\n"
RTClib/RTClib.cpp: #include <avr/pgmspace.h>
Time/DateStrings.cpp:#include <avr/pgmspace.h>
This one may be a bit more tricky since I've seen that a few library authors have already gone in and modified their code with ifdefs for the different location on esp8266.
With newer Arduino IDEs, pgmspace.h does not have to be included at all but, it still has to be included to support older Arduino IDEs.
What esp8266 could do is add a new header file avr/pgmspace.h that simply includes the real <pgmspace.h> header file so it works either way.
There was a problem hiding this comment.
well, you already have included in the merged PR :-)
There was a problem hiding this comment.
Oops. I see that now. Sorry I missed that. I only saw the diffs in the header file itself.
Very complete - very nice.
| @@ -39,6 +41,8 @@ typedef uint16_t prog_uint16_t; | |||
| typedef int32_t prog_int32_t; | |||
| typedef uint32_t prog_uint32_t; | |||
|
|
|||
| #endif // defined(__PROG_TYPES_COMPAT__) | |||
There was a problem hiding this comment.
@igrr now that the should-have-been-deprecated typedefs are using this avr macro, is it OK to keep it for pgm_read_* type cast too ?
fix #4572