brightness/power limit #352
Conversation
|
Thanks, I'll take a look when I have some time (hopefully next week). (I didn't forget it, it's just more time needed to review) |
There was a problem hiding this comment.
Looks mostly solid, thanks.
I would add a question mark with explanation for the brightness cap to explain how it's different from the brightness multiplier and what's the use-case (most people will want to leave it at max, while adjusting overall brightness should be more common), WDYT?
| } | ||
| namespace AlienFx | ||
| { | ||
| static const QString NumberOfLeds = "AlienFx/NumberOfLeds"; | ||
| static const QString LedMilliAmps = "AlienFx/LedMilliAmps"; | ||
| static const QString PowerSupplyAmps = "AlienFx/PowerSupplyAmps"; |
There was a problem hiding this comment.
AlienFX has no power supply AFAIK, it's part of the PC. But special-casing it is probably not worth the effort.
There was a problem hiding this comment.
Good point (unless repurposed!). I'll have to take that into consideration while looking into devices (see below)
| } | ||
| namespace Lightpack | ||
| { | ||
| static const QString NumberOfLeds = "Lightpack/NumberOfLeds"; | ||
| static const QString LedMilliAmps = "Lightpack/LedMilliAmps"; | ||
| static const QString PowerSupplyAmps = "Lightpack/PowerSupplyAmps"; |
There was a problem hiding this comment.
I'm wondering whether this needs to be stored separately per device. Users normally have only one device, and if the defaults are identical across devices, what's the value of having separate keys for this?
There was a problem hiding this comment.
Well, it's consistent with NumberOfLeds which has the same "issue". But I was thinking about looking into a way to have multiple devices, which would also mean multiple devices of the same type, and that'll probably make current device settings storage inadequate anyway, so I'll leave that as is for now.
There was a problem hiding this comment.
Well, NumberOfLeds has different defaults and limits across devices, but yeah.
| else if (field("isVirtual").toBool()) | ||
| device = SupportedDevices::DeviceTypeVirtual; | ||
| else if (field("isAlienFx").toBool()) | ||
| device = SupportedDevices::DeviceTypeAlienFx; |
There was a problem hiding this comment.
Consider putting this block in a helper function (for readability and to avoid going out of sync)
There was a problem hiding this comment.
TBH I don't like the current way of handling device types and grabbers. I'm sure it was fine at the start, but as both lists grow you have to duplicate tons of stuff all over the place and have this kind of redundancies.
Since the addition of UDP stuff I wanted to take a look at that, probably before multidevice thing. So I'll leave that for a future PR.
If you already have some ideas on both subjects I'm all ears.
There was a problem hiding this comment.
Some issues can be solved through the inheritance (as already is the case) but especially at the intersection with the UI some manual switching/selecting will be necessary I think
|
I was hoping the name would be enough to get the basic purpose, and people who are savvy enough to worry about power and heat would probably naturally look for a setting like that (like the person asking for the feature), but eh, it's simple enough to add, sure :) |
This is for #342, which reminded me of a similar thing implemented on WLED, both are good ideas, so I added them:
brightness cap (in device tab): to limit brightest LEDs as per request
power supply limit (in device wizard): inspired by WLED, the idea is to put current requirements per LED and whatever current the power supply can provide and if the estimated current requirement for a given frame goes over the limit readjust the colors to fit within the limit
The caveat is that IRL it's not linear and depends on different things, but it's a start.