Update Arduino.h - abs() macros

[jenyaza01]

Do not flip x, if x is zero (macros abc(x) )

[CLAassistant]

All committers have signed the CLA.

[matthijskooijman]

I guess this is relevant for floating point numbers? For integers, it should not matter, right?

[jenyaza01]

Yes, you`re right. But that looks relevant to some pointers my friend has troubles with, and '>=' helps.
Also it looks right )

[bxparks]

Curious, why is your friend performing an abs() operation on a pointer? Did you mean pointer difference?

I was curious to find out if the compiler would optimize the 2 versions to be identical, because they are semantically the same for integer types. So I disassembled the binary. It looks like on the AVR compiler, the >= version is 2 bytes smaller and one clock cycle faster, because the >= can just check the sign bit instead of comparing against 0:

Using >

  y = ((x)>0?(x):-(x));
 1aa:	80 91 02 01 	lds	r24, 0x0102	; 0x800102 <x>
 1ae:	90 91 03 01 	lds	r25, 0x0103	; 0x800103 <x+0x1>
 1b2:	18 16       	cp	r1, r24
 1b4:	19 06       	cpc	r1, r25
 1b6:	94 f0       	brlt	.+36     	; 0x1dc <main+0xb8>
[everything following identical]

Using >=

  y = ((x)>=0?(x):-(x));
 1aa:	80 91 02 01 	lds	r24, 0x0102	; 0x800102 <x>
 1ae:	90 91 03 01 	lds	r25, 0x0103	; 0x800103 <x+0x1>
 1b2:	97 ff       	sbrs	r25, 7
 1b4:	12 c0       	rjmp	.+36     	; 0x1da <main+0xb6>
[everything following identical]

For vast majority of people, this isn't a difference worth worrying about. The bigger problem with abs(), and the other macros like min() and max(), is that they can cause duplicate side-effects. They probably should be implemented as template functions... but I'm probably beating a dead horse.

Anyway, if these subtle edge cases matter in an app, I would just roll my own versions, and move on.

[Edit: fix typo]

[matthijskooijman]

Yes, you`re right. But that looks relevant to some pointers my friend has troubles with, and '>=' helps.
Also it looks right )

@jenyaza01 Right, some examples with pointers that show that this change helps would be useful, then.

@bxparks Nice, thanks for digging in, I'm actually a bit surprised to see this difference. I guess the compiler could optimize this, but needs a fairly specific optimization pattern to realize that it actually helps to change it. Still, optimization could be a reason to make this change in itself.

The bigger problem with abs(), and the other macros like min() and max(), is that they can cause duplicate side-effects. They probably should be implemented as template functions... but I'm probably beating a dead horse.

IIRC this horse was killed and buried (i.e. the problem fixed) in https://github.com/arduino/ArduinoCore-API, so that fix will migrate to to the different cores once they start using the code from -API (AVR does not yet). This also means that any change to these macros will no longer be accepted in this repo directly, but should go through the -API repo instead. And then I suspect that a small optimization on AVR is no longer a reason to change this macro, unless it would also have a similar effect on ARM.

[jenyaza01]

Thanks for THAT detail explanation) now I understand it

[bxparks]

@matthijskooijman:

I see only min() and max() template functions in Arduino-API:
https://github.com/arduino/ArduinoCore-API/blob/master/api/Common.h

The abs() remains a macro in Arduino-SAMD, which has already moved to the Core API:
https://github.com/arduino/ArduinoCore-samd/blob/master/cores/arduino/Arduino.h

This means that the side-effect problem of abs() is not fixed, and won't be fixed in the future. I'm actually more worried about the incorrect handling of negative-zero floating point that you pointed out, because I care about correctness first, then performance later. Fortunately, as far as I can tell, there is no way to observe a negative-zero on an AVR. The Print::print() method does not handle a negative-zero, and snprintf() does not support floating point numbers. (FYI: The Print::printf() functions on both ESP8266 and ESP32 will correctly print negative-zero as "-0.00000"). I suspect that negative-zero may not even be implemented in the low-level AVR floating point libraries.

I think we can summarize the problems with the current abs() macro as follows:

1. It handles the negative-zero floating point number incorrectly (though not observable on AVR),
2. It is a CPP macro, which can cause duplicate side-effects (seems relatively serious to me),
3. The current version is 2 bytes bigger and one clock cycle slower on an AVR, than the more correct version (minor optimization),
4. It will not be fixed when Core-AVR upgrades to Core-API.

