Improve Arduino compatibility with the STL (min and max macro)

[prototypicalpro]

Replaced the min and max macros with std::min and std::max to allow compilation of bits/stl_algobase.h, a core component of many STL components such as std::function.

[per1234]

They did this in the ESP8266 core for Arduino and it caused breakage due to Arduino's traditional macro-based min and max supporting parameters of different types, but std:min and std:max not supporting this.

Examples:

• Can't compile using Core Version: 2.4.0 and Adafruit_PWMServoDriver.h esp8266/Arduino#4096
• USB Host Shield Library fails to compile using 2.4.0 esp8266/Arduino#4154
• adafruit mqtt esp8266 example error esp8266/Arduino#4382

Demonstration code that compiled before this change, but causes an error after:

void setup() {
  byte foo = 42;
  byte bar = max(foo, 100);
}

void loop() {}

sketch_apr10a:3:26: error: no matching function for call to 'max(byte&, int)'

   byte bar = max(foo, 100);

                          ^

C:\Users\per\AppData\Local\Temp\arduino_modified_sketch_632562\sketch_apr10a.ino:3:26: note: candidates are:

In file included from C:\Users\per\AppData\Local\Arduino15\packages\arduino\hardware\samd\1.6.21\cores\arduino/Arduino.h:80:0,

                 from C:\Users\per\AppData\Local\Temp\arduino_build_865727\sketch\sketch_apr10a.ino.cpp:1:

c:\users\per\appdata\local\arduino15\packages\arduino\tools\arm-none-eabi-gcc\4.8.3-2014q1\arm-none-eabi\include\c++\4.8.3\bits\stl_algobase.h:260:5: note: template<class _Tp, class _Compare> const _Tp& std::max(const _Tp&, const _Tp&, _Compare)

     max(const _Tp& __a, const _Tp& __b, _Compare __comp)

     ^

c:\users\per\appdata\local\arduino15\packages\arduino\tools\arm-none-eabi-gcc\4.8.3-2014q1\arm-none-eabi\include\c++\4.8.3\bits\stl_algobase.h:260:5: note:   template argument deduction/substitution failed:

C:\Users\per\AppData\Local\Temp\arduino_modified_sketch_632562\sketch_apr10a.ino:3:26: note:   deduced conflicting types for parameter 'const _Tp' ('unsigned char' and 'int')

   byte bar = max(foo, 100);

                          ^

In file included from C:\Users\per\AppData\Local\Arduino15\packages\arduino\hardware\samd\1.6.21\cores\arduino/Arduino.h:80:0,

                 from C:\Users\per\AppData\Local\Temp\arduino_build_865727\sketch\sketch_apr10a.ino.cpp:1:

c:\users\per\appdata\local\arduino15\packages\arduino\tools\arm-none-eabi-gcc\4.8.3-2014q1\arm-none-eabi\include\c++\4.8.3\bits\stl_algobase.h:216:5: note: template<class _Tp> const _Tp& std::max(const _Tp&, const _Tp&)

     max(const _Tp& __a, const _Tp& __b)

     ^

c:\users\per\appdata\local\arduino15\packages\arduino\tools\arm-none-eabi-gcc\4.8.3-2014q1\arm-none-eabi\include\c++\4.8.3\bits\stl_algobase.h:216:5: note:   template argument deduction/substitution failed:

C:\Users\per\AppData\Local\Temp\arduino_modified_sketch_632562\sketch_apr10a.ino:3:26: note:   deduced conflicting types for parameter 'const _Tp' ('unsigned char' and 'int')

   byte bar = max(foo, 100);

                          ^

You can argue that the code that broke is bad code, but the impact of this change should at least be considered.

[prototypicalpro]

Good point. I've replaced the using declarations with template functions supporting multiple types. These functions should behave the same as the min and max macro, but they can also coexist with std::min and std::max.

[NameOfTheDragon]

This is a much needed PR for SAMD boards, I hope it will be merged soon. I tried the changes locally by pasting directly into my local Arduino.h and it works very well. Without this change, basically any use of a library that uses std:: namespace is likely to fail due to collision between std::min() and std::max() and the #defines. I know you know this but from an end user perspective, I want you to know this is causing real pain that needs a fix.

[prototypicalpro]

@per1234

[NameOfTheDragon]

This can't get into a release fast enough as far as I'm concerned. I keep having to manually edit my Arduino.h file with these changes or my code will not build.

[amirna2]

Any ideas when this is going to be merged? Currently, I am having to manually patch each time I use an Arduino Zero based board.

[matthijskooijman]

If I read the PR correctly, it now removes the abs and round macros when using C++ without providing alternatives?

Also, in the ArduinoCore-API repo, this is also fixed in a similar way, but with even more reliable macros for C. See arduino/ArduinoCore-API#85 for some discussion about this and other related changes.

[prototypicalpro]

@matthijskooijman The abs and round macros are left unmodified (see https://github.com/arduino/ArduinoCore-samd/pull/399/files#diff-517f1981b375d8695337397cddc169d2R103-R104). I do agree that this problem has already been solved in other Arduino repositories, and has been for quite a while. I'd be happy to replace this PR with the changes in the ArduinoCore-API if that would improve the merge-ability?

[matthijskooijman]

The abs and round macros are left unmodified

They are, but they are now inside the #ifndef __cplusplus block, so not usable from C++ anymore.

I'd be happy to replace this PR with the changes in the ArduinoCore-API if that would improve the merge-ability?

I'm not sure, at some point all of this code should just be replaced by the API versions (see arduino/ArduinoCore-API#95), but I'm not sure what the progress on that is. Also, I'm unsure if any such changes would still be merged here separetely, that's something for the Arduino devs to answer, I guess.

[aentinger]

As far as I can see your proposed changes have made it into ArduinoCore-API. Since #567 has been merged you should have your desired functionality. Please reopen if you've got a different opinion.

[matthijskooijman]

@aentinger IIUC #567 only adds automated testing in preparation of merging ArduinoCore-API here, right? I don't see the API used in master yet?

Regardless, I think this can be closed, as the PR as-is is not ideal and will probably not be merged with an upcoming -API merge.

[aentinger]

My bad, I linked the wrong PR. In fact, #560 which was merged just yesterday, brought ArduinoCore-API to ArduinoCore-samd. Care to give it a spin?

[matthijskooijman]

Ah, I see that the files from ArduinoCore-API have been imported verbatim now, rather than in an api subdirectory was was previously suggested for another core (AVR maybe), that's why I didn't see it. Cool to see this is merged, I'll be sure to test this when I do some SAMD work again (lots of STM32 for me lately, though).

[aentinger]

Ah, I see that the files from ArduinoCore-API have been imported verbatim now, rather than in an api subdirectory was was previously suggested for another core (AVR maybe), that's why I didn't see it.

Hi 👋 that's not correct. The API needs to be symlinked in into an api subdirectory. If you'd check out the core from master now it wouldn't compile if you don't symlink in the api. In the next packaged core release naturally the api folder will be available within the release, as requiring users to manually symlink would be a bit of a violation of Arduino philosophy.

[matthijskooijman]

Right, I didn't look close enough again. I see how it works now, thanks!