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

Update encoder.cpp LED #26946

Closed
wants to merge 1 commit into from
Closed

Update encoder.cpp LED #26946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions Marlin/src/lcd/e3v2/common/encoder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ EncoderState encoderReceiveAnalyze() {
next_click_update_ms = millis() + 300;
Encoder_tick();
#if PIN_EXISTS(LCD_LED)
//LED_Action();
LED_Action();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested this to confirm it behaves as expected? If yes, what does it actually do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unable to actually test

#endif
TERN_(HAS_BACKLIGHT_TIMEOUT, ui.refresh_backlight_timeout());
if (!ui.backlight) {
Expand Down Expand Up @@ -169,13 +169,15 @@ EncoderState encoderReceiveAnalyze() {
#if PIN_EXISTS(LCD_LED)

// Take the low 24 valid bits 24Bit: G7 G6 G5 G4 G3 G2 G1 G0 R7 R6 R5 R4 R3 R2 R1 R0 B7 B6 B5 B4 B3 B2 B1 B0
uint16_t LED_DataArray[LED_NUM];
uint32_t LED_DataArray[LED_NUM];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@classicrocker883 classicrocker883 Apr 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ill refer you to the previous PR comment as this was proposed by @ellensp
#26917 (comment)


// LED light operation
void LED_Action() {
LED_Control(RGB_SCALE_WARM_WHITE, 0x0F);
//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);
Comment on lines +177 to +180
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// LED initialization
Expand Down Expand Up @@ -210,9 +212,9 @@ EncoderState encoderReceiveAnalyze() {
for (uint8_t i = 0; i < LED_NUM; i++) {
LED_DataArray[i] = 0;
switch (RGB_Scale) {
case RGB_SCALE_R10_G7_B5: LED_DataArray[i] = (luminance * 10/10) << 8 | (luminance * 7/10) << 16 | luminance * 5/10; break;
case RGB_SCALE_R10_G7_B4: LED_DataArray[i] = (luminance * 10/10) << 8 | (luminance * 7/10) << 16 | luminance * 4/10; break;
case RGB_SCALE_R10_G8_B7: LED_DataArray[i] = (luminance * 10/10) << 8 | (luminance * 8/10) << 16 | luminance * 7/10; break;
case RGB_SCALE_R10_G7_B5: LED_DataArray[i] = uint8_t(luminance * 10/10) << 8 | uint8_t(luminance * 7/10) << 16 | uint8_t(luminance * 5/10); break;
case RGB_SCALE_R10_G7_B4: LED_DataArray[i] = uint8_t(luminance * 10/10) << 8 | uint8_t(luminance * 7/10) << 16 | uint8_t(luminance * 4/10); break;
case RGB_SCALE_R10_G8_B7: LED_DataArray[i] = uint8_t(luminance * 10/10) << 8 | uint8_t(luminance * 8/10) << 16 | uint8_t(luminance * 7/10); break;
}
}
LED_WriteData();
Expand All @@ -227,13 +229,13 @@ EncoderState encoderReceiveAnalyze() {
for (uint8_t i = 0; i < LED_NUM; i++) {
switch (RGB_Scale) {
case RGB_SCALE_R10_G7_B5:
led_data[i] = { luminance * 7/10, luminance * 10/10, luminance * 5/10 };
led_data[i] = { uint8_t(luminance * 7/10), uint8_t(luminance * 10/10), uint8_t(luminance * 5/10) };
break;
case RGB_SCALE_R10_G7_B4:
led_data[i] = { luminance * 7/10, luminance * 10/10, luminance * 4/10 };
led_data[i] = { uint8_t(luminance * 7/10), uint8_t(luminance * 10/10), uint8_t(luminance * 4/10) };
break;
case RGB_SCALE_R10_G8_B7:
led_data[i] = { luminance * 8/10, luminance * 10/10, luminance * 7/10 };
led_data[i] = { uint8_t(luminance * 8/10), uint8_t(luminance * 10/10), uint8_t(luminance * 7/10) };
break;
}
}
Expand Down
2 changes: 0 additions & 2 deletions Marlin/src/lcd/e3v2/common/encoder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,6 @@ inline bool applyEncoder(const EncoderState &encoder_diffState, T &valref) {
#define RGB_SCALE_WARM_WHITE RGB_SCALE_R10_G7_B4
#define RGB_SCALE_COOL_WHITE RGB_SCALE_R10_G8_B7

extern unsigned int LED_DataArray[LED_NUM];

// LED light operation
void LED_Action();

Expand Down
Loading