search

nodemcu / nodemcu-firmware

Lua based interactive firmware for ESP8266, ESP8285 and ESP32
https://nodemcu.readthedocs.io
MIT License
7.64k stars 3.12k forks source link

String formatting: plus sign ignored for %+g #3265

Closed mk-pmb closed 3 years ago

mk-pmb commented 4 years ago

Expected behavior

My number has a plus sign, as on Ubuntu:

$ lua <<<'print(("%+g"):format(123), _VERSION)'
+123    Lua 5.2

Actual behavior

No sign in the REPL on my ESP8266 (using this firmware built with LUA=53 from current dev branch + tmr fix.

Test code

> print(("%+g"):format(123), _VERSION)
123     Lua 5.3

NodeMCU startup banner

NodeMCU 3.0.0.0 built with Docker provided by frightanic.com
        branch: lua53-dev
        commit: 9ff8d9f3e99b5c815b90cc40cb93b0102f80c2dd
        release: 3.0-master_20190907 +104
        release DTS: 202008311952
        SSL: false
        build type: float
        LFS: 0x10000 bytes total capacity
        modules: adc,bit,coap,color_utils,cron,crypto,encoder,file,gpio,gpio_pulse,http,i2c,mdns,mqtt,net,node,ow,pcm,perf,pipe,pwm,pwm2,rfswitch,rotary,rtcfifo,rtcmem,rtctime,sigma_delta,sjson,sntp,spi,struct,tls,tmr,uart,websocket,wifi,wifi_monitor
 build 2020-08-31 19:55 powered by Lua 5.3.5 on SDK 3.0.1-dev(fce080e)
cannot open init.lua:

Hardware

ESP8266 LoLin v3

vsky279 commented 4 years ago

@mk-pmb Can you please check the fix in the PR #3270?

mk-pmb commented 4 years ago

I tested it with this firmware and this program. The results show that now the + is the same on NodeMCU as on Ubuntu's LUA. Thank you for fixing it! :+1:

However, at the same time they indicate they have a different interpretation of the number of spaces when the sign is omitted. Also you might wonder why I used 3.1410 as the number, not 3.1415… that's because they also have different opinions about rounding. Should we extend the issue and PR to tackle that?

vsky279 commented 4 years ago

Thanks for testing. I'll try to have a look at both issues. If there's easy way to fix it it can be done. Maybe we should continue the discussion directly in the PR.

nwf commented 3 years ago

This didn't get picked up as fixed automagically. If it should remain open, please feel free to reopen it!

mk-pmb commented 3 years ago

I tested with this firmware built from commit 64bbf006898109b936fcc09478cbae9d099885a8 = the current dev branch. The results are as from my previous test above.

@vsky279 Since the MR is merged now, I guess it would be confusing to continue the discussion there. Will you make a new MR for the spacing and/or rounding bugs? Shall I open issues for them? If the MR seems easy maybe we should just extend and re-open this issue.

vsky279 commented 3 years ago

You were too fast guys. I suggest reopening this issue rather than opening another one.

I have almost ready the PR for testing for the ' ' flag (e.g. % 8.3f).

As for the rounding - I think it is not something to be corrected. I think the difference @mk-pmb observes between Ubuntu Lua 5.2 and NodeMCU Lua 5.3 is due to the fact that we are using 32bit float while Ubuntu is using 64bit double.

3.1415 does not has precise representation in 32bit float. It is in fact stored as 3.141499996185303 (0x40490e56 = 2.00000000 * 1.5707499980926514) so when rounded to 3 decimals we are still at 3.141. ~~However I don't know how C manage to print 3.1415 instead of 3.141499... Maybe some more experience programmer can elaborate on this.~~ print(3.1415) prints number using %.7g format so 3.1415 prints correctly.

mk-pmb commented 3 years ago

(If anyone else wonders: He's using big endian notation.) Maybe they multiply by 10**precision first, or calculate the next decimal digit and decide based on that. Update: In-depth discussion of sprintf rounding modes. Quote by Vincent Lefèvre:

You get different results because the C standard doesn't specify these cases and the C libraries are different.

Would be waaaay to easy if computer rounding would be the same as we learned in school math.

vsky279 commented 3 years ago

@mk-pmb please test the PR #3283.

mk-pmb commented 3 years ago

Will do, probably tomorrow. Meanwhile: For changing the rounding I think we should ensure a wide consensus, do we have that? Edit: I see it's being discussed in the PR.

vsky279 commented 3 years ago

Definitely there needs to be wide agreement. This is also why I have two separate commits.

mk-pmb commented 3 years ago

Done for ESP8266. Meanwhile I got an ESP32 as well, but it may take another few hours until I can test on that. I can't currently test on that so nevermind.

pjsg commented 3 years ago

I think that this should be solved with the merging of the snprintf implementation. Can you verify?

vsky279 commented 3 years ago

I tested this already. Both flags '+' & ' ' work fine with the new snprintf implementation.

The double rounding - it would be nice to have it implemented. Is it possible to submit a PR on source files provided by a submodule?

pjsg commented 3 years ago

I think that you should submit the PR to the original module....

On Sun, Oct 4, 2020 at 5:50 PM Lukáš Voborský notifications@github.com wrote:

I tested this already. Both flags '+' & ' ' work fine with the new snprintf implementation.

The double rounding - it would be nice to have it implemented. Is it possible to submit a PR on source files provided by a submodule?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/nodemcu/nodemcu-firmware/issues/3265#issuecomment-703321177, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALQLTKCY2BBJF4S74HEC2LSJDUZVANCNFSM4QQ5VJUA .

vsky279 commented 3 years ago

I think that with 64-bit doubles (PR #3225) there's no need to have the double rounding implemented. If someone needed precise rounding with doubles it is achieved ("%.3f"%3.1415 gives 3.142 with #define LUA_NUMBER_64BITS). So think we can close this issue.

nwf commented 3 years ago

Closing by request of @vsky279.