-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
Use of macros for pin remapping breaks user code and libraries #9150
Comments
@pillo79 could you please have a look? |
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" wayIf 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++). With this attention, the code will be able to use either pin numbering choice transparently. The "easy out" wayUnfortunately, 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 |
Hi, thanks for the detailed reply!
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
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 arduino-esp32/cores/esp32/io_pin_remap.h Lines 12 to 13 in 7d26b07
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 |
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? |
I think the problem here is that the unmapped and mapped versions of e.g. That said, if changing the calls inside of the core is not an option, you could:
For instance: #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
Output for user code:
There are no ODR violations here, because the functions are in different namespaces. There might still be ODR violations if |
Thank you for your idea and insightful example! 🤩 💯 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 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? |
Hi Jantje from Sloeber here (Sloeber is a Arduino IDE alternative in eclipse) PS: I commented in this issue as it was the most relevant search result for "ARDUINO_CORE_BUILD" in google and it looks relevant. |
Correction on previous remark. |
Hello @jantje, sorry to hear this breaks for you as well. The platform recipe creates/modifies that |
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
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
The current version contains the following in the platform.txt
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. Note 1: not having the ARDUINO_CORE_BUILD define does not break the build for the default sketch and as such escapes my testing. |
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 |
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.
I know and you have been a help to me and motivator to keep fixing Sloeber ESP issues. 👍
First note I said: "ESP has a tendency to use the more exotic features" Which is "documented but (nearly) nobody uses". The are very limited platforms who use the hooks. And those who do limit themselves to pre build and post build. 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. Summary:
|
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. |
Thanks for the clarifications @jantje, I understand the issue now.
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
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".
If you don't set the flag and compile for a If you want to automatically identify targets that will be impacted by the missing #define, you can check if the board defines 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! |
I do not think this will help much. It is also undocumented and I only found out when investigating a ESP issue.
Seriously are you trying to get me angry?
That is the Sloeber default behavior because the esp hook name
Euch no. The nano_nora board in Arduino esp32 v2.0.13 does not build in Sloeber. The reason
together results in
Rebuilding without BOARD_HAS_PIN_REMAP means modifying boards.txt.
As shown above. No it does not work.
As shown above all boards who have this option by default will not work in Sloeber.
I do not think anyone in esp world really understands what it costs to push the edges. I'm not the only one saying so:
arduino/arduino-cli#2369 (comment) And seriously: you need a define to change the content of a header file based on whether it is compiled as part of the core or not. All core source files know they are core files. Why not do the define at the top of the core source files that need it? I understand arduino esp is in between the esp SDK and the arduino framework (like sloeber is between CDT and the arduino framework) and this gives some extra difficulties. But please explain why the 2 simple solutions I give above would not work. |
@jantje I am merely offering my point of view as an Arduino core developer. As I wrote, soon after the feature was released, the same pain was shared by other third party build systems - and we worked together to find a solution. So let's focus this discussion on finding a technical way to allow this functionality to be supported in Sloeber.
|
@pillo79 I am all for a better solution with less "jankiness" and exotic features. |
I'm really surprised that you even suggest changing every library in existence @me-no-dev @pillo79, and that you are trying to push these kinds of changes down to library writers. I know there are issues because of the background of there being no proper official API for Arduino, but this is a purposeful breaking change, using a pre-processor definition for such a critical function. I don't even know what fix to suggest to users other than using legacy mode which may not work with PlatformIO. As others have said further up, what about IoExpander libraries of which there are many (including mine) that at least aimed to provide some consistency across the core device and expander. Were all workflows properly evaluated before making this change? Were users of the core involved? Most library writers make barely any money off the libraries that they write, and these days they are a philanthropic effort to help new programmers, please don't push work onto us. |
@davetcc what we are discussing here is being used only on a single board from the hundreds we have. All other boards and configs are unaffected. Are you having troubles with users and Arduino Nano32? |
Yes, see the link above: TcMenu/TaskManagerIO#53 |
@davetcc Arduino IDE provides a way to switch the remap off on the board and your user commented all is OK. I guess the fastest "fix" we could come up with would be to provide a way for PIO to compile without pin remap. I'll comment in your thread |
Coming back to this, it works temporarily for most libraries with that option off, but what is the long-term fix for this? Are we expecting into the long term that users will have to turn this off to use many third-party libraries? I am sure not just mine, but many libraries will not work properly, and give users a terrible experience with the nano ESP32 core. This seems a very strange change, is there a spec for it or other documentation anywhere, so that I can read up on the change in data types to these functions, and which other cores you intend to apply this change to @pillo79? |
Hello @davetcc - the rationale comes from the way we have been numbering pins on all boards made by Arduino (the company). We have board families (Uno, Giga, Nano, etc) that share the same pinout on any of them - a sketch made for one should work out of the box on others from the same family (even if the core MCU is different). In other words, pin Usually this is easy, as GPIOs usually have BSP-specific names and a table that maps a GPIO number to "whatever defines a specific pin on this CPU" is easy to do. But this is next to impossible for the Nano ESP32, since the ESP32 core already uses numbers natively, and did not support a way to map these. We have no intent of "applying" this to anything other than our boards (and the Nano ESP32 is the only one ATM that uses an ESP32). We are deeply aware of this issue and understand the burden on library developers, but I simply don't have better alternatives at the moment. 🙁 However, I can assure that following this guide, and in particular:
will fix all issues, as in that case no macros are defined at all. |
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#L121Sketch
Debug Message
I have checked existing issues, online documentation and the Troubleshooting Guide
The text was updated successfully, but these errors were encountered: