chipKIT32 / chipKIT-core

Downloadable chipKIT core for use with Arduino 1.6 - 1.8+ IDE, PlatformIO, and UECIDE
http://chipkit.net/
Apache License 2.0
59 stars 53 forks source link

dtostrf convertig error #429

Open pgunzelmann opened 6 years ago

pgunzelmann commented 6 years ago

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 commented 6 years ago

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

majenkotech commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

EmbeddedMan commented 6 years ago

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

majenkotech commented 6 years ago

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

EmbeddedMan commented 6 years ago

Very good. Let me know if you have a PR that needs testing and I'll help out. Thanks for working on this Matt.

On Fri, Jun 29, 2018 at 5:51 PM, Majenko Technologies < notifications@github.com> wrote:

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

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/chipKIT32/chipKIT-core/issues/429#issuecomment-401492901, or mute the thread https://github.com/notifications/unsubscribe-auth/AAbeCBPZQtCkrXQUnMSAvj4Ln1woLzChks5uBq96gaJpZM4U3Flh .

majenkotech commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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.