Restructure flag inheritance in platformio.

[robertlipe]

I'm tired of watching huuuuuge lists of files build that I know won't be
used. I've not shaved everything to the bone, but I've cut the cases
that are big enough to annoy me out of the builds I care about. For
example, waiting for it to check out and build that ridiculous
olikraus/U8g2 library build up to 37 times on a full build is a drag.

Anything that used to set USE_SCREEN now uses ${use_screen.build_flags}
to get -DUSE_SCREEN and ${parent-device.lib_flags} to get the
dependencies and the -L ... -l stuff. I similarly tried to tighten up
the remote_flags case, upon which this was inspired.

There's still a bit of silliness, such as everything checking out and
building BUTTONS when almost none of our builds HAVE buttons...but that
lib builds quickly enough to not annoy me. Now checking it out and
storing it 38 times isn't awesome.

The way we have configuration split between globals.h and platform.ini
is definitely sub-awesome. I wish I could figure out how to make
Platformio's extra_script.py to look at the -D flags in CPPFLAGS and
then add_libs based on that. Then when globals.h consitionally adds
a -DFoo, we could add_libs Foo without it having to appear in globals.h
It's pretty silly to wait and checkout and build AsyncTCP or WebServer
if the configuration didn't even turn on ENABLE_WIFI, but we do

If your build is broken, it's probably this PR's fault. Buzz me with a
repro case for help if needed. Notably, this tightens many environments
to use the "correct" hardware lib_deps (which may include extra
libraries as it might have been extended) instead of those of base_libs.

Tested:
Successful python3 tools/build-all.py

Description

Contributing requirements

• I read the contribution guidelines in CONTRIBUTING.md.
• I understand the BlinkenPerBit metric, and maximized it in this PR.
• I selected main as the target branch.
• All code herein is subjected to the license terms in COPYING.txt.

[rbergen]

@robertlipe Thanks for submitting this. The segmented inheritance within platformio.ini is totally in line with the pattern I implemented in a massive overhaul of platformio.ini some time ago, so I obviously can't disagree with that. (That exercise aimed to significantly decrease repetition in the env blocks, not to decrease the inclusion of unnecessary dependencies.)

I've given the specific change some thought, and I'd like to propose a few small changes:

1. Rename the USE_SCREEN define to HAS_SCREEN or SUPPORTS_SCREEN; the flag now indicates if we have the bits on board to be able to use it, so I think it's good to reflect that in the name of the thing. Of course, the various #if USE_SCREEN etc. lines throughout the codebase then have to be updated accordingly.
2. Add a similar HAS_REMOTE or SUPPORTS_REMOTE define for the remote control scenario. We'd then have to add that to the #if ENABLE_REMOTE lines, turning them into #if HAS_REMOTE && ENABLE_REMOTE. Without this, it would now become possible to enable the remote control using its define when the stuff that's needed to make it actually work isn't there.

Let me know your thoughts on this.

[robertlipe]

I suspect we (and anyone else that stares at it for very long) agree that our platformio is sub-awesome.

I get wanting to use preprocessor for everything, but it clutters the namespace and I've tried to make https://docs.platformio.org/en/latest/scripting/index.html (and related) look for (handwaving) -DWANT_FOO in order to add lib_deps += libfoo; I just can't figure out how to make that work well. IMO, we should drive MUCH more configuration into the platform.ini (ick) where it's in build space and can be fondled and monitored by python and scons and out of preprocessor-land.

But that's not the total point of this.