I am curious to know what the ARM compiler does.. but this is not something I have time to do right now, maybe in the future.

I donno, I think this fix might be worth making, but totally up to you guys. But I don't have a strong opinion on this matter, since
I always roll my own versions of these low-level primitives.

[bxparks]

[Ignore this post. I don't think I'm thinking straight right now.]

Heh, I just realized that for negative-zero, both the > version and the >= version will return exactly same thing, in other words, abs(-0.0) will return -0.0 no matter what. This is because negative-zero is defined to be equal to positive-zero, so the expression (-0.0 >= 0.0) and (-0.0 > 0.0) will both return true, and the abs(x) macro will return the original number, which is -0.0. Float point numbers are weird.

So ignore item (1) above about negative-zero, it's fine, or at least as fine as it can be using normal operator>() and operator>=()` operators. The remaining issues are then: 2) macro side-effects, 3) slightly slower and bigger, and 4) not is not templatized by moving to Core-API.

[matthijskooijman]

The abs() remains a macro in Arduino-SAMD, which has already moved to the Core API:

Right, so the multiple evaluation / macro issue is indeed not fixed in -API yet, but it is being tracked here: arduino/ArduinoCore-API#85

The handling of floating point and negative zero is indeed an open point, but as you say, it seems that 0.0 and -0.0 behave the same. So we cannot get it really right without explicitly checking for negative zero somehow (which probably fails when using an integer, or causes float operations at runtime when called with an int). However, I expect the current version will have abs(0.0) == -0.0 and abs(-0.0) == 0.0, while the version from this PR will have abs(0.0) == 0.0 and abs(-0.0) == -0.0, so the latter seems slightly better since it does not introduce negative zero when it was not there already.

Looking at this stackoverflow post, adding 0.0 might actually force the -0 into 0. This seems to work, even with adding 0, which ensures that (I expect) it gets optimized away when called with an integer.

Here's a quick test I wrote:

matthijs@grubby:~$ cat foo.c 
#include <stdio.h>

#define oldabs(x) ((x)>0?(x):-(x))
#define newabs(x) ((x)>=0?(x):-(x))
#define pluszeroabs(x) ((x)>=0?(x)+0:-(x))

int main() {
        float zero = 0.0, negzero = -0.0;
        printf("0.0: %f, -0.0: %f\n", zero, negzero);
        printf("0.0 > 0.0: %d\n", zero > zero);
        printf("0.0 >= 0.0: %d\n", zero >= zero);
        printf("0.0 < 0.0: %d\n", zero < zero);
        printf("0.0 <= 0.0: %d\n", zero <= zero);
        printf("0.0 > -0.0: %d\n", zero > negzero);
        printf("0.0 >= -0.0: %d\n", zero >= negzero);
        printf("0.0 < -0.0: %d\n", zero < negzero);
        printf("0.0 <= -0.0: %d\n", zero <= negzero);
        printf("-0.0 > 0.0: %d\n", negzero > zero);
        printf("-0.0 >= 0.0: %d\n", negzero >= zero);
        printf("-0.0 < 0.0: %d\n", negzero < zero);
        printf("-0.0 <= 0.0: %d\n", negzero <= zero);
        printf("oldabs(0.0): %f\n", oldabs(zero));
        printf("oldabs(-0.0): %f\n", oldabs(negzero));
        printf("newabs(0.0): %f\n", newabs(zero));
        printf("newabs(-0.0): %f\n", newabs(negzero));
        printf("pluszeroabs(0.0): %f\n", pluszeroabs(zero));
        printf("pluszeroabs(-0.0): %f\n", pluszeroabs(negzero));
        return 0;
}
matthijs@grubby:~$ gcc foo.c && ./a.out .
0.0: 0.000000, -0.0: -0.000000
0.0 > 0.0: 0
0.0 >= 0.0: 1
0.0 < 0.0: 0
0.0 <= 0.0: 1
0.0 > -0.0: 0
0.0 >= -0.0: 1
0.0 < -0.0: 0
0.0 <= -0.0: 1
-0.0 > 0.0: 0
-0.0 >= 0.0: 1
-0.0 < 0.0: 0
-0.0 <= 0.0: 1
oldabs(0.0): -0.000000
oldabs(-0.0): 0.000000
newabs(0.0): 0.000000
newabs(-0.0): -0.000000
pluszeroabs(0.0): 0.000000
pluszeroabs(-0.0): 0.000000

