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

Convert .ino -> .cpp #2228

Merged
merged 34 commits into from
Apr 30, 2020
Merged

Convert .ino -> .cpp #2228

merged 34 commits into from
Apr 30, 2020

Conversation

mcspr
Copy link
Collaborator

@mcspr mcspr commented Apr 24, 2020

resolve #1306
@ali80

General approach is to:

  • add espurna.h with base headers included, each module-specific (relay.h, gpio.h) should include it
  • module-specific header can hide library includes under ifndefs, which I assume would have effect on PIO ldf mode when we switch it to chain+ or deep (https://docs.platformio.org/en/latest/librarymanager/ldf.html)
    The goal is to avoid building unnecessary libs, which is what ldf seems to promise
  • .cpp includes module-specific include at the top, thus including espurna.h and now we can add ifndefs with dependencies
    Perhaps, we can avoid it and just stuff everything into espurna.h list, thus everything includes everything at once.

Not every module loads espurna.h header, so CI should fail now with the dependency issue described above.

@mcspr
Copy link
Collaborator Author

mcspr commented Apr 25, 2020

+7.5KB in size, can't exactly pin-point the reason though. Hopefully it is settings in-lining or I missed something like with ota.h, will check it out.
Build time increased dramatically in tests, 10sec vs 28sec.

@ali80
Copy link

ali80 commented Apr 25, 2020

+7.5KB in size, can't exactly pin-point the reason though. Hopefully it is settings in-lining or I missed something like with ota.h, will check it out.
Build time increased dramatically in tests, 10sec vs 28sec.

Even for a single change, like in main.cpp?!

I have another project with esp8266 with bin size of 500kb, all with cpp files,

some measurements on my system:

  • building without any change would take 9s, no recompilation of any files, all 9s spent on dependency checking
  • building with single change in main.cpp only results in recomilation of that file which takes less than 1 second and obviously linking which totals to 12s (9s LDF, 0.5s comile, 2.5s linking)
  • building espurna without any change, 9s LDF, 15s recomilation of main.ino.cpp (even without any change) and 2s linking totals to 26s

observations:

  • pio doesn't care about change in main.ino file and always merge and recompile it which can take a lot of time if you have a lot of ino files.
  • if you omit the ino files, LDF seems to take the most amount of time which I dont know how much improvement is possible but I think is slow at the moment, compared to similar sized projects on something like Keil
  • the actual compile time for small changes is under 1s usually

I think this approach of including everything in everything is not good coding practice, maybe for a short term transition period, but I cant wrap my head around the whole code architecture of espurna so my opinion is worth nothing at the moment :)

@mcspr
Copy link
Collaborator Author

mcspr commented Apr 26, 2020

The existing configuration lives in config/all.h. Since this file is always included through espurna.h any change to config will trigger rebuild on dependant files. We cache intermediate .o's:

os_env = os.environ.copy()
os_env["PLATFORMIO_SRC_BUILD_FLAGS"] = "-DUSE_CUSTOM_H"
os_env["PLATFORMIO_BUILD_CACHE_DIR"] = "test/pio_cache"
cmd = ["platformio", "run", "-s", "-e", args.environment]

PIO purges build directory on every .ini or build_flags change. test_build.py sets SRC_BUILD_FLAGS to include custom.h file and we re-use the same environment to produce different binaries

1st build (libs are already installed)

$ ./scripts/test_build.py --no-silent -e esp8266-4m-git-base -n -a test/build/basic.h
...
> test/build/basic.h: 568208 bytes, 0:01:02.787344

2nd build, not changing anything:

Compiling .pio/build/esp8266-4m-git-base/src/espurna.ino.cpp.o
> test/build/basic.h: 568208 bytes, 0:00:06.237935

3rd, adding #define ADMIN_PASS "fibonacco" in test/build/basic.h

> test/build/basic.h: 568208 bytes, 0:00:24.388921

Nothing is retrieved from cache :/ Previous .ino build was taking at most 10 seconds in the same conditions.
Since every file depends on config/all.h, each file gets rebuilt when we change something depending on it (in this case, custom.h). SRC_BUILD_FLAGS environment has the same effect, every project's .cpp is rebuilt.

I just wonder how exactly we can change the configuration in such a way that we have each .cpp depending on a specific .h. Perhaps having a single configuration header is not the way to go? If configuration is moved to a module header, for example, we can theoretically avoid the issue. We do spread configuration across multiple files though

@mcspr
Copy link
Collaborator Author

mcspr commented Apr 26, 2020

In any case, does this resolve the Intellisense issue?

@ali80
Copy link

ali80 commented Apr 27, 2020

I just wonder how exactly we can change the configuration in such a way that we have each .cpp depending on a specific .h. Perhaps having a single configuration header is not the way to go?

Having all the configuration in one place is very convenient for those who have little to no knowlege of the codebase but want their own custom build, IMO it is worth sacrificing some compile time, and this is only when config file changes which isn't that often.

What I think is the main issue is too many modules depending on each other, which makes the code harder to understand, introduce bugs and makes well written/tested useful modules unusable outside espurna project

I think the problem is mainly in two categories

  • global defines (ifdef used for configuration selection and removing unsused options)
  • global functions and variables which could be accessed from everywhere

For global defines, I think each module should have a default configuration, so If no options are provided it still should know what to do by default, eg the telnet module should have

#ifndef TELNET_PORT
#define TELNET_PORT             23
#endif

in its telnet.h file,

On how to change the default options, one way would be to use compile time defines using -D flags in platformio.ini file, or still define them in another .h (easier to modify and see all the options toghether) file but this time to make module reusable outside espura with something like this

#ifdef DEFAULT_CONFIG_OVERRIDE
#include "global_configs.h"
#endif

Also it would be preferred if the option for the module can be set by a class constructor or init function instead of the #define

@ali80
Copy link

ali80 commented Apr 27, 2020

In any case, does this resolve the Intellisense issue?

It seems to be working so far, but project wont build giving this

 (project configuration file): 'No section: 'env''

from the platformio.ini file

@mcspr
Copy link
Collaborator Author

mcspr commented Apr 27, 2020

It seems to be working so far, but project wont build giving this

 (project configuration file): 'No section: 'env''

from the platformio.ini file

Hm. I can build the project via Build task, no issue there. platformio.ini is the file, there is [env] section:


(https://github.com/mcspr/espurna/tree/refactoring/ino-cpp checked out in a new dir, just to be sure)

@mcspr mcspr changed the title [wip] .ino -> .cpp Convert .ino -> .cpp Apr 28, 2020
@mcspr
Copy link
Collaborator Author

mcspr commented Apr 30, 2020

Just to be sure, I also tried arduino-cli to build:

$ readlink -f ~/Arduino/hardware/esp8266com/esp8266 # linked from PIO git environment
/home/builder/.platformio/packages/framework-arduinoespressif8266@src-31d658a59f41540201fc3726a1394910
$ ls ~/Arduino/libraries/ # linked from the PIO installation, too
ArduinoJson@  AsyncMqttClient@  EEPROM_Rotate@  Embedis@  ESPAsyncTCP@  ESP Async WebServer@  FauxmoESP@  JustWifi@
$ arduino-cli compile --verbose --build-properties=compiler.cpp.extra_flags="-DITEAD_SONOFF_BASIC -DALEXA_SUPPORT=0" -b esp8266com:esp8266:generic:eesz=4M
/home/builder/Arduino/hardware/esp8266com/esp8266/tools/xtensa-lx106-elf/bin/xtensa-lx106-elf-size -A /tmp/arduino-sketch-DAA6A8A9C45835668D8B6E797077EEEA/espurna.ino.elf
...
Sketch uses 561968 bytes (53%) of program storage space. Maximum is 1044464 bytes.
Global variables use 38532 bytes (47%) of dynamic memory, leaving 43388 bytes for local variables. Maximum is 81920 bytes

@mcspr mcspr merged commit edb23db into xoseperez:dev Apr 30, 2020
@mcspr mcspr deleted the refactoring/ino-cpp branch April 30, 2020 10:55
@mcspr
Copy link
Collaborator Author

mcspr commented Apr 30, 2020

I will look into headers configuration next, this is only for conversion itself. Although, flat config file seems to be the choice for some of embedded projects that I know of:
https://www.nongnu.org/lwip/2_1_x/group__lwip__opts.html
https://www.freertos.org/a00110.html
https://www.lua.org/source/5.3/luaconf.h.html

@ali80
Copy link

ali80 commented May 3, 2020

Excellent job, everything seems to be working so far with very fast build times :)
I also agree that flat configurations are much easier to understand and modify for those unfamiliar with the project

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

Successfully merging this pull request may close these issues.

Intellisense in Platformio IDE (VS Code)
2 participants