WLED icon indicating copy to clipboard operation
WLED copied to clipboard

Fetch supported LED types from Bus

Open netmindz opened this issue 1 year ago • 39 comments

First of 2 different PRs to move the hard coding of all the different LED output types and their config from the current settings_led.htm to be fetched from the various Bus classes

This is so that when you add a new Bus, you do not need to edit the settings_led.htm, which is a neater workflow that also allows for easier use of compile-time flags for which output types are available

See the motivation for this change - https://github.com/Aircoookie/WLED/pull/3777

netmindz avatar Jul 14 '24 17:07 netmindz

This is great! Thank you. Could you move strings to PROGMEM? That would reduce RAM pressure on ESP8266.

blazoncek avatar Jul 14 '24 20:07 blazoncek

References to PROGMEM seem to all talk about it's usage in relation to const char, which I am not using at all

netmindz avatar Jul 15 '24 09:07 netmindz

PROGMEM is a storage specifier. It directs compiler (and linker) to store variable in flash instead of RAM (and hence const). It has to be used on ESP8266 for strings (especially long ones) or available RAM will suffer.

blazoncek avatar Jul 15 '24 09:07 blazoncek

Change made to the strings @blazoncek

netmindz avatar Jul 15 '24 17:07 netmindz

Hang fire on the merge, I'm seeing an issue with 8266

oappend() buffer overflow. Cannot append 1127 bytes

netmindz avatar Jul 16 '24 12:07 netmindz

Fixed

netmindz avatar Jul 16 '24 12:07 netmindz

@netmindz the issue with oappend() (stack) buffer is resolved only temporary as buffer content may increase in the future. I know you've fought stack buffer issue in MM fork. Perhaps we should move getting LED types into a separate HTTP request in the future to reduce stack pressure. It would be an easy change.

blazoncek avatar Jul 17 '24 19:07 blazoncek

I cannot push directly to this branch, please edit PR to allow push.

blazoncek avatar Jul 17 '24 20:07 blazoncek

Actually, if this really is a constant thing that you're building at compile time and you're not later adding or removing members, maybe this shouldn't be a std::vector at all, but rather a std::array. Then you can definitely make that compile-time initializaed and all that code that handles calling constructors adding to the end and reallocating the vector as you push_back and such will all just decay away.

I'm on a phone, but I'll try to pull the branch and see if I can help later. I think a Std:array will help with the code size.

Is esp32 under platdoemio with a modern g++ a reasonable development place or is this like FastLED that cares about univac and 8 bit hosts? (For all those 8266s that can drive six HUB75 panels, you know...)

On Wed, Jul 17, 2024 at 9:15 AM netmindz @.***> wrote:

@.**** commented on this pull request.

In wled00/bus_manager.cpp https://github.com/Aircoookie/WLED/pull/4056#discussion_r1681136930:

@@ -377,6 +377,86 @@ void BusDigital::cleanup() { pinManager.deallocatePin(_pins[0], PinOwner::BusDigital); }

+std::vector<LEDType> BusDigital::getLEDTypes() {

That's way over the head of my C++ knowledge sorry

— Reply to this email directly, view it on GitHub https://github.com/Aircoookie/WLED/pull/4056#discussion_r1681136930, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD3253OIU2TQSVVL4FATZMZ36ZAVCNFSM6AAAAABK3KHH4WVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOBTGA3DMOJYG4 . You are receiving this because you commented.Message ID: @.***>

robertlipe avatar Jul 17 '24 20:07 robertlipe

permission has already been granted what error are you getting when you try to push @blazoncek ?

netmindz avatar Jul 17 '24 21:07 netmindz

I welcome any refactoring that helps reduce the code, the work so far was a bit of a transposition from HTML to C++ where my main focus was on the accuracy of conversion rather than optimisation

netmindz avatar Jul 17 '24 21:07 netmindz

It says "You don't have permissions to push to "netmindz/WLED" on GitHub. Would you like to create a fork and push to it instead?" and git push netmindz produces 403 error (forbidden). Perhaps the issue lies in the fact that you forked from MM.

I usually have no issues when modifying PR branches like that in other repositories:

  • add remote repo (https://github.com/netmindz/WLED.git)
  • checkout PR branch
  • commit
  • push to selected remote

blazoncek avatar Jul 18 '24 04:07 blazoncek

@robertlipe you are right all this is compile time operation and does not need vectors per se. The construction of JSON string may benefit from that but I am unsure how to transfer array into vector (lack of knowledge).

I've modified code and will provide links to my Dropbox if you PM me on Discord until @netmindz fixes the push issue.

blazoncek avatar Jul 18 '24 05:07 blazoncek

Thinking of it, it may really be unnecessary to extend BusXXX classes with getLEDTypes() method (in the context of this PR at least). Something like this might suffice:

String BusManager::getLEDTypes() {
  std::array<LEDType, 27> types = {{
    {TYPE_WS2812_RGB, "D", PSTR("WS281x")},
    {TYPE_SK6812_RGBW, "D", PSTR("SK6812/WS2814 RGBW")},
    {TYPE_TM1814, "D", PSTR("TM1814")},
    {TYPE_WS2811_400KHZ, "D", PSTR("400kHz")},
    {TYPE_TM1829, "D", PSTR("TM1829")},
    {TYPE_UCS8903, "D", PSTR("UCS8903")},
    {TYPE_APA106, "D", PSTR("APA106/PL9823")},
    {TYPE_TM1914, "D", PSTR("TM1914")},
    {TYPE_FW1906, "D", PSTR("FW1906 GRBCW")},
    {TYPE_UCS8904, "D", PSTR("UCS8904 RGBW")},
    {TYPE_WS2805, "D", PSTR("WS2805 RGBCW")},
    {TYPE_WS2812_1CH_X3, "D", PSTR("WS2811 White")},
    //{TYPE_WS2812_2CH_X3, "D", PSTR("WS2811 CCT")},
    //{TYPE_WS2812_WWA, "D", PSTR("WS2811 WWA")},
    {TYPE_WS2801, "2P", PSTR("WS2801")},
    {TYPE_APA102, "2P", PSTR("APA102")},
    {TYPE_LPD8806, "2P", PSTR("LPD8806")},
    {TYPE_LPD6803, "2P", PSTR("LPD6803")},
    {TYPE_P9813, "2P", PSTR("PP9813")},
    {TYPE_ONOFF, "", PSTR("On/Off")},
    {TYPE_ANALOG_1CH, "A", PSTR("PWM White")},
    {TYPE_ANALOG_2CH, PSTR("AA"), PSTR("PWM CCT")},
    {TYPE_ANALOG_3CH, PSTR("AAA"), PSTR("PWM RGB")},
    {TYPE_ANALOG_4CH, PSTR("AAAA"), PSTR("PWM RGBW")},
    {TYPE_ANALOG_5CH, PSTR("AAAAA"), PSTR("PWM RGB+CCT")},
    //{TYPE_ANALOG_6CH, PSTR("AAAAAA"), PSTR("PWM RGB+DCCT")},
    {TYPE_NET_DDP_RGB, "V", PSTR("DDP RGB (network)")},
    {TYPE_NET_ARTNET_RGB, "V", PSTR("Art-Net RGB (network)")},
    {TYPE_NET_DDP_RGBW, "V", PSTR("DDP RGBW (network)")},
    {TYPE_NET_ARTNET_RGBW, "V", PSTR("Art-Net RGBW (network)")}
  }};
  String json = "[";
  for (const auto &type : types) {
    String id = String(type.id);
    json += "{i:" + id + F(",t:\"") + FPSTR(type.type) + F("\",n:\"") + FPSTR(type.name) + F("\"},");
  }
  json.setCharAt(json.length()-1, ']'); // replace last comma with bracket
  return json;
}

(or moved outside to be static)

@netmindz what was your driving force for vector implementation? In #4058 we can see that you also added information needed for UI (description for pins) but that is hardly a justifiable reason. Or am I wrong?

I played with this PR yesterday and it is the step in the right direction, unfortunately oappend() does not seem to be the correct approach as it is very vulnerable to overflow, except if another request is made just for types (current condensed JSON object uses almost 1000 bytes which is a lot for 8266 2k buffer).

blazoncek avatar Jul 18 '24 11:07 blazoncek

The motivation for the change is that if you look at the hub75 branch, you will see how the list of LEDTypes is dependent on which Bus classes you include in the build, so you can use a hard-coded list of LED types directly in the BusManager::getLEDTypes, the bus manager must always delegate to the individual Bus classes.

We can perhaps use an array within the specific Bus classes as the size of the number of LED types it supports is fixed

netmindz avatar Jul 18 '24 13:07 netmindz

I'd like to invite @Aircoookie into discussion as he still has a Settings pages rewrite in mind. 😄

blazoncek avatar Jul 18 '24 13:07 blazoncek

which Bus classes you include in the build

Isn't that a "build time" decision? So the array can be built the same.

blazoncek avatar Jul 18 '24 13:07 blazoncek

Yes it is build time, but you can't for example set the array to be size 27 but then 28 if this particular build flag is enabled without it all getting really messy

netmindz avatar Jul 18 '24 14:07 netmindz

std::array is somewhat new in C++. It's not been there forever. It's not wizardry; it's just new. It's pretty much a C array, wrapped up to act like a vector, so you can do .size() without the ARRAYSIZEOF() noise we've done for decades. You can check for next(), iterate with back(), set it with fill(), access it with data() (though that's a tad silly) and basically do templatey/iteratory things.

Yes, Blaz, this is very much what I was thinking. Sometimes having three dozen bytes of code running constructors, making copies, and reallocating a vector for dozen bytes that are constants can be saved - especially since ITT, size was a concern. std:array is much like a plain ole C array that you can treat a vector (or any other STL object), as it still lets you iterate sensibly, as your sample demonstrates. I was just trying to offer up a recipe to help save the cost of the push_back and the internal reallocations for what's essentially a static data table. vectors are cool, but sometimes cheap, static data tables are still the prescription. (Plus, in ESP32-class devices, we live in a world where flash is cheap and runtime clock cycles and RAM aren't. Smart data with dumb code is a useful rule in our world.)

