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

Move to Kconfig-like build configuration #3130

Closed
nwf opened this issue May 28, 2020 · 20 comments
Closed

Move to Kconfig-like build configuration #3130

nwf opened this issue May 28, 2020 · 20 comments
Labels

Comments

@nwf
Copy link
Member

nwf commented May 28, 2020

user_modules.h and user_config.h are a little unfortunate. Among other things, this style of configuration means:

  • users express their configuration preferences by directly modifing files managed by git
  • we always build everything
  • we occasionally have to paw through C headers looking for particular strings with regexes (e.g., in tools/update_buildinfo.sh)

We should move to something like Kconfig, where we articulate the dependencies and parameters in a machine readable language and have tools generate a .config file that everything downstream can source easily. Because .config is merely a list of assertions, with no computational content, there's less risk that things go wrong (e.g. the use of #if 0 in user_modules.h will cause a discrepancy between actually included modules and the list of included modules).

We can use this to speed up builds (skipping mbedtls, for example, if there's no need for it, and not building modules not requested) but more significantly the improved articulation of configuration will let tools beyond C consume it and let us, for example, decide which Lua module files land in LFS/SPIFFS more readily, I think.

https://pypi.org/project/kconfiglib/ is a full Python implementation (and so works on Windows) and is already used by esp-idf upstream.

This should be very straightforward, just time consuming and invasive across the tree, with impacts on the various build tools (esp. web image builder). Given the invasiveness, if there is consensus that this is a good idea, I propose we wait until the 5.3 work has landed, first.

@TerryE
Copy link
Collaborator

TerryE commented May 28, 2020

@nwf Nathaniel, @jmattsson Johny, this also overlaps with our discussions on #1661. What Johny and I were suggesting is to move our ESP8266 builds to an ESP-IDF (or variant) build environment with as much as the folder tree unified across the two branches. OK, our ESP8266 builds will stil need to build an ESP8266 non-OS SDK based firmware image rather than an ESP32 RTOS based one, but the more of the core components and build processes that are shared across the two variants the better, IMO.

But quite a bit more planning needed before we start along this path, I think.

@nwf
Copy link
Member Author

nwf commented May 28, 2020

Given that I'd like to move us in the direction of Lua+C modules sooner rather than later, I think pursuing this sooner rather than later is probably a good idea. The alternatives all necessitate teaching builders about the dependencies anyway, right? If we want "click button foo, get firmware image with foo.lua in LFS/SPIFFS/both and foo.c compiled in" to work.

@jmattsson
Copy link
Member

The next item on my TODO list now that the 5.3 stuff is merged, is indeed to switch the 8266 branch to use an IDF style build. I already prepared a non-OS SDK IDF here, and a small sample app to verify the linking etc. If you or someone else wants to help shift the esp8266 build into this format, I wouldn't turn down the help - the details are outlined in point 2 of the proposed way to merge 8266 & 32 branches (aka discussion 9). Item 3 deals with transitioning our user_config.h and user_modules.h into the IDF (aka kconfig) framework, which is pretty much what you're proposing here.

@nwf
Copy link
Member Author

nwf commented May 29, 2020

Awesome! I'll try to carve out some time this weekend. :D

@nwf
Copy link
Member Author

nwf commented May 30, 2020

Given that we have both in-C and in-Lua modules, would you @jmattsson accept a patch to the esp32 branch that renamed the symbols in https://github.com/nodemcu/nodemcu-firmware/blob/dev-esp32/components/modules/Kconfig from LUA_MODULE_* to C_MODULE_*, so that LUA_MODULE_* could refer to lua_modules/?

@jmattsson
Copy link
Member

Sure, but in that case you'll also need to update the macro in module.h as it's using that LUA_MODULE_xxx to construct the symbols used by the linker for generating the internal array of c modules, plus component.mk in modules/ for related reasons. I think those two are the only occasions, other than the obvious Kconfig files and the <module>.c files themselves, but please double (or triple) check :)

@TerryE
Copy link
Collaborator

TerryE commented May 30, 2020

so that could refer to lua_modules/?

This one doesn't make sense to me. The LUA_MODULE_* defines are in a C namespace. They are Lua modules, albeit coded in C and using the Lua C API. You can't reference the Lua source modules from the C world.

Unless, of course, we add some form of NodeMCU specific introspection.

@nwf
Copy link
Member Author

nwf commented May 30, 2020

@TerryE The proposal is to move configuration out of C and into Kconfig (or, rather, to derive C configuration from Kconfig), so there will be Kconfig-namespace symbols for both in-C modules from app/modules (or wherever it relocates) and in-Lua modules from lua_modules (or ...). In that context, LUA_MODULE_foo is a confusing name for app/modules/foo.c.

@TerryE
Copy link
Collaborator

TerryE commented May 30, 2020

