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

Fix pgm_read_ptr(): 'const void*' is not a pointer-to-object type #864

Merged
merged 1 commit into from
Sep 6, 2022

Conversation

bblanchon
Copy link
Contributor

@bblanchon bblanchon commented Aug 23, 2022

Calling this macro caused the following error:

'const void*' is not a pointer-to-object type

Similar macros, like pgm_read_word() and pgm_read_byte() are also incorrect; I can open a second PR for them if you need.

Fixes bblanchon/ArduinoJson#1790

Calling this macro caused the following error: 'const void*' is not a pointer-to-object type
@bblanchon bblanchon changed the title Fix pgm_read_ptr() (fixes bblanchon/ArduinoJson#1790) Fix pgm_read_ptr(): 'const void*' is not a pointer-to-object type Aug 23, 2022
@2bndy5
Copy link
Member

2bndy5 commented Aug 23, 2022

We had a report a while ago about pgm_read_ptr being incorrectly defined for the esp8266 core, but it seemed (at the time) to be limited to the Arduino online IDE.

IMO, arduino/ArduinoCore-API#118 should have been considered a breaking change and require a major version bump. Now, you're retrofitting libs and cores to satisfy that change... Usually there's a grace period for that, but I'm not sure how the Arduino team could have notified all maintainers of third party libs & cores. You're doing the Arduino world a huge favor with these PRs. I just fear some will fall through the cracks.

The changes here are specific to Linux drivers and the Pico SDK. I'll have to do some extensive hardware testing because of this. The CI is passing, but that doesn't say much for using RF24::printDetails()

Copy link
Member

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

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

This tested well on a ESP32-S2 and RP2040 board, but I had to re-define printf_P to make use of the RF24::printDetails() because all of our calls to pgm_read_ptr() are used to get interpreted debugging values from c-str constants in *print*Details(). The following should work the same for ESP8266 because it looks like esspressif just copied the code print.h/cpp from ESP8266 core into the ESP32 core.

    #if defined(ARDUINO_ARCH_ESP8266) || defined(ESP32) || (defined(ARDUINO_ARCH_RP2040) && !defined(ARDUINO_ARCH_MBED))
        #include <pgmspace.h>
        #define PRIPSTR "%s"
        #ifndef pgm_read_ptr
            #define pgm_read_ptr(p) (*(void* const*)(p))
        #endif
        // Serial.printf() is no longer defined in the unifying Arduino/ArduinoCore-API repo
        // Serial.printf() is defined if using the arduino-esp32/8266/pico repo
        #undef printf_P // needed for ESP32 core
        #define printf_P Serial.printf

I've also tested the changes to the SPIDEV, RPi drivers on Linux, and there were no problems.

I still have to test the MRAA & wiringPi (on an old RPi OS) Linux drivers, and the PicoSDK.

#define pgm_read_ptr(p) (*(p))
#define pgm_read_ptr(p) (*(void* const*)(p))
Copy link
Member

Choose a reason for hiding this comment

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

Technically this is not used. Maybe at some point in the past it was needed, but the latest versions of the ESP* cores have their own definitions

  • ESP8266 uses pgmspace.h defined in the arduino15\packages\esp8266\tools\xtensa-lx106-elf-gcc\3.0.4-gcc10.3-1757bed\xtensa-lx106-elf\include\sys\
    However, the definition doesn't match the definition in ArduinoCore-API without explicitly defining
    #define PGM_READ_UNALIGNED 0
    Otherwise it uses the old definition seen here.
  • ESP32 uses pgmspace.h defined in Arduino15\packages\esp32\hardware\esp32\2.0.4\cores\esp32\ which matches the ArduinoCore-API definition.

#define pgm_read_ptr(p) (*(p))
#define pgm_read_ptr(p) (*(void* const*)(p))
Copy link
Member

Choose a reason for hiding this comment

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

This is need to satisfy the changes in arduino/ArduinoCore-API#118

I suspect that this is the problem shown in bblanchon/ArduinoJson#1790, but the line/column numbers don't match.

Ultimately, this should affect any board wrapping the mbed framework (which is most of the newer boards that Arduino has produced).

@bblanchon
Copy link
Contributor Author

The best would still be to remove all these macros.
After all, it's up to the Arduino cores to define them, not libraries.
For example, ArduinoJson emulates the pgmspace.h header without defining any macro.

@2bndy5
Copy link
Member

2bndy5 commented Sep 3, 2022

This isn't just an Arduino library though.

@2bndy5 2bndy5 self-assigned this Sep 6, 2022
@2bndy5
Copy link
Member

2bndy5 commented Sep 6, 2022

MRAA still working. I'm going to test the Pico SDK and then probably merge this.

Copy link
Member

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

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

MRAA, wiringPi and PicoSDK still function as expected.

@2bndy5 2bndy5 merged commit 0bb5af4 into nRF24:master Sep 6, 2022
@bblanchon bblanchon deleted the pgm_read_ptr branch September 6, 2022 06:31
2bndy5 added a commit that referenced this pull request Sep 6, 2022
@2bndy5 2bndy5 mentioned this pull request Sep 6, 2022
6 tasks
TMRh20 pushed a commit that referenced this pull request Sep 18, 2022
* make other pgm_read_* macros complaint with unified API

Corresponding to #864, this completes compliance with the unified ArduinoCore-API about 

- `pgm_read_word()`
- `pgm_read_byte()`

see https://github.com/arduino/ArduinoCore-API/blob/e26862e453c1234e1c23506d1839bfa68999d911/api/deprecated-avr-comp/avr/pgmspace.h#L102-L103

* add some boards to the Arduino & PIO CI

* run clang-format on changed files


fix param name

* install arduino-pico core in arduino CI

install cpp-linter from PyPI

keep installing AVR, megaAVR, MBED, SAM, & SAMD platforms in addition to arduino-pico
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

Successfully merging this pull request may close these issues.

error: 'const void*' is not a pointer-to-object type
2 participants