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.

Sign up for GitHub

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

Jump to bottom

Fetch supported LED types from Bus #4056

Closed
wants to merge 13 commits into from
Closed

Fetch supported LED types from Bus #4056

wants to merge 13 commits into from

Conversation

netmindz
Copy link
Collaborator

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 - #3777

@netmindz netmindz mentioned this pull request Jul 14, 2024
Open
@netmindz netmindz changed the title Fetch supported LED types form Bus Fetch supported LED types from Bus Jul 14, 2024
@blazoncek
Copy link
Collaborator

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

@netmindz
Copy link
Collaborator Author

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

@blazoncek
Copy link
Collaborator

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.

@netmindz
Copy link
Collaborator Author

Change made to the strings @blazoncek

@netmindz netmindz mentioned this pull request Jul 15, 2024
Draft
@netmindz
Copy link
Collaborator Author

netmindz commented Jul 16, 2024 •
edited
Loading

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

oappend() buffer overflow. Cannot append 1127 bytes

@netmindz
Copy link
Collaborator Author

Fixed

@blazoncek
Copy link
Collaborator

@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
Copy link
Collaborator

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

@robertlipe
Copy link

robertlipe commented Jul 17, 2024 via email

@netmindz
Copy link
Collaborator Author

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

@netmindz
Copy link
Collaborator Author

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

@blazoncek
Copy link
Collaborator

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:

@blazoncek
Copy link
Collaborator

@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
Copy link
Collaborator

blazoncek commented Jul 18, 2024 •
edited
Loading

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).

@netmindz
Copy link
Collaborator Author

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

@blazoncek
Copy link
Collaborator

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

@blazoncek
Copy link
Collaborator

which Bus classes you include in the build

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

@netmindz
Copy link
Collaborator Author

netmindz commented Aug 5, 2024

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
Copy link
Collaborator Author

netmindz commented Aug 5, 2024

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?

@robertlipe
Copy link

robertlipe commented Aug 5, 2024 via email

@robertlipe
Copy link

robertlipe commented Aug 5, 2024 via email

@blazoncek
Copy link
Collaborator

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

@robertlipe
Copy link

robertlipe commented Aug 6, 2024 via email

@blazoncek
Copy link
Collaborator

@robertlipe you will need to start using simpler English. I have hard time following. 😄
I do not know what was @netmindz intent but overall this PR is one step towards merging HUB75 support which has "unconventional" settings (for WLED). The code I modified still serves the purpose but simplifies things a bit.

