Skip to content
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

Calling round() inside print() result differs between v2.4.2 and v2.5.0-beta2 #5530

Open
Paraphraser opened this issue Dec 20, 2018 · 8 comments

Comments

@Paraphraser
Copy link
Contributor

Basic Infos

  • [Y] This issue complies with the issue POLICY doc.
  • [Y] I have read the documentation at readthedocs and the issue is not addressed there.
  • [N] I have tested that the issue is present in current master branch (aka latest git). I simply upgraded the IDE to 2.5.2-beta2 and then compared it with 2.4.2 behaviour and observed an unexpected and unwelcome difference.
  • [Y] I have searched the issue tracker for a similar issue.
  • [N/A] If there is a stack dump, I have decoded it.
  • [Y] I have filled out all fields below.

Platform

  • Hardware: [ESP8286]
  • Core Version: [2.5.0-beta2 & 2.4.2]
  • Development Env: [Arduino IDE]
  • Operating System: [MacOS]

Settings in IDE

  • Module: [Wemos D1 mini r2]
  • Flash Mode: [dio - as far as I am aware]
  • Flash Size: [4MB/1MB]
  • lwip Variant: [v2 Lower Memory]
  • Reset Method: [whatever the IDE does at the end of a download]
  • Flash Frequency: [no menu item for this - can't answer]
  • CPU Frequency: [80Mhz]
  • Upload Using: [SERIAL]
  • Upload Speed: [921600)

Problem Description

Under version 2.4.2, round() appears to return an integer quantity. Under 2.5.0-beta2, round() appears to return a real quantity. I am inferring this from a change in print() behaviour.

MCVE Sketch

#include <Arduino.h>

void setup() {

    Serial.begin(115200);
    while (!Serial);
    delay(100);
    Serial.println();
    Serial.println();

}

void loop() {

    // wait a while
    delay(1511);

    // precalculate upTime to the nearest second
    unsigned long upTime = round(millis()/1000.0);

    // display upTime with an inline calculation
    Serial.print("Up time: inline = ");
    Serial.print(round(millis()/1000.0));
    Serial.print(", pre-calculated = ");
    Serial.println(upTime);
    
}

Debug Messages

Output from compiling under 2.4.2 libraries. Note ABSENCE of decimal places for "inline =" values. This is the expected behaviour.

Up time: inline = 2, pre-calculated = 2
Up time: inline = 3, pre-calculated = 3
Up time: inline = 5, pre-calculated = 5
Up time: inline = 6, pre-calculated = 6
Up time: inline = 8, pre-calculated = 8
Up time: inline = 9, pre-calculated = 9
Up time: inline = 11, pre-calculated = 11
Up time: inline = 12, pre-calculated = 12
...

Output from compiling under 2.5.0-beta2 libraries. Note PRESENCE of decimal places for "inline =" values. This suggests print() is treating the return value from round() as a real quantity. This is unexpected.

Up time: inline = 2.00, pre-calculated = 2
Up time: inline = 3.00, pre-calculated = 3
Up time: inline = 5.00, pre-calculated = 5
Up time: inline = 6.00, pre-calculated = 6
Up time: inline = 8.00, pre-calculated = 8
Up time: inline = 9.00, pre-calculated = 9
Up time: inline = 11.00, pre-calculated = 11
Up time: inline = 12.00, pre-calculated = 12
...
@penfold42
Copy link

If this credible: http://www.cplusplus.com/reference/cmath/round/
“The value of x rounded to the nearest integral (as a floating-point value).”
The current behaviour seems correct.

@penfold42
Copy link

Either cast it to an int or specify the precision in print :
https://www.arduino.cc/en/Serial/Print
Serial.println(1.23456, 0) gives "1"

@Makuna
Copy link
Collaborator

Makuna commented Dec 20, 2018

@devyte
Copy link
Collaborator

devyte commented Dec 20, 2018

Arduino is not standard C++, but some sort of language of its own that is based on C++

That makes no sense at all. The reasons behind some of the contents of Arduino.h are other, and some of that explanation hints at it: they didn't have the standard implementation way back when they started out, and by now they've entrenched themselves for historical and backwards compatibility reasons, bugs and all.

Here, we try to stick to the C++ standard, and to standard implementations, because not doing that leads to madness. If other sketches are found to behave differently as a result, those sketches need to be fixed. That's a good thing. It has already happened before, e.g. with abs() => std::abs().

@penfold42 shows that we comply with the cplusplus reference.

Closing as non issue.

@devyte devyte closed this as completed Dec 20, 2018
@Paraphraser
Copy link
Contributor Author

Where did "Arduino is not standard C++, but some sort of language of its own that is based on C++" come from? I don't see that anywhere in the thread.

Rather than aiming for brevity, I should probably have gone on to explain what I found most troubling about this change in behaviour.

The header file "... cores/esp8266/Arduino.h" contains this line:

#define round(x)     ((x)>=0?(long)((x)+0.5):(long)((x)-0.5))

The line is the same in both 2.4.2 and 2.5.0-beta2 so it should be clear that the intention of the author(s) was that both implementations should have the same behaviour.

If that #define had disappeared in the v2.5.0-beta2 then it would be clear that the math.h implementation was now in force.

The #define is still present so I don't understand how the behavioural change has come about. If we makers can't rely on reading the code to understand behaviour then I think we are on a very slippery slope.

@devyte
Copy link
Collaborator

devyte commented Dec 20, 2018

Sigh, I thought that macro was removed already. Looks like our Arduino.h needs some further cleanup.

The mention comes from the link to issue 6098 in the arduino repo, linked by @Makuna .

I'm reopening to cover the cleanup in Arduino.h .

@devyte devyte reopened this Dec 20, 2018
@devyte devyte self-assigned this Dec 20, 2018
@devyte devyte modified the milestones: W, 2.5.0 Dec 20, 2018
@Makuna
Copy link
Collaborator

Makuna commented Dec 20, 2018

I am sorry to hear that "here" we don't stick to the Arduino standard. I don't agree with breaking the Arduino standard as all it leads to is more support issues. There was a reason we had that "fix"/"change", it was to keep compatiblity. Get Arduino to change first, then follow. It seems that they will at some point but they haven't yet.
If you read the link, several other entry level languages have this same broken round method that Arduino stuck to. So entry level developers are used to this and have the expectation. More advanced developers should be able to figure it out and use alternatives, like roundf.
Its sick that even math.h uses macros.

@devyte
Copy link
Collaborator

devyte commented Dec 20, 2018

@Makuna I feel I need to clarify something at this point. Arduino has a reference implementation of a core, and the api of that reference is mimicked in cores implemented for other microcontrollers in order to maintain compatibility. That reference implementation doesn't have revisions, there is no committee that gets together periodically to discuss changes, there is no publication by a standards organization, and the documentation is only of the api meaning, and not of the behavior in detail. In fact, the Arduino reference docs don't even mention argument types or return type for functions, and for this particular case, it doesn't even mention round() as a function.

Therefore, Arduino is not a standard.

C89/99/11/18 are standards. C++11/14/17 are standards. Even the AVR-libc, isn't a standard, but rather an arduino port of the standard C library for AVR. Which, btw, has float round(float) and long lround(float).

Get Arduino to change first, then follow

Point of fact, the issue you linked was opened 2 years ago, was discussed for a year after that, and is currently without even a milestone assigned. I have had a discussion or two with arduino guys regarding changes (not necessarily related to the topic at hand), and it has been explained to me in no uncertain terms why they resist change, even for fixing known bugs. It is my personal view that their lack of agility hampers users more than it benefits them, so I do not consider it wise to spend effort in that direction. Instead, I rather lead the way, showing what is correct and works well, and let the results ripple outwards. As I said, this has happened at least once already, and I expect it will happen again. Leading by example is better than pushing repeatedly against resistance, and waiting around just lets users hit the same old bugs over and over again.

all it leads to is more support issues

Compatibility in the API is a priority. Compatibility with known bugs is not. As stated in the link, the macro behavior is buggy and unpredictable.

entry level developers are used to this

I don't agree with this. Entry level developers should deal with learning and experimenting, and not with brokenness and ancient bugs. But that's just me.

@devyte devyte removed this from the 2.5.0 milestone Jan 22, 2019
@devyte devyte added this to the 3.0.0 milestone Nov 9, 2019
@d-a-v d-a-v modified the milestones: 3.0.0, 3.0.1 Apr 1, 2021
@d-a-v d-a-v modified the milestones: 3.0.1, 3.1 Jun 16, 2021
@d-a-v d-a-v modified the milestones: 3.1, 4.0.0 Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants