Skip to content

abs() does not work on floats #362

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

Open
nielsswinkels opened this issue May 9, 2018 · 11 comments
Open

abs() does not work on floats #362

nielsswinkels opened this issue May 9, 2018 · 11 comments
Assignees

Comments

@nielsswinkels
Copy link

I'm not sure how to rewrite the page, so I rather write it here as issue instead of pull-request.

abs() does not work on floats, because it returns an int. It does not say so in the documentation at https://github.com/arduino/reference-en/blob/master/Language/Functions/Math/abs.adoc

Apparently for floats there exists fabs(), but this is not written in the arduino ref docs.

Should this be added, or is there some reason for this information missing?

@SeppPenner
Copy link
Contributor

The documentation is unclear on data types anyway. I do not know the fabs method, but this concern seems reasonable. @mastrolinux: Do you have a suggestion here?

@mastrolinux
Copy link
Contributor

@cmaglie please look into this one

@funchords
Copy link

I was led here from this Reddit discussion ... really nothing to add except that 'Calculates the absolute value of an integer.' might be clearer. Or perhaps 'returns, as an integer, the absolute value of the argument' which is more accurate.

@per1234
Copy link
Collaborator

per1234 commented Apr 1, 2019

I plan to go through the entire Arduino Language Reference and document all parameter and return types. I think that will clear up the confusion about abs() and floats. I made the first step towards that goal by establishing a consistent style for documentation of parameter and return types in #559, but now I must wait until someone else gets the time to review and merge it.

As for fabs(), there is an open issue requesting that this function, be added to the Language Reference: #522 I've been a bit on the fence on that, since I've always though of the Language Reference as being a primarily place to document the Arduino-specific functions, with some of the basics of C++ thrown in to get beginners started. There are already some excellent resources elsewhere on the Internet that fully document the C++ language. However, I now know that Arduino's official policy is that the .ino files of Arduino sketches are written in the Arduino Language, rather than in C++ as I had thought previously. With this knowledge, it seems much more logical for the Arduino Language Reference to contain a complete documentation of the Arduino Language, since definitive documentation of this Arduino Language does not exist anywhere else on the Internet.

@per1234
Copy link
Collaborator

per1234 commented Apr 1, 2019

I took a closer look into this issue and found that it's a bit complicated. The standard C++ and C implementations of abs() indeed don't work with float. However, Arduino has replaced that with their own macro implementation in their hardware cores:
https://github.com/arduino/ArduinoCore-avr/blob/1.6.23/cores/arduino/Arduino.h#L87-L94

#define abs(x) ((x)>0?(x):-(x))

which does work with any type. This macro will be used in all .ino files, and any other source file that #includes Arduino.h.

Out of the official Arduino hardware cores, the exception to this is Arduino megaAVR Boards, which does not define its own abs(). Thus, currently the only official Arduino board for which abs() does not support float is the Arduino Uno WiFi Rev2. Arduino AVR Boards, Arduino SAMD Boards, Arduino SAM Boards, and Intel Curie Boards all do define their own abs() macros.

I also checked the most popular 3rd party hardware cores and found that Teensy's abs() does support float, while the "ESP8266 core for Arduino" and "Arduino core for the ESP32" do not.

Arduino's official documentation can't be expected to cover 3rd party projects. Those projects must provide their own documentation for any differences from the official cores. I'm not sure why Arduino made the decision to depart from the convention they established with all their other hardware cores when it came to Arduino megaAVR Boards. I'll have to be sure to document that in the Arduino Language Reference's abs() page.

@funchords
Copy link

I think we definitely have the right person (you) on the case. You understand this very well. I also appreciate @pnndra's comment that the policy is to defer to what the beginner might do and think happens.

Thank you for your service and leadership here.

@SeppPenner
Copy link
Contributor

Out of the official Arduino hardware cores, the exception to this is Arduino megaAVR Boards, which does not define its own abs(). Thus, currently the only official Arduino board for which abs() does not support float is the Arduino Uno WiFi Rev2. Arduino AVR Boards, Arduino SAMD Boards, Arduino SAM Boards, and Intel Curie Boards all do define their own abs() macros.

This should be documented on the page. And you're doing a great job by the way ;)

@dsyleixa
Copy link

dsyleixa commented May 3, 2019

I reported a similar issue in the espressive repo - it finally turned out that is was because of an abs() issue, too:
espressif/esp-idf#3405 (comment)

IMO it's better to use std::abs() instead of non-standard macros.

@bperrybap
Copy link

I would recommend adding fabs() to the reference page.
It is the easiest solution as it solves the issue in a way that will work for all platforms including 3rd party platforms with zero additional work.
If people notice and start to complain about abs() working with floats on some platforms/cores but not on others, we can point to the Arduino language reference page and say that fabs() is what should be used for floats.

Long term, Arduino can address it in some other better yet portable way like creating its own proprietary Arduino math functions with different names that can handle any type.
Perhaps Abs(), Min(), Max(), Round(), etc....


@per1234 I think you need a qualifier to your statement:

Arduino's official documentation can't be expected to cover 3rd party projects.

because I believe that this issue is actually something a little bit different in that, in this case,
Arduino is re-defining and silently overriding a standard C library function to alter its functionality, behavior, and has actually created a silent bug.
Discussions about the issue of using macro overrides for these math functions and potential solutions has come up several times on the Arduino developers mailing list over the past 8-10 years but nothing has ever come out of it.
IMO, silently redefining/replacing standard C functions like abs(), min(), max(), constrain(), round(), etc... is a VERY BAD thing to be doing and just shouldn't be done. Particularly when it introduces a silent bug.

It isn't the 80's anymore where developers often wrote these type of macros to replace system provided functions in order to gain speed and/or potentially reduce code size. Today's compilers are way better at doing optimizations than simple macros could ever do and compilers now have special built in optimizations for these math functions.
Plus these types of macros are dangerous in they can cause variable corruption. Particularly in a macro like the typical abs() implementation
#define abs(x) ((x)>0?(x):-(x))

A more common way to handle these macro re-definitions was to use a different name with all caps being the more common one.
I..e ABS() would be the macro replacement where abs() would be the compiler tools provided version.

IMO, if Arduino wants to create its own proprietary Arduino math functions that can handle any type that is perfectly ok, as long as they don't name them the same as a standard C math function and override the standard C function, especially when it doesn't work the same way or even has a bug in it.

So, IMO, I think it is crazy for Arduino to silently override any standard C math function that has existed for 50 years with a version that works differently and has a bug in it.
IMO, Arduino should remove its macros for all these math functions so they work as they are defined and documented by the C standard and if they want different behavior, come up with new names for their proprietary math functions.

There are other examples of where Arduino has decided to go the proprietary route (which I also disagree with) but at least they did it without overriding standard functions, types, macros.
Some other examples of Arduino proprietary stuff:
boolean - this originally was a uint8_t instead of bool since at the time, on the AVR, uint8_t it often generated better/smaller code than an actual bool type.
This is a great example in that they didn't simply override the bool type but created a new Arduino proprietary type.

Then there is all Arduino proprietary WCharacter functions where Arduino came up with new names for all the standard ctype functions. Seemed silly to me, but at least they didn't override any of the standard ctype functions to work differently.

The goal for Arduino is to create a portable programming environment.
Doing things like overriding standard C functions with simplistic macros that work differently than the standard functions and have bugs, is, IMO, a move in the wrong direction.

In the future if Arduino wants a simpler easier to use solution that doesn't require different functions for different argument types, it should come up with its own functions that work on any type.
It could use templates or inline functions to ensure that the code is optimized inline and doesn't suffer from the potential variable corruption issues that the macros do.

@bperrybap
Copy link

To add a bit more complexity to the issue. As of C++ 17, the standard functions like abs(), min(), max() now support any type. So as soon as the platforms use newer compiler tools in their cores, they can remove these silly math macros and get multi type support from the standard functions (at least when using C++)

@dsyleixa
Copy link

as to C++17: when will this be a standard in the Arduino IDE?
but as to std::abs(), this is supposed to work already now, isn't it (CMIIW)?

as to

However, I now know that Arduino's official policy is that the .ino files of Arduino sketches are written in the Arduino Language, rather than in C++

actually nothing exists like an "Arduino Language". Arduino API code is nothing else than C++ wrapped by some "convenient" and "handy" C++ API libs, so it's still C++, including all language documentations, references, and standards, and so are expected to be compatible to each other.

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

No branches or pull requests

8 participants