Use of macros for pin remapping breaks user code and libraries

[tttapa]

Board

Arduino Nano ESP32

Version

latest master (checkout manually)

Description

Redefining Arduino API functions like digitalWrite as macros is problematic: It breaks user code and libraries because the macros do not honor C++ namespace rules.

The code in question was added to cores/esp32/io_pin_remap.h in 7d26b07 in order to add support for the Arduino Nano ESP32.

Background: I'm the author of the Control Surface, which has member functions with names like digitalWrite: https://github.com/tttapa/Control-Surface/blob/7fbff1e994dea4fc249da2b88d4df05f62f7fec9/src/AH/Hardware/ExtendedInputOutput/ExtendedIOElement.hpp#L121

Sketch

class MyClass {
  void digitalWrite() { /* something */ }
};

namespace custom {
  void digitalWrite() { /* something */ }
}

void setup() {}
void loop() {}

Debug Message

sketch_jan21b.ino:2:21: error: macro "digitalWrite" requires 2 arguments, but only 1 given
    2 |   void digitalWrite() { /* something */ }
      |                     ^
In file included from ~/.arduino15/packages/esp32/hardware/esp32/3.0.0-alpha3/cores/esp32/Arduino.h:239,
                 from /tmp/arduino/sketches/5B2320DC96E62936CEB584BE31053B9E/sketch/sketch_jan21b.ino.cpp:1:
~/.arduino15/packages/esp32/hardware/esp32/3.0.0-alpha3/cores/esp32/io_pin_remap.h:46: note: macro "digitalWrite" defined here
   46 | #define digitalWrite(pin, val)                      digitalWrite(digitalPinToGPIONumber(pin), val)
      |

I have checked existing issues, online documentation and the Troubleshooting Guide

• I confirm I have checked existing issues, online documentation and Troubleshooting guide.

[me-no-dev]

@pillo79 could you please have a look?

[pillo79]

Hello @tttapa and thanks for raising this. Your analysis is totally correct, this is an unfortunate side effect of the work that has been added last year to support the Arduino Nano ESP32 and the Arduino Nano pin numbering scheme. You can read more about the reasons behind this here if you are interested.

To fix this there are two ways:

The "zero friction" way

If your API is a perfect clone of the Arduino API, and you wish your library to have zero friction with the Nano ESP32 folks, you should work with these macros and make sure they apply exactly once in the whole call stack (most probably, when users directly call one of your reimplemented Arduino APIs from their sketch).

To fix the specific issue, you can wrap the symbol name in parenthesis, such as:

class MyClass {
  void (digitalWrite)(int pin, PinStatus status) { /* something */ }
};

to avoid the macro expansion to apply in these spots (this is safe and standard C/C++).
You must also make sure that any other place in your code where you are calling the Arduino API implementation is not translated as well, so even the actual API call in your C files will have to look like (digitalWrite)(...stuff...).

With this attention, the code will be able to use either pin numbering choice transparently.

The "easy out" way

Unfortunately, there are obsolete and/or unmaintained but working libraries and we had to offer a compatible solution. By forcing users to use the standard ESP32 pin naming (by choosing "Tools" -> "Pin Numbering" -> "By GPIO number (legacy)" in the Arduino IDE), no macros will be defined and your library will work out-of-the-box with no source changes. You can even detect if the core is using the pin remapping adding the following assertion early in your includes:

#if defined(BOARD_HAS_PIN_REMAP) && !defined(BOARD_USES_HW_GPIO_NUMBERS)
#error This library is not compatible with pin remapping. Please disable it by choosing "Tools" -> "Pin Numbering" -> "By GPIO number (legacy)" in the Arduino IDE.
#endif

[tttapa]

Hi, thanks for the detailed reply!

If your API is a perfect clone of the Arduino API

Unfortunately, this is not the case (the pin numbers users pass to the functions refer to pins on IO expanders etc., not Arduino pin numbers). So I'm afraid I'll have to

[force] users to use the standard ESP32 pin naming

Have any alternatives been considered? In my opinion, macro-based text replacement is not the right tool for the job, especially not with commonly used names that do not follow the standard macro naming conventions.

The following seemingly innocuous #ifndef could also cause hard-to-debug ODR violations:

arduino-esp32/cores/esp32/io_pin_remap.h

Lines 12 to 13 in 7d26b07

	// Apply pin remapping to API only when building libraries and user sketch
	#ifndef ARDUINO_CORE_BUILD

^{https://en.cppreference.com/w/cpp/language/definition}
There can be more than one definition in a program of each of the following: class type, enumeration type, inline function, inline variable, templated entity [...], as long as all of the following is true:

• each definition appears in a different translation unit
• each definition consists of the same sequence of tokens (typically, appears in the same header file)
• [...]

An alternative could be to use inline functions instead of macros:

#if defined(BOARD_HAS_PIN_REMAP) && !defined(BOARD_USES_HW_GPIO_NUMBERS)
#define REMAP_PIN_TO_GPIO(p) (digitalPinToGPIONumber((p)))
#else
#define REMAP_PIN_TO_GPIO(p) (p)
#endif

inline void ARDUINO_ISR_ATTR digitalWrite(uint8_t pin, uint8_t val) {
    return __digitalWrite(REMAP_PIN_TO_GPIO(pin), val);
}

#undef REMAP_PIN_TO_GPIO

[pillo79]

Thank you for the insight! Definitely open to suggestions, it is a thorny field so help is very much appreciated.

Most of the API functions in their body call each other freely, so the issue is applying the pin renaming everywhere but exactly once. Also, ideally, the change has to be compact. Oh, did I mention completely transparent when turned off? 🤯

I think we explored lots of alternatives, from custom types to templates to bit fields to differentiate between mapped and unmapped pins. Having this set of macros active only for "outside" code was the most minimal hack we could find that at least allowed basic compatibility, including for third party libraries that rely on the API for I/O. Of course when a library starts to access registers or questions what a "pin" is, things go south quickly.

The idea above is very clean but I think it fails on the exactly once bit - unless you rewrite the whole core to call double underscores, which I would very much prefer not to do. How would you address this?

[tttapa]

I think the problem here is that the unmapped and mapped versions of e.g. digitalWrite are fundamentally different functions. They should not have the same name. Ideally, they shouldn't even have the same argument types.
I would be in favor of changing the core to be very explicit about the functions it calls (mapped or not). Since most of these GPIO functions are used only rarely inside of the core itself, it shouldn't be too much work.

That said, if changing the calls inside of the core is not an option, you could:

1. Define the unqualified function as unmapped when compiling the core. This is no different than what exists now.
2. Define a mapped and unmapped version in their own namespace when compiling in C++/Arduino user mode.
3. Either a) the current macro-based solution for user C code, or b) not remap anything in C code (same as the core).

For instance:
https://godbolt.org/z/s6sx3MWdq

#if defined(BOARD_HAS_PIN_REMAP) && !defined(BOARD_USES_HW_GPIO_NUMBERS)
#define REMAP_PIN_TO_GPIO(p) (digitalPinToGPIONumber((p)))
#else
#define REMAP_PIN_TO_GPIO(p) (p)
#endif