Sorry. I don't agree. LUA_ and lua_ (and NODEMCU_) are prefixes denote a namespace reserved for the Lua / NodeMCU environments. LUA_MODULE_ does not imply a module written in Lua but rather the MODULE sub space of the LUA runtime environment and that is exactly where these components should be declared. If you want to differentiate between C and Lua modules then perhaps LUA_CMODULE_ would make sense, but C_ definitely doesn't as these still form part of the Lua runtime ecosystem.

@nwf
Copy link
Member Author

nwf commented May 31, 2020

Alright, point taken. How about NODEMCU_LMODULE_ and NODEMCU_CMODULE_ for the namespace prefixes?

@TerryE
Copy link
Collaborator

TerryE commented May 31, 2020

Not going to argue about that one.

@jmattsson
Copy link
Member

Ah, and I think I recall seeing @marcelstoer mention that the cloudbuilder supports esp32 now, so @nwf, you'll probably need to provide a patch for that one too to go in at the same time as the esp32 branch PR.

And NODEMCU_{C,L}_MODULE is fine with me too. My initial choice of Kconfig variable name was simply in order to minimise the changes I had to make when moving things from user_modules.h to Kconfig for the esp32. There were enough things to juggle getting the initial port done, so I went with the easy route for that part.

@marcelstoer
Copy link
Member

I think I recall seeing @marcelstoer mention that the cloudbuilder supports esp32 now

It's been live since mid February and appears to work well. I keep the scripts here for anyone who cares: https://github.com/marcelstoer/nodemcu-custom-build/tree/master/ESP32

@TerryE
Copy link
Collaborator

TerryE commented May 31, 2020

If we are going to do this then why not drop the USE_ bit? So we would have NODEMCU_CMODULE_WIFI etc. In this case the _ separates the namespaces, so it doesn't name sense overloading it as a space after C.

@nwf
Copy link
Member Author

nwf commented May 31, 2020

USE_ is not in the existing esp32 branch nor was I going to put it in mine, yes.

nwf added a commit to nwf/nodemcu-firmware that referenced this issue May 31, 2020
Using the NODEMCU_ namespace prefix makes it obvious that these are not
part of Lua proper (contrast, e.g., LUA_BUILTIN_STRING).  Using
"CMODULE" gives us room to differentiate between modules whose
implementation is in C and whose implemenation is in Lua ("LMODULE").

The ESP8266 branch can adopt the same convention when it moves to
Kconfig; see nodemcu#3130
@nwf nwf mentioned this issue May 31, 2020
12 tasks
nwf added a commit to nwf/nodemcu-firmware that referenced this issue May 31, 2020
Using the NODEMCU_ namespace prefix makes it obvious that these are not
part of Lua proper (contrast, e.g., LUA_BUILTIN_STRING).  Using
"CMODULE" gives us room to differentiate between modules whose
implementation is in C and whose implemenation is in Lua ("LMODULE").

The ESP8266 branch can adopt the same convention when it moves to
Kconfig; see nodemcu#3130
@marcelstoer
Copy link
Member

marcelstoer commented May 31, 2020

How about NODEMCU_LMODULE_ and NODEMCU_CMODULE_ for the namespace prefixes?

That string is already long enough (fine for me, I like expressive identifiers) that I see no point in abbreviating "LUA" to "L". Hence, my proposal is NODEMCU_LUA_MODULE_ and NODEMCU_C_MODULE_ to make things really clear.

@nwf
Copy link
Member Author

nwf commented May 31, 2020

I think @TerryE objected to NODEMCU_LUA_MODULE and NODEMCU_C_MODULE since _ is the namespace separator. I'm content with NODEMCU_LUAMODULE_, tho', and really, I'm happy to let y'all bikeshed this one, so long as there's an answer in short order.

@TerryE
Copy link
Collaborator

TerryE commented May 31, 2020

The Lua runtime already uses the C and L prefixes, e.g. CClosure and LClosure, so this is following in a good tradition.

@marcelstoer
Copy link
Member

I'm happy to let y'all bikeshed this one

Not me. I despise everything that's ineffective use of time. @nwf @TerryE Feel free to choose any prefix you deem sensible.

marcelstoer pushed a commit that referenced this issue Aug 23, 2020
Using the NODEMCU_ namespace prefix makes it obvious that these are not
part of Lua proper (contrast, e.g., LUA_BUILTIN_STRING).  Using
"CMODULE" gives us room to differentiate between modules whose
implementation is in C and whose implemenation is in Lua ("LMODULE").

The ESP8266 branch can adopt the same convention when it moves to
Kconfig; see #3130
@nwf nwf mentioned this issue Feb 3, 2021
4 tasks
@stale
Copy link

stale bot commented Jun 2, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 2, 2021
@stale stale bot closed this as completed Jun 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants