Skip to content

pgm_read_*: cast address to const void* #4619

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Apr 10, 2018
Merged

pgm_read_*: cast address to const void* #4619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
53a576b
pgm_read_*: cast address to const void*
d-a-v Apr 7, 2018
a055e4a
casting pgm_read_+* only with __PROG_TYPES_COMPAT__, also add <avr/pg…
d-a-v Apr 8, 2018
e3ff5ea
Merge branch 'master' into 4572
devyte Apr 9, 2018
9c9e74c
restrict usage of prog_* deprecated typedefs to when __PROG_TYPES_COM…
d-a-v Apr 9, 2018
259b63c
Merge branch 'master' into 4572
d-a-v Apr 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cores/esp8266/avr/pgmspace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
#include "../pgmspace.h"
9 changes: 9 additions & 0 deletions cores/esp8266/pgmspace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ extern "C" {

#define _SFR_BYTE(n) (n)

#ifdef __PROG_TYPES_COMPAT__

typedef void prog_void;
typedef char prog_char;
typedef unsigned char prog_uchar;
Expand All @@ -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__)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, should be okay.


#define SIZE_IRRELEVANT 0x7fffffff

// memchr_P and memrchr_P are not implemented due to danger in its use, and
Expand Down Expand Up @@ -112,8 +116,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__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bperrybap I guess you made it through, merge is coming.

Copy link

@bperrybap bperrybap Apr 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, you already have included in the merged PR :-)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. I see that now. Sorry I missed that. I only saw the diffs in the header file itself.
Very complete - very nice.

#define pgm_read_byte(addr) pgm_read_byte_inlined((const void*)(addr))
#define pgm_read_word(addr) pgm_read_word_inlined((const void*)(addr))
#else
#define pgm_read_byte(addr) pgm_read_byte_inlined(addr)
#define pgm_read_word(addr) pgm_read_word_inlined(addr)
#endif

#else //__ets__
#define pgm_read_byte(addr) (*reinterpret_cast<const uint8_t*>(addr))
Expand Down