// When compiling the core, never map
#if defined(ARDUINO_CORE_BUILD)
#define REMAPPED_FUNC(ret, name, signature, mapped_args, unmapped_args) \
    inline ret ARDUINO_ISR_ATTR name signature { return __##name unmapped_args; }
// In user C++/Arduino code, mapped is the default, unmapped available in namespace if necessary
#elif defined(__cplusplus)
#define REMAPPED_FUNC(ret, name, signature, mapped_args, unmapped_args) \
    inline namespace mapped { inline ret ARDUINO_ISR_ATTR name signature { return __##name mapped_args; } } \
    namespace unmapped { inline ret ARDUINO_ISR_ATTR name signature { return __##name unmapped_args; } }
// In user C code, what to do? Existing macro-based solution?
// We could use
//     inline ret ARDUINO_ISR_ATTR name signature { return __##name mapped_args; }
// but that would break the ODR.
// Alternatively, we could just not do remapping in C code, since most Arduino users won't use it anyway.
#else
/* ? */
#endif

// List all functions ...
REMAPPED_FUNC(uint8_t, digitalRead, (uint8_t pin), (REMAP_PIN_TO_GPIO(pin)), (pin));
REMAPPED_FUNC(void, digitalWrite, (uint8_t pin, uint8_t val), (REMAP_PIN_TO_GPIO(pin), val), (pin, val));

Usage:

// For demonstration
constexpr uint8_t digitalPinToGPIONumber(uint8_t p) { return p + 100; }
extern "C" uint8_t __digitalRead(uint8_t pin) { return pin; }

int main() {
    std::cout << +digitalRead(42) << std::endl;
#ifndef ARDUINO_CORE_BUILD
    std::cout << +mapped::digitalRead(42) << std::endl;
    std::cout << +unmapped::digitalRead(42) << std::endl;
#endif
}

Output with ARDUINO_CORE_BUILD:

42

Output for user code:

142
142
42

There are no ODR violations here, because the functions are in different namespaces. There might still be ODR violations if digitalWrite etc. are called in inline functions in headers that are included both by the core and in user code, but this is no different from the macro-based solution.

[pillo79]

Thank you for your idea and insightful example! 🤩 💯
I think this is actually actionable and could work like the current solution, with the improvement of not polluting the code with macros, but the drawback of making the API definitions pretty opaque: from

uint8_t digitalRead(uint8_t pin);

to some variation of

uint8_t __digitalRead(uint8_t pin);
REMAPPED_FUNC(uint8_t, digitalRead, (uint8_t pin), (REMAP_PIN_TO_GPIO(pin)), (pin));

With some extra FOR_EACH-like macro functionality I could be able to simplify the macro invocation to something like

REMAPPED_FUNC(uint8_t, digitalRead, (uint8_t pin), (pin), MAP_ARG_1);

or even possibly

REMAPPED_FUNC(uint8_t, digitalRead, (uint8_t pin), MAP_ARG_1);

@me-no-dev what do you think about all of this?

[jantje]

Hi Jantje from Sloeber here (Sloeber is a Arduino IDE alternative in eclipse)
It looks as if Sloeber has an issue with this implementation as well.
As far as I can understand this new implementation is dependent on de define ARDUINO_CORE_BUILD which is unknown to Sloeber.
I checked the boards.txt, the platform.txt and https://arduino.github.io/arduino-cli/0.35/platform-specification/ and I do not see any reference/mentions.
Can you elaborate on the define ARDUINO_CORE_BUILD

PS: I commented in this issue as it was the most relevant search result for "ARDUINO_CORE_BUILD" in google and it looks relevant.

[jantje]

Correction on previous remark.
After reinstalling I found ARDUINO_CORE_BUILD in the platform.txt.
I removed it in a effort to work around the file_opts problem

[pillo79]

Hello @jantje, sorry to hear this breaks for you as well.
What Arduino-ESP32 version are you using? This was already implemented in the 2.0.x branch since 2.0.10 IIRC.
What specifically is the issue you are facing?

The platform recipe creates/modifies that file_opts file contents so that only the core files are compiled with the ARDUINO_CORE_BUILD flag set. If you edit the core, you should technically be able to build with no flag at all; in this case, however, there will be a build warning and the core will revert to GPIO pin numbering (same as all other ESP32 boards out there) - which may not be what a Nano ESP32 user is expecting.

[jantje]

The platform recipe creates/modifies that file_opts file contents so that only the core files are compiled with the ARDUINO_CORE_BUILD flag set.

Basically I do not know how Sloeber can meet the requirement to "have a different compile command for the core as for the sketch". This is also the reason why Sloeber does not support some of the hooks
Sloeber/arduino-eclipse-plugin#927 (comment)
The problems that Sloeber faces to support these hooks are:

1. Sloeber supports building multiple files at the same time.
2. In Sloeber make drives the order of build commands. How to tell make to group the core and do not build core and sketch files at the same time
3. Sloeber currently does not distinguish between the core or project files (the core is build with include references to the libraries)

The library includes on the command line for the core build has been a problem before with esp32. Then @me-no-dev and I could fix this with the __IN_ECLIPSE__ define in the esp32 code. A fix which I doubt it will work in this case.

This was already implemented in the 2.0.x branch since 2.0.10 IIRC.

The current version contains the following in the platform.txt

# Set -DARDUINO_CORE_BUILD only on core file compilation
file_opts.path={build.path}/file_opts
recipe.hooks.prebuild.set_core_build_flag.pattern=bash -c ": > {file_opts.path}"
recipe.hooks.core.prebuild.set_core_build_flag.pattern=bash -c "echo -DARDUINO_CORE_BUILD > {file_opts.path}"
recipe.hooks.core.postbuild.set_core_build_flag.pattern=bash -c ": > {file_opts.path}"

Which is not compliant with the Arduino framework specification (set_core_build_flag should be 1) and has been fixed in the current esp32 master for a long time.
This "set_core_build_flag" only works in the Arduino IDE due to a "bug" in arduino IDE and does not work in Sloeber.
Due to the fact these opt files historically were there to allow build command modifications and Sloeber supports build command modifications from the gui I have grown the habit of simply removing these hooks and opts file references from the platform.txt as a workaround.
Which I did here as well; I did it; without noticing the -DARDUINO_CORE_BUILD. As the -DARDUINO_CORE_BUILD is not strictly necessary much later I bumped into it and obviously failed to find the place where ARDUINO_CORE_BUILD was defined. Therefore my comment and correction above.

Note 1: not having the ARDUINO_CORE_BUILD define does not break the build for the default sketch and as such escapes my testing.
Note 2: 😠 ESP has a tendency to use the more exotic features (and create them without documenting them) of the arduino framework. That makes it very hard for other arduino framework implementers to support esp. For this reason I have considered to simply no longer support esp boards. If this -DARDUINO_CORE_BUILD becomes a "must have" for the arduino framework implementer; I will have to stop esp support. unfortunately I currently feel it is not a matter of "if" but of "when". 😠

[me-no-dev]

ESP has a tendency to use the more exotic features (and create them without documenting them) of the arduino framework. That makes it very hard for other arduino framework implementers to support esp

Can you please clarify a bit what you mean? We use only what is available and documented by Arduino.cc. AMF @pillo79 is part of Arduino.cc and this discussion is a result of their efforts to have a working pin remapping. We (the Espressif side) looked into ways to bake that functionality in and not have to rely on defines. Unfortunately, because of the underlying ESP-IDF, such efforts would cause more issues, rather than help.

I will be sad to see you drop ESP support, as Sloeber is one of my fav ways to develop Arduino projects

[jantje]

I will be sad to see you drop ESP support,

I do not want to drop ESP support. But ESP has always been the platform I needed to invest in the most to get it working.
The current -DARDUINO_CORE_BUILD for core only ... I do not see a possible solution (even if we find one it will not be implemented in the next couple of months) which means ESP is not 100% supported in Sloeber and will not be in the near future and I see no workaround.
Now the ARDUINO_CORE_BUILD define is there I think chances it becoming required are very high. At that point I have no other option than stopping ESP support.

as Sloeber is one of my fav ways to develop Arduino projects

I know and you have been a help to me and motivator to keep fixing Sloeber ESP issues. 👍

We use only what is available and documented by Arduino.cc.

First note I said: "ESP has a tendency to use the more exotic features" Which is "documented but (nearly) nobody uses".
I may be wrong assuming esp8266 and ESP32 stuff is one team. But this is an example of usage of a undocumented feature for which I requested documentation 5 year ago.
arduino/arduino-cli#980
Also I do not know Arduino documentation states you can nested environment variables
Sloeber/arduino-eclipse-plugin#1184

The are very limited platforms who use the hooks. And those who do limit themselves to pre build and post build.
Look at these search results https://github.com/search?q=%22recipe.hooks.core.prebuild%22&type=code

ESP is also the only platform that tries to add end user command line modification to Arduino IDE (which Sloeber support out of the box) which caused a whole lot of issues with opt files.
ESP now has gone a step further with using opt files by combining hooks to modify opt files. Pretty exotic stuff in arduino world in my book.

Summary:

• I do not want to stop ESP support at all. It would be very painful to me.
• I'm not saying ESP does anything wrong; what I'm saying is that the more exotic features are being used (and combined) and it is very hard to keep ESP running in Sloeber.
• At this point in time Sloeber is not 100% compatible. In Sloeber it is: ARDUINO_CORE_BUILD is defined everywhere or nowhere.
• I did not want ESP to find out the ESP platform no longer works in Sloeber without getting a warning about the difficulties Sloeber is facing.

[me-no-dev]

I hope that we will find a different approach to pin remapping in a near future, as this is something that we really like and want working correctly everywhere.

[pillo79]

Thanks for the clarifications @jantje, I understand the issue now.

3. Sloeber currently does not distinguish between the core or project files (the core is build with include references to the libraries).

I think improving this bit could be really helpful to the compatibility. PlatformIO had a similar issue and they worked around it not by implementing the core.prebuild / core.postbuild stuff, but by adding the ARDUINO_CORE_BUILD define to each core file's build options. Maybe a similar solution could be implemented using Eclipse CDT as well?

• At this point in time Sloeber is not 100% compatible. In Sloeber it is: ARDUINO_CORE_BUILD is defined everywhere or nowhere.

If this is the case, please do not set it anywhere. The only effect this should have is, users will not be able to use pin remapping. The ones that will notice this change are those who try a sketch designed for an older "Arduino Nano"-series board on an "Arduino Nano ESP32".

• I did not want ESP to find out the ESP platform no longer works in Sloeber without getting a warning about the difficulties Sloeber is facing.

If you don't set the flag and compile for a nano_nora, users will receive a compile-time warning saying that pin remapping has been disabled by the build system. Can you confirm this part actually works?

If you want to automatically identify targets that will be impacted by the missing #define, you can check if the board defines BOARD_HAS_PIN_REMAP, as that is the presence marker of the functionality.

---

I understand these are all hacks that have to be implemented in your project to support ESP targets (and the Nano ESP32 in particular), and I'm sorry for pushing the edges of what is supported. As you can see from the first messages of this thread, we had to find a quite narrow compromise between adding a very invasive feature and keeping platform compatibility. Absolutely open to improving though!