Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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.

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 arduino:master from edgar-bonet:many-digits
Jan 25, 2021
Merged
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Exchanging the type of 'value' from 'long' to 'double' prevents overf...
...low when parsing float values.
However, we still need to ensure against too large values contained in streams. This should be possible because the maximum length of a float value pre-comma is known to be 38 digits (FLT_MAX_10_EXP).
  • Loading branch information
aentinger committed Dec 14, 2020
commit fae13e5f5324295118cf57104c69c3b087f7d64b
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

AltStyle によって変換されたページ (->オリジナル) /