So, it seems that using the expression ((x)>=0?(x)+0:-(x)) is actually best:

1. Using >= is the most sane, no need to invert 0
2. Using +0 ensures -0.0 becomes 0.0
3. This produces slightly shorter code on AVR (side benefit)

However, unless abs was intentionally left undefined in the -API repo (which is an open question in arduino/ArduinoCore-API#85 (comment)), I think the best place to fix this would be in the -API repo, not here. So I'll add a comment summarizing this discussion in that issue, to be fixed when the remaining math macros are converted to template functions as well.

[matthijskooijman]

If this is indeed going to be fixed via -API, I think this PR can be closed.

However, @jenyaza01, I'm still curious to see an example of your (friend's) usecase of pointers with abs that was fixed by this change. Right now I have no idea what you mean there, so I cannot comment on whether that particular usecase is something to support or fix at all...

[jenyaza01]

However, @jenyaza01, I'm still curious to see an example of your (friend's) usecase of pointers with abs that was fixed by this change.

Sorry, don't know either, what he was actually doing, all i saw -

this error screen, where error message contains abs() without '=', and he tell that redefining with '>=' fixed it. I can't reproduce any error with abs() on my PC either, but still think about fix for that, looks like, typo for all at once ))

[matthijskooijman]

Right, I can imagine that a pointer argument would trigger that error (since >= 0 is always true on a pointer or any unsigned value, so that could prevent the -(x) from being evaluated and triggering this error, maybe. Haven't actually tried, though.

[dok-net]

Please take a look at the abs macro in ESP8266 core's Arduino.h, the object copy issue for x is solved there (there it's >, >= is my edit for the benefit of this PR):

#define abs(x) ({ __typeof__(x) _x = (x); _x >= 0 ? _x : -_x; })

[bxparks]

@dok-net: How will you prevent 0.0 from becoming -0.0 if you change it to >=? (Recall that (-0.0 >= 0) returns true).

[dok-net]

@bxparks If -0.0 >= 0 is true, but 0.0 >= 0as well, how would abs(0.0) yield -0.0?

[dok-net]

The MCVE in esp8266/Arduino#8115 builds for Arduino AVR, I haven't run it just yet, will possibly do so later today.

[bxparks]

@dok-net : Sorry I got my signs reversed, which is really easy to do with -0.0. What I meant was:

• Using >, abs(0.0) returns -0.0, which is unexpected.
• Using >=, abs(-0.0) returns -0.0, which is also unexpected.

I believe this is why @matthijskooijman added the extra +0 operation in the previous comment above. I have not checked if the compiler optimizes that away for integer operands.

The following was executed on an ESP8266 (NodeMCU 1.0/v2):

$ cat AbsUsingGreaterThanEqual.ino 
#include <Arduino.h>
#undef abs
#define abs(x) ({ __typeof__(x) _x = (x); _x >= 0 ? _x : -_x; })
void setup() {
  delay(1000);
  Serial.begin(115200);
  double posZero = 0.0;
  double negZero = -0.0;
  Serial.printf("posZero=%lf; abs(posZero)=%lf\n", posZero, abs(posZero));
  Serial.printf("negZero=%lf; abs(negZero)=%lf\n", negZero, abs(negZero));
}
void loop() {}

$ au --cli upmon nodemcu:USB0
+ /home/brian/bin/linux/arduino-cli  compile --upload  --fqbn esp8266:esp8266:nodemcuv2:xtal=80,vt=flash,exception=disabled,ssl=all,eesz=4M2M,led=2,ip=lm2f,dbg=D
isabled,lvl=None____,wipe=none,baud=921600 --port /dev/ttyUSB0 --warnings all [...] AbsUsingGreaterThanEqual.ino
[...]
posZero=0.000000; abs(posZero)=0.000000
negZero=-0.000000; abs(negZero)=-0.000000

[dok-net]

@bxparks Unfortunately, this minor fix isn't going forward either for AVR or ESP8266. Anyway, we should definitely not talk about other HW like ESP's in this AVR PR.