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

Avoid float-double-conversion #7559

Merged
merged 1 commit into from
Aug 28, 2020
Merged

Avoid float-double-conversion #7559

merged 1 commit into from
Aug 28, 2020

Conversation

dirkmueller
Copy link
Contributor

Converting floats to doubles is very expensive on esp8266, so prefer
calculations or comparisons as float. This saves 10% (20 bytes) of the
String::parseFloat() code size and probably quite a bit of runtime
overhead.

@d-a-v
Copy link
Collaborator

d-a-v commented Aug 28, 2020

float fraction = 1.0; has been missed

Converting floats to doubles is very expensive on esp8266, so prefer
calculations or comparisons as float. This saves 10% (20 bytes) of the
String::parseFloat() code size and probably quite a bit of runtime
overhead.
@dirkmueller
Copy link
Contributor Author

@d-a-v thats just initialization, which seems to be handled fine by the compiler. anyway, fixed, thanks!

@earlephilhower
Copy link
Collaborator

earlephilhower commented Aug 28, 2020

Can you please build an example that uses these calls w/ and w/o this change so we can measure the code differences and report the full flash usage (i.e. the IROM/IRAM/BSS/DATA block at the end of build)

@dirkmueller
Copy link
Contributor Author

so there are examples that use the code, but they're not run in the ci checks. no idea why :-(

Here's what I did to reproduce:

add a (nonsensical) usage:

--- a/libraries/esp8266/examples/TestEspApi/TestEspApi.ino
+++ b/libraries/esp8266/examples/TestEspApi/TestEspApi.ino
@@ -242,6 +242,7 @@ void setup() {
   // messages are also readable.
 
   Serial.begin(74880);
+  Serial.parseFloat();
 
   // Try pushing frequency to 160MHz.
   system_update_cpu_freq(SYS_CPU_160MHZ);

then build it:

$ pio ci ./libraries/esp8266/examples/TestEspApi --board nodemcuv2
...
RAM:   [===       ]  33.9% (used 27752 bytes from 81920 bytes)
Flash: [===       ]  25.5% (used 265836 bytes from 1044464 bytes)
.pio/build/nodemcuv2/firmware.elf  :
section                                        size         addr
.data                                          1248   1073643520
.noinit                                           4   1073644768
.text                                           232   1074790400
.irom0.text                                  236132   1075843088
.text1                                        26656   1074790632
.rodata                                        1568   1073644784
.bss                                          24936   1073646352
.comment                                       5065            0

then I applied the patch to ~/.platformio/packages/framework-arduinoespressif8266/cores/esp8266/Stream.cpp
and built again:

$ pio ci ./libraries/esp8266/examples/TestEspApi --board nodemcuv2
...
RAM:   [===       ]  33.9% (used 27752 bytes from 81920 bytes)
Flash: [===       ]  25.4% (used 265804 bytes from 1044464 bytes)
.pio/build/nodemcuv2/firmware.elf  :
section                                        size         addr
.data                                          1248   1073643520
.noinit                                           4   1073644768
.text                                           232   1074790400
.irom0.text                                  236100   1075843088
.text1                                        26656   1074790632
.rodata                                        1568   1073644784
.bss                                          24936   1073646352

@devyte devyte merged commit 953dfd9 into esp8266:master Aug 28, 2020
@devyte devyte added this to the 3.0.0 milestone Aug 28, 2020
davisonja added a commit to davisonja/Arduino that referenced this pull request Sep 10, 2020
* master: (299 commits)
  Fix error message typo (esp8266#7581)
  Update certs-from-mozilla.py (esp8266#7578)
  Update DigestAuthorization.ino (Simple example update) (esp8266#7579)
  Fix gzip+signed OTA error (esp8266#7577)
  Properly replace toolchain in PlatformIO CI script (esp8266#7580)
  Update certs-from-mozilla.py (esp8266#7573)
  Fixup weird combination of oneline/multi line comments (esp8266#7566)
  Reduce codesize of setOutputPower (esp8266#7572)
  Fix typos in tests
  Force gcc inlining, use same style for getCycleCount as for getCpuFreqMHz.
  Even more concise #if form.
  Inline, fewer LOC, remove redundant definition in cpp.
  Netump Initial commit (esp8266#7527)
  Delete owner field (esp8266#7563)
  Avoid float-double-conversion (esp8266#7559)
  Use direct member initialization instead of ctr initialisation (esp8266#7556)
  Add CI test for eboot build (esp8266#7546)
  getCpuFreqMHz(): fix when F_CPU is not defined (esp8266#7554)
  emulation-on-host makefile update, allowing to pass more options (esp8266#7552)
  add sdk options to "generic esp8285 module" (esp8266#7550)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants