adafruit / Adafruit_MQTT_Library

Arduino library for MQTT support
MIT License
566 stars 292 forks source link

Fix connectErrorString return type for ESP-32 BSP 2.0.8 #222

Closed brentru closed 1 year ago

brentru commented 1 year ago

This PR fixes return type of connectErrorString due to a change in https://github.com/espressif/arduino-esp32/pull/7941 by adding a preprocessor for ESP32-arch and returning a const char * rather than a __FlashStringHelper to Serial.println().

Related: https://github.com/adafruit/Adafruit_Learning_System_Guides/pull/2483

mrengineer7777 commented 1 year ago

@brentru Hi, it's David from arduino-esp32. I'm a volunteer.

You shouldn't have to make new cases for ESP32. Just change the return type of function connectErrorString to const char *. That should be more universal and may work for other platforms. Worst case you might also have to redefine macro F for ESP32.

ESP32 currently defines F as:

#define FPSTR(pstr_pointer) (reinterpret_cast<const __FlashStringHelper *>(pstr_pointer))
#define F(string_literal) (FPSTR(PSTR(string_literal)))

https://github.com/espressif/arduino-esp32/blob/master/cores/esp32/WString.h

You can override it something like this:

#ifdef ARDUINO_ARCH_ESP32
undef F
#define F(s) (s)
#endif

If you use the undef, you'll probably want it in your .cpp file so it doesn't affect compilation of other libraries.

brentru commented 1 year ago

@mrengineer7777

You shouldn't have to make new cases for ESP32. Just change the return type of function connectErrorString to const char *. That should be more universal and may work for other platforms.

Changing the return type to const char* causes the reverse of the original error to occur on AVR (and other?) platforms, error: cannot convert 'const __FlashStringHelper*' to 'const char*' in return

brentru commented 1 year ago

@mrengineer7777

ESP32 currently defines F as: #define FPSTR(pstr_pointer) (reinterpret_cast<const __FlashStringHelper *>(pstr_pointer)) #define F(string_literal) (FPSTR(PSTR(string_literal)))

This is not true as of https://github.com/espressif/arduino-esp32/pull/7941, it was changed to:

#define FPSTR(pstr_pointer) (pstr_pointer)
#define F(string_literal) (string_literal)

We're finding that the compiler does not seem to allow implicit casting to occur in either direction.

We could override the current macro to use the older one:

#ifdef ARDUINO_ARCH_ESP32
undef FPSTR
undef F
#define FPSTR(pstr_pointer) (reinterpret_cast<const __FlashStringHelper *>(pstr_pointer))
#define F(string_literal) (FPSTR(PSTR(string_literal)))
#endif

But I'm not sure if having divergent #defines between a BSP and library code is the best way to go - I like keeping things in-line.

mrengineer7777 commented 1 year ago

@mrengineer7777

ESP32 currently defines F as: #define FPSTR(pstr_pointer) (reinterpret_cast<const __FlashStringHelper *>(pstr_pointer)) #define F(string_literal) (FPSTR(PSTR(string_literal)))

This is not true as of espressif/arduino-esp32#7941, it was changed to:

#define FPSTR(pstr_pointer) (pstr_pointer)
#define F(string_literal) (string_literal)

We're finding that the compiler does not seem to allow implicit casting to occur in either direction.

We could override the current macro to use the older one:

#ifdef ARDUINO_ARCH_ESP32
undef FPSTR
undef F
#define FPSTR(pstr_pointer) (reinterpret_cast<const __FlashStringHelper *>(pstr_pointer))
#define F(string_literal) (FPSTR(PSTR(string_literal)))
#endif

But I'm not sure if having divergent #defines between a BSP and library code is the best way to go - I like keeping things in-line.

Hmmm. That is messy. That PR is one of the breaking changes that will be discussed in our community meeting next week.