Update encoder.cpp LED #26946
Update encoder.cpp LED #26946
Conversation
| uint16_t LED_DataArray[LED_NUM]; | ||
| uint32_t LED_DataArray[LED_NUM]; |
There was a problem hiding this comment.
Did this code work at all before, or were you prevented from even trying it due to the errors?
I am wondering what the old behavior was here, with a 16-bit integer trying to hold a 24-bit value?
There was a problem hiding this comment.
ill refer you to the previous PR comment as this was proposed by @ellensp
#26917 (comment)
| //LED_Action(); | ||
| LED_Action(); |
There was a problem hiding this comment.
Have you tested this to confirm it behaves as expected? If yes, what does it actually do?
There was a problem hiding this comment.
unable to actually test
| //LED_GraduallyControl(RGB_SCALE_COOL_WHITE, 0x0F, 1000); | ||
| delay(30); | ||
| LED_Control(RGB_SCALE_WARM_WHITE, 0x00); | ||
| //LED_GraduallyControl(RGB_SCALE_COOL_WHITE, 0x00, 1000); |
There was a problem hiding this comment.
I think there is a reason LED_GraduallyControl wasn't used anywhere. It has the potential to loop for a very long time, which probably is not desired.
Even if it is ok, the 1000 argument you have is going to make it wait a full second between each adjustment.
Have you tried to actually enable and use this? Reading the code, I wonder if the more appropriate action is to just delete the LED_GraduallyControl function completely.
There was a problem hiding this comment.
i think graduallycontrol is basically just that, to loop through colors or brightness. the function exists and isn't used. so maybe the original code when first place can say something about it. otherwise it is what it is.
|
Closing this, on the basis that contains untested changes, and there has been no attempt to resolve the questions I've made on the PR. If there is a warning to be fixed that can be done on its own, but I don't think we should be making changes to encourage the use of untested functions, which per my earlier comments I suspect were unused for very good reasons. |
Description
this PR should fix #26552
it appears the LED component for encoder in DWIN UI's does not appear used, like
LED_GraduallyControl()so this should help a user with using the different ways an LED can be controlled.
update with @ellensp comment
Requirements
Benefits
Configurations
Related Issues