Sure, I can do a global search and replace of USE_SCREEN with HAS_SCREEN. It didn't really meet Blinkenbit guideline, but I don't hate adding that onto the load here. I slightly prefer the active verb form we have now (just because we HAVE a screen doesn't mean we want to USE that screen) but I really won't dig in my heels on that. I'll submit a global search and replace in a bit.

The more I dig into our REMOTE handling, the more screwy I think it is. globals.h actually turns it ON unless it's turned OFF elsewhere.

NightDriverStrip/include/globals.h

Line 1391 in b3f87ba

#ifndef IR_REMOTE_PIN

but it's sort of turned off here

NightDriverStrip/include/globals.h

Line 1213 in b3f87ba

#ifndef ENABLE_REMOTE

.

The result is that remote is MOSTLY included in all the builds (and I hate, hate, hate waiting for it to check out and build dozens of 900KB files that are effectively empty) when the reality is that our ENABLE_REMOTE flags - especially after this change, mostly turns ON the -DDONT_COMPILE_THE_ENTIRE_PLANET flag, meaning that it's the builds that don't ENABLE_REMOTE or set remote_flags AT ALL that get penalized most by compiling dozens of those 900KB files that are basically #if 0'ed away. But that's another scab I was reluctant to pick too deeply at.

So I thought about it and really don't have a great plan for that last one. On one hand, we want plat*ini to use remote_flags for the lib_deps because otherwise we end up sucking in and compiling that giant remote library that we may or may not want. OTOH, we want -DENABLE_REMOTE to be lightweight in special developer pet projects (ala

NightDriverStrip/include/globals.h

Line 347 in b3f87ba

#define ENABLE_REMOTE 1 // IR Remote Control

) where we can rely on having the lib_deps (and without it, the library won't be checked out)

So I ack point 1 and agree that a rename to SUPPORTS_SCREEN, while not my favorite, is a reasonable additional request for this PR and will push it soon. If I fall asleep before you QA and merge this, I'll agree to come back and "fix" that. But then, we should similarly think about USE_HUB75, USE_OLED, USE_M5DISPLAY, USE_TFTPSPI, and a bunch of others, too. My custom board (see external communication on the same) might be a devkit-c that I've attached a 1306 (USE_OLED, see related pins) and want an effect of either SPECTRUM or MESMERIZER. Requiring FOUR configurations seems overkill.

I acknowledge point 2 is a problem, but it's deeper than a superficial one. The push and pull between platini and globh is intense and I don't know how to find a happy place for it. It's self-evident that copy-pasting blocks in glob*h is easy (too easy) but that it's a problem just because we have so many internally inconsistent projects. For example, the effect SPECTRUM is jumping through #ifdef hoops to try to be happy with multiple host boards like WROVER and ELECROW and such ... when PIN0 should be a trait of a host board (an env or probably even a dev) and definitely not an effect like "spectrum".

REALLY, we want remote_foo in platformio.ini. Otherwise, we have to pull in - and thus, build - the library everywhere. This is how we end up with out builds checking out and building 38 copies of the remote library. OTOH, I totally understand the tempting precision of adding another #ifdef block in *globals.h that precisely defines pins, libraries needed (danger!) and such.

Since our remote_flags is the thing that turns OFF the -DCOMPILE_THE_WORLD options and most of our targets end up neededing REMOTE_FLAGS to build, there's a perverse incentive that it's the project that absolutely declared NO interest in needing a remote that get swept into the defaults and thus building with a lib_deps of remote_flags, but not getting the build_flags of remote_flags. This patch makes that better, but it's hard to mentally disprove all the cases that are still pulling in the giant remote builds.

I think this change is a solid step forward.
If a global name change is needed to get it accepted, I'm OK with that - now or in a followup.
I think we ultimately need to change our internal naming to separate (perhaps) chip/module, board, and effect. That's going to be painful.
I wish someone with scons and platformio superpowers magically appeared and could look at our configuration tension between platformio and globals.h and recommend a solid plan forward. Our current plan isn't great and is getting worse as we grow. It seems that hints are there in construction, actions, and middleware sections of https://docs.platformio.org/en/latest/scripting/index.html, but I'm not not getting how we can NOT have configuration in two places...easily.

Can I push back on #2 and agree to keep thinking about it until we have a solid plan forward that lets us disentangle the platform.ini and the globals.h representations? It's messy. (If I'm misunderstanding, you're free to type more slowly until I see an easy way that actually makes it simpler and not more complicated.)

I'll absolutely admit that I keep flying into the gravity of our configuration mess (sorry, not sorry) and alternately thinking that I can do better or getting sucked into the sun and burning. I've really struggled with supporting the "Soldering Iron Crowd" and the normies and trying to find configurations that work for both and haven't yet found a happy place.

[rbergen]

Can I push back on 2 and agree to keep thinking about it until we have a solid plan forward that lets us disentangle the platform.ini and the globals.h representations?

I'm sorry but no, not if you do want to apply selective inclusion of the remote control library dependency in this PR. The reason is this: we now remove the ability to use the remote by not including the necessary library dependency for certain configurations, and that will cause weird problems when people then set (or leave) ENABLE_REMOTE to 1 in those builds. Unless they have their head around the entire dependency/define/... stack and know the library may just not be there because of this new thing in platformio.ini, they won't have a clue why the thing isn't working when ENABLE_REMOTE says it should.

My statement is basically this: if we selectively include library dependencies for certain capabilities in platformio.ini, we have to tell the code that's being built what we have, and thereby what we don't. That's what the HAS/SUPPORTS (I understand you prefer the latter, which is fine with me) defines are for. That's also where they (should) differ from the USE variety - those should indicate what is chosen to be used. I can definitely imagine that other defines (USE or otherwise) should also be converted to HAS/SUPPORTS, but that can indeed be a follow-up.

In a sense, selectively including (or excluding) lib_deps adds another layer of complexity to the multiple levels that are already embedded in the platformio.ini/globals.h combo. I could have raised that as an objection to this change as a whole, but I didn't as I do agree this is a sensible step to take. We just can't abandon the rest of the setup until we fix it, regardless of how icky we think it now looks.

So, as far as I see it, we can do one of three things:

1. Add the SUPPORTS_REMOTE flag and check for it in places that matter, on top of whatever else is already there, or
2. Clean up the ENABLE_REMOTE and other IR remote defines throughout the project now - possibly replacing ENABLE_REMOTE with SUPPORTS_REMOTE and killing the former with fire altogether -, or
3. Postpone the selective inclusion of the relevant library dependency in platformio.ini and leave it in the global list for now. We can then pull the selective inclusion back in when we get around to 2.

[revision29]

I'll add my two cents. Thanks for cleaning this up. I can guarantee this change will break my setups, but not sure how. I needed to restart with a fresh fork anyway.

I do have reservations about changing ENABLE_REMOTE to SUPPORTS_REMOTE. It is an input feature I want to enable, just like the web server. It is not natively supported by most boards since it requires adding an additional piece of hardware (IR diode). Logically, supports_remote is not true for most boards. ENABLE_REMOTE makes more sense to me. It is a small point in the midst of the greater issue being discussed above. But, it should not be enabled by default.

I can appreciate the need for these modifications. Wrapping my brain around the interplay between globals.h and platformio.ini took a while. There is a lot of redundancy and inconsistency in what features are enabled where. Sometimes web server is enabled in plat*.ini and sometimes in globals.h.

[robertlipe]

I've been trying to come up with a way forward on this and haven't yet succeeded. Adding MORE flags is the opposite of a goal for me. Removing nonsensical components from the build (and that agonizing fetch) is. Fetching and building that UG8 library for mesmerizer, for example, is a total waste of electrons. I agree, Joe, that having it in a flag in global namespace but that is meaningfully examined in only three source files (which could be lowered) but thus requires a recompilation of the world on change isn't great. But, like UG8, these libraries are so darned large that building them - and hopefully throwing them away - isn't great, either. The current state where remote is rarely used, included in all builds, but only sometimes paired with the flag that makes the build less annoying is just super weird. One of the tensions (that I've been pulling at for most of my duration here) is that we effectively have build systems within a build system. We ALREADY have cases where the flags are out of sync. Moving that for any project not represented in the build - and I spent the time to ensure that all included builds worked - from a runtime cost to a build failure actually seems like a direction forward to me. I would rather move this forward and move ALL the remote flags, including the one that sets the pin for those boards that ship with IR sensors, into platformio.ini and out of globals.h totally. I'm pretty much out of love with any hardware details inside globals.h. We already have IR_REMOTE_PIN, and ENABLE_REMOTE. Adding a HAS_REMOTE doesn't help our support problem. If anything, I'd vote to remove the defaultl IR_REMOTE_PIN 25 condition which turns it on for everyone, making IR_REMOTE_PIN && !define ENABLE_REMOTE a hard error that's tested and enforced quickly, and move the IR_REMOTE_PIN to the board configuration definitions to the DEVICE definitions in the cases where the pin is known because it ships on the hardware. For those people taking bare modules and building them up, they know they have to build a device configuration that describes their device, turning on ENABLE_REMOTE and USE_PIN. But I don't have the dozen boards in question and am sure I'd take the fall for messing up one of them. After all, I think if you're taking a bare board (I think you're with me in that category, Joe) and wiring up sensors and maybe a display and maybe buttons and such, you're building a new DEVICE definition and should be prepared to provide an accurate definition of your pin configurations (including things like [ABCDE_PIN]) there instead of having it sprinked between MatrixConfig and globals and having to #undef this or that. Maybe we remove the default for IR_REMOTE_PIN and use the presence of THAT as our global. Then we'd at least have one knob instead of thee. Building the world and hoping the linker can sort it out at the same time you're struggling with memory consumption isn't a great situation to be in. Like web server, it's tempting to build it in and make it checkbox enabled in the GUI, but there's a substantial code size/build time cost. (I'm also against building ESPAsyncWebServer in every configuration, even when ENABLE_WEBSERVER isn't present, but unlike these two, it builds quickly enough that I don't rage against it when it builds. MOST of lib_deps is not used in MOST of our actual builds. I just didn't want to try to tackle them all at once and from the reaction so far, it's a good thing. So we may keep building QRCode in demo and other zany combinations. Unless I can come up with some approach that doesn't add to the number of flags instead of lowering them and overall improving the consistency, I probably won't move forward with it. It'll probably stay in my private tree which might just become a permanent fork as I find the saved compilation time totally worth it. I'm just not happy with the direction this is being asked to go. I'm still kicking this around but I won't proceed with it until we can agree on a way forward that I think is actually forward. I think few of us are happy with the current situation. None of us yet have produced a clear step forward. We have things that are clearly effects settings, some that are clearly hardware settings, some that are user preferences, some that are vestigial (I just noticed that MILLIS_PER_FRAME is never defined to NOT be '0' - which doesn't seem good), some that are project settings, etc. all mixed together. If you wanted to build an UMBRELLA (display) on an ESP32-S3 (psram with appropriate flags), for example, I think you'll spend as much time #undefing or just changing existing flags as you will wiring it up. For example, ONBOARD_LED_foo will be wrong if your ONBOARD_LED is the commonly provided WS2812 and it'll mess with hardware pins that you might not be expecting to be taken. The SPECTRUM (display) makes even more greedy assumptions about the hardware layout. I keep thinking about this a LOT as I tend to help the DIY crowd a lot. (I know there are others on both axes there...) It's really messy and so far we've not found a clearly better way forward. That's why I've been chipping at making it a little more consistent and a little less annoying in small steps. On this PR, I may fail, but this change is useful enough that I may keep something like it in my work trees going forward.

…

On Sat, Nov 18, 2023 at 9:17 PM Joe Schneider ***@***.***> wrote: I'll add my two cents. Thanks for cleaning this up. I can guarantee this change will break my setups, but not sure how. I needed to restart with a fresh fork anyway. I do have reservations about changing ENABLE_REMOTE to SUPPORTS_REMOTE. It is an input feature I want to enable, just like the web server. It is not natively supported by most boards since it requires adding an additional piece of hardware (IR diode). Logically, supports_remote is not true for most boards. ENABLE_REMOTE makes more sense to me. It is a small point in the midst of the greater issue being discussed above. But, it should not be enabled by default. I can appreciate the need for these modifications. Wrapping my brain around the interplay between globals.h and platformio.ini took a while. There is a lot of redundancy and inconsistency in what features are enabled where. Sometimes web server is enabled in plat*.ini and sometimes in globals.h. — Reply to this email directly, view it on GitHub <#500 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD377AQOLCOA3KH5MA3DYFF23PAVCNFSM6AAAAAA7HOD5ZWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJXG4ZTCMRYHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>