-
-
Notifications
You must be signed in to change notification settings - Fork 345
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
Initial restructure to support multiple target architectures #1675
Conversation
Rearrange files and fix #includes * `Platform` -> `Platform/Esp8266/Platform` * Create `Platform/Common` for shared headers and code * `custom_heap` -> `Platform/Esp8266/System/components/custom_heap` * `compiler` -> `Platform/Esp8266/Compiler` * `appspecific/ * `tools` -> `Platform/Esp8266/Tools` * Esp8266-specific modules from `SmingCore` -> `Platform/Esp2866/Core' * `SmingCore/Network/rBootHttpUpdate*` -> `Platform/Esp2866/Core/Network` * Fix gdb paths - use `gdb/` for always-build, `gdbstub/` for internal * Rename `system` directory to `System` Create Esp8266 system components and group with related submodules * Move Esp8266-specific files from `system` into `Platform/Esp8266/System/components/esp8266` * `appinit/user_main.cpp` -> `Platform/Esp8266/System/components/esp8266/startup.cpp` * `gdb` -> `Platform/Esp8266/System/components/gdbstub` * `appspecific/gdb` -> `Platform/Esp8266/System/components/gdbstub/appcode` * `Services/FATFS` -> `Platform/Esp8266/System/components/fatfs` * third-party libraries: `rBoot`, `new-pwm`, `axtls-8266`, `umm_malloc`, `esp-open-lwip`, `lwip2` * Move `.patch` file moved into submodule parent directory * `system/include/rboot-integration.h`, `appspecific/rboot/overrides.c` -> `Platform/Esp8266/System/components/rboot/appcode` * `Services/SpifFS` -> `Platform/Esp8266/System/components/spiffs` Makefile revisions * Remove `Makefile-project.mk` * Rename `Makefile-rboot.mk` -> `Makefile-app.mk`, but retain `Makefile-rboot.mk` for backward compatibility * Move common environment setup into `build.mk` and add platform-specific directory definitions * Move common target creation code into `modules.mk` * Put ESP8266-specific script into `Platform/Esp8266/` makefiles: `build.mk`, `sming.mk`, `app.mk` and `flash.mk` * Pull related variable definitions and rules together in makefiles * Define macros to replace repetitive script * Ignore errors in all clean operations * Use `TOOLS`, `TOOLS_CLEAN` and `CLEAN` variables containing pre-requisites for `tools`, `tools-clean` and `clean` targets * Remove `include` directory, move `user_config.h` into `Platform/$PLATFORM/System` folder * Distribute PHONY declarations * Add `PLATFORM_TOOLS` path, adjust locations for memanalyzer in host makefiles * Simplify user library building by adding to `LIBS`, each library gets an additional shorthand make target e.g. `make axtls`. * Add `SUBMODULES` variable to identify submodule directories - they're now not all in third-party directory * Use `.submodule` file to simplify reloading * Make `ARDUINO_LIBRARIES` a public variable to optionally restrict which libraries get loaded and built Add `building.md` Fixes: * Add `libpwm_open` to `CUSTOM_TARGETS` so it builds with framework Bugs found: * Testing with `ENABLE_CUSTOM_LWIP=0` fails because of static definitions in `mem_manager.h` - also happens with existing build
Wouldn't it be better to rename it to
Why not
Why not
How about
Also
Interesting. I have double checked this and you are right. That's strange because it used to work... but it is not a big issue because I guess no one has used the Espressif's LWIP since years. |
I've noticed that this PR consistently uses about 60 bytes more IRAM. I tracked this down to the |
... also
|
We've kind of appropriated It's also going to be more important to 'namespace' header files, thus:
This means
I chose to use lower-case
Our
IDF
IDF
OK.
The only thing in
Both |
How's about this:
That just leaves There's also
Not sure about these:
|
Also, -> We might also consider updating from V5.1.1 to version 6? |
Better not. We should not mix our own-code with third-party code. Because it has its own coding styles, own compiler directives, etc. It would be much better to store external, third-party, libraries or components in strict predefined folder(s), for example
ESP8266_NONOS_SDK goes to `Sming/Platform/Esp8266/Components/Sdk/
No, that goes to
Yes, that sounds like a good location.
Maybe
Maybe
Hm... You've got me...
SmingCore/Drivers/Yeelight ? |
Leave Libraries/ArduinoJson in its place. It is part of Sming but also an Arudino Library. No need to move it to Core or Data. At the moment at least.
I will take a look and see if it is a lot of effort to migrate. Hopefully not. |
As a way of dealing with the (Also, ...Or perhaps |
I like both |
At some stage we might introduce sub-architectures, something akin to 'boards' in Arduino, but perhaps more flexible so various system-dependent stuff can be more easily configured. All that is probably much easier to deal with in CMake though... |
`Platform/Esp8266/app.mk` * Move variable statements before `flashinit` and `flash` rules (doesn't work otherwise) `Makefile` * Add `mkdir` for `libraries-clean` to improve reliablity (sometimes fails) * Add `submodules` and `submodules-clean` rules to replace `third-party` and `libraries` rules * Prioritise rules in `dist-clean`, put sample cleaning last
* Relocate submodules and tidy `.gitmodules` file * Move libsming definitions and MQTT flag into `build.mk` as they apply to both framework and application builds * Move SDK into `Arch/Esp8266/Sdk` and update to use Version 3 un-patched release
* Fix `decode-stacktrace.py` so pasting content works, using `raw_input()` instead of `input()`
With the above changes, we've got quite a clean set of 'root' directories:
Note that I think it probably best to leave I also have further changes to the makefiles, the goal being to document and clarify the existing build system to make it easier to migrate (e.g. to CMake), rather than adding anything new. |
Perfect.
Yes, I agree with you.
Meanwhile the migration to ArduinoJson v6 is ready for testing #1677. The ArduinoJSON v6 API looks more elegant and intuitive. But we should check also the memory usage. |
* Document relevant targets using `##` comments, groups with `##@` * Tag configuration variables using `CONFIG_VARS` so values can be examined (using `list-config`) * Add `help` and `list-config` targets * Add `rebuild` target for framework (app already has one) * Move `kill_term`, `terminal` and `decode-stacktrace` targets into `Makefile-app.mk` * Use CURDIR instead of fabricating CURRENT_DIR * Add `FixPath` function to deal with Windows/POSIX path conversion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work! Only few small but important things that will help/guide current users to migrate to v4 from 3.8.x.
@@ -1,5 +1,5 @@ | |||
#include <user_config.h> | |||
#include <SmingCore/SmingCore.h> | |||
#include <SmingCore.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should provide slightly smoother path to migrate to v4. Therefore I would suggest the following:
- Create a Wiki page with the migration effort needed to migrate from v3.8.x to v4 (migrate-v3.8-v4.0)
As a start include just the update of the include: from<SmingCore/SmingCore.h>
to<SmingCore.h>
- Create SmingCore directory and put only SmingCore.h file in it.
SmingCore/SmingCore.h should
-> include Core/SmingCore.h.
-> Emit a #warning message that informs the user to update his include
-> And shows a link to the wiki migration page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I note you renamed the Wiki page for this, now the link is broken!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for noticing. Fixed with #1681.
#include "SmingCore/SmingCore.h" | ||
#include "SmingCore/Network/SmtpClient.h" | ||
#include <SmingCore.h> | ||
#include <Network/SmtpClient.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The migration wiki should also include "recommended" way to update includes that are from SmingCore. Example:
"SmingCore/Network/SmtpClient.h"
-> <Network/SmtpClient.h>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the new file/folder layout has been fixed, before committing I'll need to do the following:
- Fix
#include
guard naming - Update all
#include
s to use the recommended/namespaced convention
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix #include guard naming
Or just use #pragma once. And then you will not need to fix them in the future.
I guess I forgot to mention that I like a lot the new structure. IMHO it is more intuitive than it used to be. So definitely a good way forward. |
Something else we need to deal with is I guess the characteristic of the file is that it (usually) gets #included ahead of anything else so is a good place to put overrides. How about this as a template for users to follow:
|
Seems good to me. But I would prefer to have it as a separate PR so that it is, as you wrote once, "easier to digest". |
The |
Leave it as it is for the moment. |
…order-only pre-requisites
…i migration page
…_input()` for consistent behaviour across python versions
…y with non-GNU `awk` implementations
As per discussion #1333.
Rearrange files and fix #includes:
SmingCore
->Core
SmingCore/Platform
->Platform
system
->System
Services/libb64
->Components
Services/WebHelpers
->Core/Network
apptest
Platform
sources ->Arch/Esp8266/Platform
SmingCore
sources ->Arch/Esp2866/Core
compiler
->Arch/Esp8266/Compiler
tools
->Arch/Esp8266/Tools
SmingCore/Network/rBootHttpUpdate
->Arch/Esp2866/Core/Network
custom_heap
->Arch/Esp8266/Components
system
files ->Arch/Esp8266/Components/esp8266
(reflects IDF layout)appinit/user_main.cpp
->Arch/Esp8266/Components/esp8266/startup.cpp
gdb
->Arch/Esp8266/Components/gdbstub
appspecific/gdb
->Arch/Esp8266/Components/gdbstub/appcode
Services/FATFS
->Arch/Esp8266/Components/fatfs
system/include/rboot-integration.h
,appspecific/rboot/overrides.c
->Arch/Esp8266/Components/rboot/appcode
Services/SpifFS
->Arch/Esp8266/Components/spiffs
include/user_config.h
->Arch/Esp8266/System/include
Makefile revisions
Makefile-project.mk
Makefile-rboot.mk
->Makefile-app.mk
Makefile-rboot.mk
(redirects toMakefile-app.mk
) for backward compatibilitybuild.mk
contains host environment setupARCH_BASE
,ARCH_SYS
,ARCH_CORE
,ARCH_COMPONENTS
andARCH_TOOLS
modules.mk
contains rule creation scriptArch/*/build.mk
included bybuild.mk
fileArch/*/sming.mk
included byMakefile
Arch/*/app.mk
included byMakefile-app.mk
Arch/*/out
TOOLS
,TOOLS_CLEAN
andCLEAN
variables containing pre-requisites fortools
,tools-clean
andclean
targetsLIBS
, each library gets an additional shorthand make target e.g.make axtls
.decode-stacktrack
rule to application makefilehelp
target to extract formatted makefile commentslist-config
targetSubmodule handling
rBoot
,new-pwm
,axtls-8266
,umm_malloc
,esp-open-lwip
,lwip2
->Arch/Esp8266/Components
.patch
file into related submodule parent directoryComponents/Sdk
and updated to use unpatched Version 3.0 releasethird-party
submodules intoComponents
SUBMODULES
variable to specify which submodules are required.submodule
file as consistent pre-requisite for update/patching of all submodulesARDUINO_LIBRARIES
a public variable to optionally restrict which libraries get loaded and builtsubmodules
andsubmodules-clean
rules to replacethird-party
andlibraries
rulesOther
building.md
help
andlist-config
targetsFixes:
libpwm_open
toCUSTOM_TARGETS
so it builds with frameworkBugs found:
ENABLE_CUSTOM_LWIP=0
fails because of static definitions inmem_manager.h
- also happens with existing build