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

dtostrf convertig error #429

Open
pgunzelmann opened this issue Jun 26, 2018 · 13 comments
Open

dtostrf convertig error #429

pgunzelmann opened this issue Jun 26, 2018 · 13 comments

Comments

@pgunzelmann
Copy link

example simplified to show only the problem:

void setup() {
  // put your setup code here, to run once:
  double d=1.04;
  Serial.begin(9600);
  Serial.println(String(d));
}

void loop() {
  // put your main code here, to run repeatedly:

}

Leads as output at serial using ATmega2560 Meaga to
1.04
but on chipKIT MAX32 to

1.40000

i tried to work arround useing the dtostrf function, but unfortunately the same occurance.

please try to simulate and may confirm or to reject

@JacobChrist
Copy link
Member

JacobChrist commented Jun 26, 2018

Nice find. Do you know if correct expected output is documented anywhere on Arduino.cc? I have not attempted to confirm this yet.

@majenkotech
Copy link
Member

dtostrf is an avr-libc specific (non-standard) function. We emulate it (or try to), but it uses the same code as is used by String. (actually, String just calls dtostrf).

That simply uses snprintf to format the number into a string.

Print::print, on the other hand uses its own internal printFloat() function. Maybe we should replace our dtostrf function with something based around that function for consistent results?

@majenkotech
Copy link
Member

majenkotech commented Jun 26, 2018

Actually, looking at it, it's not quite that simple. I mis-read your values - 1.4000 != 1.04...!

So this looks like the formatting isn't working, and is dropping leading zeros from the fractional part (it splits it into integer and fractional components then prints "%d.%d" of them). Maybe that second %d should be %0d... And then maybe erase trailing zeroes...

@pgunzelmann
Copy link
Author

Yes this is exactly why i opened the issue, i written an interpollation function and had wondering why it is not correct. Do to String funktion also seem to use the dtostrf function it should be improved.

@JacobChrist as majenkotech mentioned the return values are incorect, indepent from Arduino.cc

@JacobChrist
Copy link
Member

Ah, I missed that. Formatting is not the issue.

@EmbeddedMan
Copy link
Member

It would be great to have a fix for this. Anybody have time to submit a PR?

@majenkotech
Copy link
Member

I'd like to port dtostrf from avr-libc if possible.

@EmbeddedMan
Copy link
Member

EmbeddedMan commented Jun 29, 2018 via email

@majenkotech
Copy link
Member

Unfortunately, dtostrf relies heavily on __ftoa_engine, which is written in AVR assembly. So that's a bit of a non-starter really. Shame. It would have been a good way to get 100% compatibility.

However I have just come across this implementation which is in pure C, using sprintf. Could be improved to use snprintf instead for greater security... and maybe improved formatting.

@majenkotech
Copy link
Member

I'm sure it can be shrunk down to just this:

char *dtostrf(double __val, signed char __width, unsigned char __prec, char *__s) {
    snprintf(__s, __width, "%.*f", __prec, __val);
    return __s;
}

There would be a more efficient way of doing it that doesn't rely on snprintf, such as taking portions of doprint, but for now it should get us out of a hole. I'll test it now.

@majenkotech
Copy link
Member

Ok, trimming is not needed, the Arduino defaults to 2 decimal places only, which threw me. Also snprintf needs an extra space in the width for NULL which dtostrf doesn't include. So with a bit of trial and error it's now formatting the same (ish).

@EmbeddedMan
Copy link
Member

@pgunzelmann @JacobChrist Matt just pushed up some new code that should completely fix this issue. Can either of you confirm that the changes fix the reported issue?

@EmbeddedMan
Copy link
Member

I haven't heard back from anybody that Matt's change fixed things for you, but I'm going to assume that it did, so I merged the change in. A new core version will be coming shortly which will include this change, so I'll close the issue then.

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

5 participants
@JacobChrist @EmbeddedMan @majenkotech @pgunzelmann and others