Skip to content

Test Stream::parseFloat() with many input digits #133

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.

Sign up for GitHub

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

Merged
aentinger merged 7 commits into from
Jan 25, 2021
Merged

Test Stream::parseFloat() with many input digits #133

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
129ae52
Test Stream::parseFloat() with many digits
edgar-bonet Dec 10, 2020
8eb4013
Use Approx() macro for fuzzy comparison
edgar-bonet Dec 11, 2020
5445db3
Test Stream::parseFloat() with a large number
edgar-bonet Dec 11, 2020
fae13e5
Exchanging the type of 'value' from 'long' to 'double' prevents overf…
aentinger Dec 14, 2020
1266b08
Replacing computational expensive pow call with result of accumulated…
aentinger Dec 14, 2020
91c5e29
Test Stream::parseFloat() with a huge number of digits
edgar-bonet Jan 25, 2021
6197511
Avoid overflowing parseFloat()'s internal 'value'
edgar-bonet Jan 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions api/Stream.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

#include "Common.h"
#include "Stream.h"
#include <math.h>

#define PARSE_TIMEOUT 1000 // default number of milli-seconds to wait

Expand Down Expand Up @@ -162,9 +163,9 @@ float Stream::parseFloat(LookaheadMode lookahead, char ignore)
{
bool isNegative = false;
bool isFraction = false;
long value = 0;
double value = 0.0;
int c;
float fraction = 1.0;
unsigned int digits_post_comma = 0;
Copy link
Contributor Author

@edgar-bonet edgar-bonet Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If value is a floating point number, there is no need for this extra variable. It could be folded into value.

Copy link
Contributor

@aentinger aentinger Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question here. What's more computing time expensive - pow or repeatedly doing a double multiplication?

Copy link
Contributor

@aentinger aentinger Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, a further argument against pow is that there's a question of availability (and with what argument types) across all supported platforms.


c = peekNextDigit(lookahead, true);
// ignore non numeric leading characters
Expand All @@ -181,7 +182,7 @@ float Stream::parseFloat(LookaheadMode lookahead, char ignore)
else if(c >= '0' && c <= '9') { // is c a digit?
value = value * 10 + c - '0';
Copy link
Contributor Author

@edgar-bonet edgar-bonet Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value may overflow to INFINITY on AVR if there are more than 38 digits (maybe that's so many we do not care about that case?), event with a number smaller than FLT_MAX. I suggest instead:

  if(isFraction) {
    fraction *= 0.1;
    value = value + fraction * (c - '0');
  } else {
    value = value * 10 + c - '0';
  }

Note that on something other than AVR one would need more than 308 digits to demonstrate the problem.

Copy link
Contributor

@aentinger aentinger Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this form, however in that case we've got to commit to using a double for value. I personally would not mind since we are talking about stream parsing after all and the only Stream channels currently available are Serial and various networking streams and which are slow even compared to floating point multiplication.

I can further support my argument pro floating point implementation that if you want to run performance-critical/floating-point applications on an 8-Bit architecture you've selected the wrong MCU alltogether.

@facchinm / @cmaglie what's your take on this?

Copy link
Contributor

@aentinger aentinger Jan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I got no feedback over quite a long period of time (unfortunate but not unexpected given what we are currently swamped in) I'm moving forward with merging this PR.

Copy link
Contributor

@aentinger aentinger Jan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's hold on for a second though @edgar-bonet can you integrate your last change suggested above by yourself:

if(isFraction) {
    fraction *= 0.1;
    value = value + fraction * (c - '0');
  } else {
    value = value * 10 + c - '0';
  }

Copy link
Contributor Author

@edgar-bonet edgar-bonet Jan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aentinger: OK, I'll do it today.

if(isFraction)
fraction *= 0.1;
digits_post_comma++;
}
read(); // consume the character we got with peek
c = timedPeek();
Expand All @@ -190,10 +191,11 @@ float Stream::parseFloat(LookaheadMode lookahead, char ignore)

if(isNegative)
value = -value;

if(isFraction)
return value * fraction;
else
return value;
value /= pow(10, digits_post_comma);
Copy link
Contributor Author

@edgar-bonet edgar-bonet Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pow() involves a logarithm and an exponential, which is very expensive on MMU-less processors.


return value;
}

// read characters from stream into buffer
Expand Down
12 changes: 12 additions & 0 deletions test/src/Stream/test_parseFloat.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

#include <StreamMock.h>

#include <float.h>

/**************************************************************************************
* TEST CODE
**************************************************************************************/
Expand Down Expand Up @@ -43,6 +45,16 @@ TEST_CASE ("Testing parseFloat(LookaheadMode lookahead = SKIP_ALL, char ignore =
mock << "\r\n\t 12.34";
REQUIRE(mock.parseFloat() == 12.34f);
}
WHEN ("A float is provided with too many digits after the decimal point")
{
mock << "3.1415926535897932384";
REQUIRE(mock.parseFloat() == Approx(3.141592654f));
}
WHEN ("A float is larger than LONG_MAX")
{
mock << "602200000000000000000000.00";
REQUIRE(mock.parseFloat() == Approx(6.022e23f));
}
}

TEST_CASE ("Testing parseFloat(LookaheadMode lookahead = SKIP_NONE, char ignore = NO_IGNORE_CHAR)", "[Stream-parseFloat-02]")
Expand Down