A lot of WLED code is now 2 or 3 years old and would benefit greatly if it could be modernised. But that is a huge task.
If we keep it to bus manager then I would start by changing busses into vector (as it doesn't change a lot after it is set up and we can save a few bytes by releasing unused array elements). Still, busses is just an array of pointers, 20 on ESP32 and much less on others so perhaps not worth investing time in it except if that would make code smaller.

And BTW I do appreciate any and every feedback. Thank you.

@netmindz netmindz mentioned this pull request Aug 8, 2024
Closed
@netmindz
Copy link
Collaborator Author

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

These changes fail to delegate the list of types to the specific bus classes so fails the requirement of "to be fetched from the various Bus classes" as stated in the description

netmindz and others added 3 commits August 20, 2024 22:11
@netmindz
table-style definition of LEDType
c452bd5
@willmmiles
Use list initialization for getLEDTypes
566c9f0 
In modern C++, we can load a vector with brace initialized list of
values.  Use this to speed up the list assembly.

Saves about 1kb of code space.
@willmmiles
Tighten LED types JSON assembly
16716d4 
We don't really need to bother building the whole list, just the string.

Saves about 300 bytes of code.
@willmmiles
Copy link
Collaborator

willmmiles commented Aug 21, 2024 •
edited
Loading

I put some suggestions in https://github.com/willmmiles/WLED/tree/pr/netmindz/4056-1 .

  • willmmiles@566c9f0 -- We can list-initialize the std::vectors<> , instead of individually pushing each element. It's a bit faster at runtime as std::vector can allocate exactly the right amount of memory in one go, and saves a bunch of code space.
  • willmmiles@16716d4 -- A tweak to the JSON formatting code to save some temporary copies

I agree with @robertlipe that using std::vector feels a bit dirty as the contents for each bus classification are static and known at compile time, so we don't really need to copy them in to heap memory every time. Unfortunately, as far as I'm aware, there's no good stdlib solution that doesn't require the API to expose the list length as part of each return type until C++20 when we get std::span, essentially a pointer+length pair with the C++ stdlib container API. The best I think we could do there with the current build tools is something like:

std::pair<const LEDType*, size_t> BusXXX::getLEDTypes() {
    const static LEDType[] myTypes = {...};
    return {myTypes, std::extent<decltype(myTypes)>::value};
}

Less C++-flavored, but faster and probably smaller still.

@blazoncek
Copy link
Collaborator

The solution I proposed avoids memory manipulations and its only drawback is the removal of fetching LED types from different buses (no abstraction).
I know I am not a very good (C++) reference but BusManager is the one outside world communicates with and is the one to handle all LED types so IMO it is still the place to provide the functionality for getLEDTypes().

I hope I understand @netmindz correctly that he wants to include additional information like required pins, etc. into LEDtype array to provide dynamic settings UI in next iteration of this PR but I still do not see that as an obstacle as all that is a compile time operation and LEDType array can be constructed in the same way with enhanced content within BusManager.
To put it the other way, I see no added value to have each BusXXX provide getLEDTypes(). And having different return type for Bus::getLEDTypes() and BusManager::getLEDTypes() is confusing. But that is just my opinion.

@netmindz
Copy link
Collaborator Author

I put some suggestions in https://github.com/willmmiles/WLED/tree/pr/netmindz/4056-1 .

  • willmmiles@566c9f0 -- We can list-initialize the std::vectors<> , instead of individually pushing each element. It's a bit faster at runtime as std::vector can allocate exactly the right amount of memory in one go, and saves a bunch of code space.
  • willmmiles@16716d4 -- A tweak to the JSON formatting code to save some temporary copies

Thanks for these, I have pulled these changes into my branch

I agree with @robertlipe that using std::vector feels a bit dirty as the contents for each bus classification are static and known at compile time, so we don't really need to copy them in to heap memory every time. Unfortunately, as far as I'm aware, there's no good stdlib solution that doesn't require the API to expose the list length as part of each return type until C++20 when we get std::span, essentially a pointer+length pair with the C++ stdlib container API. The best I think we could do there with the current build tools is something like:

std::pair<const LEDType*, size_t> BusXXX::getLEDTypes() {
    const static LEDType[] myTypes = {...};
    return {myTypes, std::extent<decltype(myTypes)>::value};
}

Less C++-flavored, but faster and probably smaller still.

Regarding this, I really think we are making way too heavy weather of this. If this were used in normal operations then I could understand the need to optimise within an inch of its life, but for something that is only used to fetch the info needed to render the LED preferences page, which is used for setup and then rarely touched again .....

Surely for the nice new LEDTypesToJson method that relies on having a common parameter type - which surely an array with size would not be?

The alternative would to just change each bus class to return JSON as a string, then the bus manager just wraps them into a single JSON response

@netmindz
Copy link
Collaborator Author

The solution I proposed avoids memory manipulations and its only drawback is the removal of fetching LED types from different buses (no abstraction). I know I am not a very good (C++) reference but BusManager is the one outside world communicates with and is the one to handle all LED types so IMO it is still the place to provide the functionality for getLEDTypes().

That lack of abstraction will prevent what I want to do with later PRs, but yes it should be BusManager that returns the data, not direct calls from xml.cpp to specific drivers

I hope I understand @netmindz correctly that he wants to include additional information like required pins, etc. into LEDtype array to provide dynamic settings UI in next iteration of this PR but I still do not see that as an obstacle as all that is a compile time operation and LEDType array can be constructed in the same way with enhanced content within BusManager.

Due to the fixed size issue of an array, this causes problems, as mentioned in my comment regarding LEDTypesToJson

To put it the other way, I see no added value to have each BusXXX provide getLEDTypes(). And having different return type for Bus::getLEDTypes() and BusManager::getLEDTypes() is confusing. But that is just my opinion.

We could change the return type to be a vector for both, but then we need to put the serialization to json elsewhere, so that's why I was trying to keep xml.cpp neat and without memory-hungry json documents where not required.
Alternatively, change both to string, but then we lose the option to do anything like sorting by name on the combined list before retuning. This could be done in the UI code however if required

With regards to later changes, I still need to work out the details of how to give the BusXXX the most flexibility of what options they return, without making the UI code overly complex and also how to manage any migration from the current really hacky use/abuse of the pins array into something a little better.

@robertlipe
Copy link

robertlipe commented Aug 21, 2024 via email

@robertlipe
Copy link

robertlipe commented Aug 21, 2024 via email

@blazoncek
Copy link
Collaborator

I've made simpler variant of the same here.
Does not use abstraction and has (I hope) minimal footprint.

@blazoncek
Copy link
Collaborator

@netmindz if you are willing to undo changes in xml.cpp and led_settings.html and change destination branch to bus-config I am willing to merge the latest incarnation of this PR.

@blazoncek blazoncek mentioned this pull request Sep 5, 2024
Merged
@netmindz
Copy link
Collaborator Author

netmindz commented Sep 7, 2024

Hi @blazoncek , I'll try and find some time over the next few days to have a look at the two branches

@blazoncek
Copy link
Collaborator

I've implemented all of the discussions from above while keeping your (extensible) approach.

@netmindz netmindz changed the base branch from 0_15 to bus-config September 7, 2024 18:08
@netmindz netmindz changed the base branch from bus-config to 0_15 September 7, 2024 18:09
@netmindz netmindz changed the base branch from 0_15 to bus-config September 7, 2024 18:17
@netmindz
Copy link
Collaborator Author

netmindz commented Sep 7, 2024

Shall I close this one then as it looks like you already have add the changes you want to keep into your own branch? @blazoncek

@netmindz netmindz closed this Sep 7, 2024
@blazoncek
Copy link
Collaborator

as it looks like you already have add the changes you want to keep into your own branch

Yes I did, thought of it and figured out it would be much more hassle otherwise. I hope you are ok with the credit in the comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertlipe robertlipe robertlipe left review comments

@blazoncek blazoncek blazoncek left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@netmindz @blazoncek @robertlipe @willmmiles