Netmindz is right that It IS crazy annoying that you have to size the std:array in the declaration AND the definition. There's a solution for that below courtesy class template argument deduction...that thankfully hides the ugliness of CTAD itself.

Combine std::array with designated initializers like {.count = 3, .name = "foo" } if you have a sparsely filled table so you're not counting commas or later adding a comma to everything just to keep everything in the right place. It really helps with the brittleness of arrays. (C99's long had DI.)

If heap fragmentation is a concern, and I understand that Arduino's String is bad at this, you might consider <<'ing into a std::stringstream which will hold pointers to the data and then flatten out those references when you slurp them out, such as via a ss.str(). (That makes the .str() expensive, but the << cheap.) Perhaps better still, and also from a new page in C++, std::format might amortize out building pointers to three byte strings. It's quite readable to build those formatted JSON pieces like you've written. The tension between Arduino code and non-trivial C++ in embedded is often a landmine, so measure that if it matters.

Anyway, that's my tiny little contribution to painting this bikeshed. I saw one potential solution to the battle of size vs. flexibility. It's up to those of you that actually know what you NEED of this code to decide if this hammer fits any of your nails.

I'm excited to see WLED for HUB75. I have an Adafruit Matrix S3 available for testing when this gets to a buildable state that's ready for extra eyes.

Thank you for all the work on this and related code!

P.S. If you really do have a table that needs to be conditionally compiled, the way to handle that is with std::to_array() It's still in std::experimental::to_array if you're not fully up to date as I think that's c++20. I think that's something like

String BusManager::getLEDTypes() { static constexpr auto types = std::to_array<LEDType> ({ {TYPE_WS2812_RGB, "D", PSTR("WS281x")}, #if SUPERSIZE {TYPE_SK6812_RGBW, "D", PSTR("SK6812/WS2814 RGBW")}, #endif // {TYPE_TM1814, "D", PSTR("TM1814")}, {TYPE_WS2811_400KHZ, "D", PSTR("400kHz")} } ); should make that with two elements. I'll admit I have to look up the syntax when I need it.

On Thu, Jul 18, 2024 at 10:00 AM netmindz @.***> wrote:

Yes it is build time, but you can't for example set the array to be size 27 but then 28 if this particular build flag is enabled without it all getting really messy

— Reply to this email directly, view it on GitHub https://github.com/Aircoookie/WLED/pull/4056#issuecomment-2236800073, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD34H3QKAORPPI2BQ4KTZM7KALAVCNFSM6AAAAABK3KHH4WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZWHAYDAMBXGM . You are receiving this because you were mentioned.Message ID: @.***>

robertlipe avatar Jul 18 '24 18:07 robertlipe

Well now. You've tossed more than I can handle. :smile: I don't do coding for living and I only learned C++ from Stroustrup's original book back in the 90's. Yet I learn as I go.

If we don't need iterators or .size(), etc. (and I don't see that we do) we can just use:

const LEDtype types[] = {{...}};
for (auto i=0; i<sizeof(types)/sizeof(LEDtype); i++) {
...
}

Don't we?

blazoncek avatar Jul 18 '24 19:07 blazoncek

I'm self-taught, too, but I spent >13 years in C++ at Google, so I was steeped in this stuff for a long time. Now in retirement/disability, I have time to read C++ proposals and ratifications and such but it still doesn't mean I understand (or have use for...) all of it. We all learn from each other.

If plain old C arrays work for you, they're still awesome. They're cheap and they're well known. Your sizeof() is the ARRAYSIZE thing I mentioned. This is certainly less tortured (sorry) than the push_back() for every element in the original.

They're not really any simpler (with the to_array) approach and if you DO need STL methods and templating, it's a wash. Being able to call accumulate, binary_search, sort check struct equality via == or whatever without having to keep up with the size is convenient, but programmers have gotten by for decades without that safety and convenience. The classes don't decay to a pointer when passed as an argument. Performance is identical.

std:array is largely just a groucho nose disguise over C arrays that let them work with STL conventions.

The designated initializer thing is just handy if you have a struct that doesn't have initializers for every member. If you're initializing a sparse structure, designated initializers will initialize everything ELSE to zero, just like ",,," will - and will keep everything together if you later add a structure member.

If this PR used const C arrays, I'd have let it go without a word. It's the code to initialize const data that's the losing move..especially since size was mentioned as a concern earlier.

I'd prefer std::array over c arrays, but those are fine if you dont' need STL or are in heavily C-accented (Arduino, ahem) code - but if you don't need STL, you sure don't need std::vector and an emplace_back() for each member, which was the original point of my fussing and I think we're all (?) agreeing there.

If you know a lot about your data (does everything have an initializer?) and the intended use (in embedded, you almost always do - in a Google-scale monorepo, you almost +never+ do) and have access to all the callers, you can go with whichever you like.

I'm not sure I've seen a revised PR, but I think we're mostly in agreement that the style:

ledType.id = 33; ledType.name = F("TM1914"); result.push_back(ledType);

ledType.id = 28; ledType.name = F("FW1906 GRBCW"); result.push_back(ledType);

ledType.id = 29; ledType.name = F("UCS8904 RGBW"); result.push_back(ledType);

should be a table of some kind and not code.

Oh, and LEDType .type and .name can be plain ole const char*'s. No need to be Arduino strings in storage. The less Arduino anything, the better. :-)

On Thu, Jul 18, 2024 at 2:28 PM Blaž Kristan @.***> wrote:

Well now. You've tossed more than I can handle. 😄 I don't do coding for living and I only learned C++ from Stroustrup's original book back in the 90's. Yet I learn as I go.

If we don't need iterators or .size(), etc. (and I don't see that we do) we can just use:

const LEDtype types[] = {{...}};for (auto i=0; i<sizeof(types)/sizeof(LEDtype); i++) { ... }

Don't we?

— Reply to this email directly, view it on GitHub https://github.com/Aircoookie/WLED/pull/4056#issuecomment-2237373133, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD3ZOGGCJZMNP3RLRPI3ZNAJMPAVCNFSM6AAAAABK3KHH4WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZXGM3TGMJTGM . You are receiving this because you were mentioned.Message ID: @.***>

robertlipe avatar Jul 18 '24 20:07 robertlipe

Now that's interesting! This works OOB.

  const LEDType types[] = {...};
  for (const auto &type : types) {
  ...
  }

Is this some auto C++ magic?

EDIT: Indeed it looks like compiler silently converting LEDType [] into std::array. As if I change types into a pointer it starts complaining about missing .begin() and .end() methods.

blazoncek avatar Jul 19 '24 11:07 blazoncek

Interesting indeed! I'd have lost that bet, so now I'm the student.

I think that goes down smoothly because of the aforementioned Class Template Argument Deduction, CTAD. The initializer list is a variadic parameter pack (an infinitely long list of garbage in {} collections that's inside the initializer list of a {} pair) and the construction is allowed to treat it as if we'd used make_array, as I'd called out earlier. Because it knows the type, getting a const ref of that type is easy, however it does NOT go ahead and promote it to a std::array (that might imply an allocation behind your back, so it's loathesome to do so, despite what Rustaceans might evangelize) , so I doubt you could reference types.size(), types.begin(), types.end() and such. The deduced type for |type| is probably a const struct LEDType&, so it's enough like a pointer that it's iterable, but not a class, so you won't have STL methods available on it.

If you'd declared that const std::array types(std::to_array<LEDType>({ { ... }, { ... } });

I think you'd have had the best of both worlds and zero costs and only a little splatter from the C++ bloodletting.

I think. I could be wrong on the reasons, but I find that decl and that loop better than several of the alternatives mentioned here. I wouldn't fuss about that. (I'm not an owner/reviewer. My fussing means nothing. I know this... )

Citation? https://en.cppreference.com/w/cpp/container/array/deduction_guides

I like it. It's zero code to initialize the struct and it's difficult to screw up the loop so that you iterate too many or too few..

Sold!

On Fri, Jul 19, 2024 at 6:52 AM Blaž Kristan @.***> wrote:

Now that's interesting! This works OOB.

const LEDType types[] = {...}; for (const auto &type : types) { ... }

Is this some auto C++ magic?

— Reply to this email directly, view it on GitHub https://github.com/Aircoookie/WLED/pull/4056#issuecomment-2238977935, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD33VELSP2YVIMQMRKDLZND4X3AVCNFSM6AAAAABK3KHH4WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZYHE3TOOJTGU . You are receiving this because you were mentioned.Message ID: @.***>

robertlipe avatar Jul 21 '24 04:07 robertlipe

@netmindz is there still interest to push this forward? Unfortunately I cannot push changes made locally as you use MM for as your base but I sent you modified files if you want to include them (adding the information from last few messages).

blazoncek avatar Aug 05 '24 19:08 blazoncek

Sorry I've not had any time to look at this properly over the last few weeks. I think the point we had got to was that some of the suggested optimisations made sense for the first stage of these related PRs, but then would face issues when they try and take further, so premature optimisation in the first PR would then cause issues later so I was going to try and sit down and see what improvements we can make to this PR that don't compromise the later ones

netmindz avatar Aug 05 '24 21:08 netmindz

It sounds like there had been quite a lot of discussion about this and I'm very much a novice with C++ so would it perhaps make sense for perhaps @robertlipe to create a new PR with the same goal where the bus manager to return the details from all the bus implementations. Take my idea, but not my code?

netmindz avatar Aug 05 '24 21:08 netmindz

I'd be happy to help with the editor exercise of pushing the bits around, but my WLED internal mojo is weak. (I'm a regular contributor on a very similar project, so it's not alien.) I can build/run on a variety of ESP32 platforms. Can you give me some guidance on how to appropriately exercise this PR? e.g. "build and run this on native to confirm absence of memory leaks and build and run on ESP32 with configuration X and confirm that saved JSON files before doing X and Y are identical" or that the web interface works or whatever.

I have no idea what a BusManager in this context actually IS, though I seem to have strong opinions about how to spell it internally. :-)

On Mon, Aug 5, 2024 at 4:16 PM netmindz @.***> wrote:

It sounds like there had been quite a lot of discussion about this and I'm very much a novice with C++ so would it perhaps make sense for perhaps @robertlipe https://github.com/robertlipe to create a new PR with the same goal where the bus manager to return the details from all the bus implementations. Take my idea, but not my code?

— Reply to this email directly, view it on GitHub https://github.com/Aircoookie/WLED/pull/4056#issuecomment-2269932965, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD35XES7PJ5ZAPAE4N5LZP7TSBAVCNFSM6AAAAABK3KHH4WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRZHEZTEOJWGU . You are receiving this because you were mentioned.Message ID: @.***>

robertlipe avatar Aug 05 '24 22:08 robertlipe

Actually, honestly, the meat of this is so mechanical that I can probably just take a swing at it and we can work on making it work together.

ISTR that WLED has strong opinions about branch management and doesn't like PRs against the trunk. What's the preferred delivery branch to base it on? CONTRIbzUTING.md says "Please make all PRs against the 0_15 branch." but I also know that doc (including my own) sometimes fibs or this may be targeted to a different branch or entwined with some other work or something.

I can't even remember if I got here because I expressed an interest in the HUB75 work or if it was just random nerd-sniping drive-by in the open PR queue. I sometimes read open issues and help coach (some people call that "butting in" and "running off his mouth where it wasn't welcome" :-) ) others just to randomly help projects I care about.

On Mon, Aug 5, 2024 at 5:36 PM Robert Lipe @.***> wrote:

I'd be happy to help with the editor exercise of pushing the bits around, but my WLED internal mojo is weak. (I'm a regular contributor on a very similar project, so it's not alien.) I can build/run on a variety of ESP32 platforms. Can you give me some guidance on how to appropriately exercise this PR? e.g. "build and run this on native to confirm absence of memory leaks and build and run on ESP32 with configuration X and confirm that saved JSON files before doing X and Y are identical" or that the web interface works or whatever.

I have no idea what a BusManager in this context actually IS, though I seem to have strong opinions about how to spell it internally. :-)

On Mon, Aug 5, 2024 at 4:16 PM netmindz @.***> wrote:

It sounds like there had been quite a lot of discussion about this and I'm very much a novice with C++ so would it perhaps make sense for perhaps @robertlipe https://github.com/robertlipe to create a new PR with the same goal where the bus manager to return the details from all the bus implementations. Take my idea, but not my code?

— Reply to this email directly, view it on GitHub https://github.com/Aircoookie/WLED/pull/4056#issuecomment-2269932965, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD35XES7PJ5ZAPAE4N5LZP7TSBAVCNFSM6AAAAABK3KHH4WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRZHEZTEOJWGU . You are receiving this because you were mentioned.Message ID: @.***>

robertlipe avatar Aug 05 '24 22:08 robertlipe

@robertlipe modified @netmindz 's files are here. I've employed what was said above and it seems to work.

blazoncek avatar Aug 06 '24 10:08 blazoncek

Thanx. Those contain a lot of changes beyond what's in this pr. I'm pretty happy with that structure creation and iteration. I might tinker with an std::stdstream (and a .reserve since we'll know the approximate size) instead of the Arduino String bashing in that loop as those are known to be brutal on heap fragmentation and abuse, but that's mostly me hating Arduino. :-)

Is my value add here to extract just what @netmindz changes from that and produce a pr against 0_15? I'm happy to do that if that's the assignment.

P.S. not to get off into tangents upon tangents, but I just went through this in another code review and now see this new hunk struggling with the same issue. Compilers are starting to get smart/wise/fussy about implicit narrowing and in our embedded parts, that explicit mask with an 0xff to make it a uint8 when it's really a loop index and just needs to be ... something, that's often spelled int_fast8_t. That'll pick the appropriate type whether you're on RISC-V, Xtensa, or x86 that'll hold at least 8 bits, and lets the compiler pick "best".

But really, in most of the places you changed it, Blaz, you can avoid the type completely, probably get better code out of it, and less likely to get into trouble with bounds violations on the edges. For example:

getTotalLength() could just be a call to std::accumulate. Then if you're on RISC-V with Vector or similar hardware, it's easy for the optimizer to see your intent and give you vectorization.

This basic idiom happens a lot in your pending change:

  • for (uint8_t i = 0; i < numBusses; i++) {
  • for (unsigned i = 0; i < numBusses; i++) {

Just let loop type induction work this out. Maybe for (const auto& bus : busses) { if bus.canShow() return false; }

Now you've left the iteration to the optimizer. If busses later becomes, oh, a std::map (I have no idea why in this example, but we all know data types change...) and it's no longer an array, it'll still iterate correctly.

  • uint8_t numPins = NUM_PWM_PINS(_type);
  • for (uint8_t i = 0; i < numPins; i++) {
  • unsigned numPins = NUM_PWM_PINS(_type);
  • for (unsigned i = 0; i < numPins; i++) {

Same idiom: for (const auto& pin : pins) { do the thing with pin. } Is keeping numPins as a separate thing wise? Could pinArray be a vector (or array) and just have size() used on it when needed? Those things always get out of sync over the years somewhere.

I won't say that C-style loops should never be written, but using range-based for ("new" in C++11, but available well before that) is almost always more readable, avoids the kind of issues that you're running into in this very PR. Optimizers don't have to worry about crazy thing like what if some store through a pointer inside the loop modifies the loop counter via a store (!!) which might force it to keep reloading.

I think range-based maps were c++17, but here in embedded-land, most of us have access to really recent toolchains. Range-for is just a better contract between the author and the consumer of the source.

Also, if you put this in your .vimrc, youll see the random whitespaces you've accidentally added:

:highlight ExtraWhitespace ctermbg=red guibg=red :match ExtraWhitespace /\s+$/

I also totally get that you didn't ask for my opinion on any of this and have the right to not care. :-)

Back on topic: do I understand the assignment? Shall I extract the parts of this PR that we discussed and that appeared in the original PR and make a new one based on that?

Thanx!

On Tue, Aug 6, 2024 at 5:09 AM Blaž Kristan @.***> wrote:

@robertlipe https://github.com/robertlipe modified @netmindz https://github.com/netmindz 's files are here https://www.dropbox.com/scl/fo/1ud3z7gvkpgypchspxfpr/ALf3CU2zd66jUtmgHANSOAo?rlkey=lfvwawfyu7qyesxnqix6qqnp9&dl=0 . I've employed what was said above and it seems to work.

— Reply to this email directly, view it on GitHub https://github.com/Aircoookie/WLED/pull/4056#issuecomment-2270903284, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD33GPUI6I7R3QBL7KO3ZQCOFJAVCNFSM6AAAAABK3KHH4WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZQHEYDGMRYGQ . You are receiving this because you were mentioned.Message ID: @.***>

robertlipe avatar Aug 06 '24 11:08 robertlipe