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

Kconfig: rename LUA_MODULE_* to NODEMCU_CMODULE_* #3135

Merged
merged 1 commit into from
Aug 23, 2020

Conversation

nwf
Copy link
Member

@nwf nwf commented May 30, 2020

As we begin to contemplate baking into firmware modules entirely written
in Lua or which are split between both Lua and C, the name LUA_MODULE_*
more obviously refers to the in-Lua bits, so rename the C ones out of
the way.

  • This PR is for the dev-esp32 branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/*.

I have not yet convinced my machine to build for the esp32, but I think this is probably about right. Still, someone checking for me would be welcome.

This is part of #3130, indirectly, at least in keeping with the idea of harmonizing the two branches.

@nwf nwf requested a review from jmattsson May 30, 2020 18:08
@TerryE TerryE self-requested a review May 30, 2020 21:09
@TerryE
Copy link
Collaborator

TerryE commented May 30, 2020

Please see my discussion on #3130. This change makes no sense to me as these are Lua runtime modules, albeit written in C. If we really want to emphasise the "C-ness" of these then LUA_CMODULE would make more sense.

@nwf nwf force-pushed the dev32-rename-kconfig branch from 1aeb853 to 1131687 Compare May 31, 2020 12:42
@nwf nwf changed the title Kconfig: rename LUA_MODULE_* to C_MODULE_* Kconfig: rename LUA_MODULE_* to NODEMCU_CMODULE_* May 31, 2020
nwf added a commit to nwf/nodemcu-custom-build that referenced this pull request May 31, 2020
components/lua/lua.h Outdated Show resolved Hide resolved
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 force-pushed the dev32-rename-kconfig branch from 1131687 to ff6c471 Compare May 31, 2020 15:21
@nwf nwf requested a review from marcelstoer May 31, 2020 15:21
@nwf nwf added the ESP32 label May 31, 2020
Copy link
Member

@marcelstoer marcelstoer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nwf @TerryE this is now ready, isn't it?

@nwf
Copy link
Member Author

nwf commented Jun 24, 2020

I think it's ready, but it'd be good for someone with an ESP32 environment to look over my shoulder.

@marcelstoer marcelstoer mentioned this pull request Jul 9, 2020
4 tasks
@marcelstoer marcelstoer merged commit dd3b217 into nodemcu:dev-esp32 Aug 23, 2020
marcelstoer pushed a commit to marcelstoer/nodemcu-custom-build that referenced this pull request Aug 23, 2020
@nwf nwf deleted the dev32-rename-kconfig branch August 23, 2020 21:59
marcelstoer pushed a commit to marcelstoer/nodemcu-custom-build that referenced this pull request Sep 15, 2020
@@ -6,9 +6,9 @@
#define NODE_VERSION_REVISION 0U
#define NODE_VERSION_INTERNAL 0U

#define NODE_VERSION "NodeMCU ESP32"
#define NODE_VERSION "NodeMCU ESP32" " built with Docker provided by frightanic.com\n\tbranch: dev-esp32\n\tcommit: f4887bf134235c05e6c9b2efad370e6d5018f91a\n\tSSL: true\n\tmodules: -\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arghh, missed this one...shouldn't have ended up in this change set.

marcelstoer added a commit that referenced this pull request Sep 17, 2020
Undos changes from #3135
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants