-
-
Notifications
You must be signed in to change notification settings - Fork 19.1k
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.
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
Misc. ProUI fixes & cleanup #26917
base: bugfix-2.1.x
Are you sure you want to change the base?
Misc. ProUI fixes & cleanup #26917
Conversation
alternative fix, that doesnt convert r,g,b to 32 bit values to store 0-255 values diff --git a/Marlin/src/lcd/e3v2/common/encoder.cpp b/Marlin/src/lcd/e3v2/common/encoder.cpp
index 5825fb0f77..fd3a5f5f48 100644
--- a/Marlin/src/lcd/e3v2/common/encoder.cpp
+++ b/Marlin/src/lcd/e3v2/common/encoder.cpp
@@ -85,7 +85,7 @@ EncoderState encoderReceiveAnalyze() {
next_click_update_ms = millis() + 300;
Encoder_tick();
#if PIN_EXISTS(LCD_LED)
- //LED_Action();
+ LED_Action();
#endif
TERN_(HAS_BACKLIGHT_TIMEOUT, ui.refresh_backlight_timeout());
if (!ui.backlight) {
@@ -169,7 +169,7 @@ 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]; // you cannot store 24bit in 16bit type... so use 32bit
// LED light operation
void LED_Action() {
@@ -210,9 +210,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] = uint32_t(luminance * 10/10) << 8 | uint32_t(luminance * 7/10) << 16 | uint8_t(luminance * 5/10); break;
+ case RGB_SCALE_R10_G7_B4: LED_DataArray[i] = uint32_t(luminance * 10/10) << 8 | uint32_t(luminance * 7/10) << 16 | uint8_t(luminance * 4/10); break;
+ case RGB_SCALE_R10_G8_B7: LED_DataArray[i] = uint32_t(luminance * 10/10) << 8 | uint32_t(luminance * 8/10) << 16 | uint8_t(luminance * 7/10); break;
}
}
LED_WriteData();
@@ -227,13 +227,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;
}
}
diff --git a/Marlin/src/lcd/e3v2/common/encoder.h b/Marlin/src/lcd/e3v2/common/encoder.h
index ce431c9811..9e7b4c7f59 100644
--- a/Marlin/src/lcd/e3v2/common/encoder.h
+++ b/Marlin/src/lcd/e3v2/common/encoder.h
@@ -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();
|
… into bugfix-2.1.x-March2
@ellensp should this be instead? 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] = uint32_t(luminance * 10/10) << 8 | uint32_t(luminance * 7/10) << 16 | uint8_t(luminance * 5/10); break;
- case RGB_SCALE_R10_G7_B4: LED_DataArray[i] = uint32_t(luminance * 10/10) << 8 | uint32_t(luminance * 7/10) << 16 | uint8_t(luminance * 4/10); break;
- case RGB_SCALE_R10_G8_B7: LED_DataArray[i] = uint32_t(luminance * 10/10) << 8 | uint32_t(luminance * 8/10) << 16 | uint8_t(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; |
a87f4fb
to
c273079
Compare
… into bugfix-2.1.x-March2
@thinkyhead hey I was able to fix the issue when starting ProUI with ExtUI by "reverting" the important part I wanted to mention is looking back through where "Probing Point" shows up, I find in feature/bedleve/ubl/ubl_G29.cpp:
this reminded me of another similar error/bug I discovered before, showing random stuff in the display. and it isn't just showing this one line "Probing Point...", but I think every single line having anyway, even though there is no "real" error or issue when everything is working and compiled correctly, I wanted to point this out as a possible "real" unseen issue in the background. another issue which I seem not being able to fix at the moment is selecting a file to print. it crashes and reboots immediately selecting a .gcode file. edit: could this be the reason for the issue? I see some use ui.status_printf(0,
F(S_FMT GANG_N_1(NUM_AXES, " %c") " %c"),
GET_TEXT(MSG_LCD_ENDSTOPS),
NUM_AXIS_LIST_(chrX, chrY, chrZ, chrI, chrJ, chrK, chrU, chrV, chrW) chrP
) so for context, I noticed this message popup randomly in a previous bug issue I posted. another thing is what is the purpose for |
@classicrocker883 do you think you could re-post these as individual changes that have the narrowest scope possible? Ideally there isn't a lot of I know you might not have seen that request on other commits, but I'm not familiar with this code, so the narrower the scope of each PR, the easier it is for you to convince that your changes in each PR are correct. Otherwise it only takes one line I'm not sure about to block the whole thing. Also descriptions like "Change some things" and "tweaks" don't help me understand why a change is being made. Maybe these are just readability changes, that's ok, but preferably those would be separated from behavior changes. |
well the reason for some descriptions being vague is easier for the code to be looked at than explained. I can try being more descriptive for "changes" and "tweaks" which require explanation, otherwise they are irrelative to the PR as a whole so they are better seen instead of heard - if you understand that. |
OK, let me try to phrase this in a different way. Your description mentions reverting one change in one file. Your PR is touching 19 files, and I don't know why. I'm not going to try to reverse-engineer the meaning behind all these changes you have coupled together, so I am going to close this PR. Please feel free to re-post your improvements as individual changes which belong together and can be accurately described. |
I literally am changing it right now why did you close this |
you know you didnt have to close this. I am editing it now. wtf? |
Your reply to my request appeared to be a refusal to change anything beyond the title. I'll re-open it if you are adjusting it. |
Most likely this needs to become multiple PRs though. If you are just adjusting descriptions it is still likely coupling unrelated changes together. I would recommend one small targeted PR that fixes the regression you mentioned in SETUP_RUN, then follow-up PRs changing other things. If any of it is just pure refactoring to improve readability, that would ideally be a change by itself without functional changes. |
I dont think you understand the code how it is related |
they are all related to this previous PR that was merged. so instead of splitting each change, its better to keep them all in one because they are all equally related to the ProUI changes |
It's fine to have 5 follow-up PRs. What's important is that each PR is easy to understand and justify on its own. It also has the advantage that if one is rejected the other four can still go in. I know that doesn't match your previous PR experience, but that is the best way to get PRs in quickly without controversy, rather than drawing them out for months. |
PR #26563 added various ExtUI calls to Marlin/src/lcd/e3v2/proui/dwin.cpp, but this file does not have #include "../../extui/ui_api.h" resulting in Many errors
My test Configs that trigger this error Issue is #define FILAMENT_RUNOUT_SENSOR, if you dare to disable that you get errors With this enabled #include "../../../feature/runout.h" includes #include "../lcd/extui/ui_api.h" Added a PR to fix this issue |
I had originally thought about including that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any major issues here, just a few questions for further explanation or to consider some possible side-effects.
@classicrocker883 along with answering the questions in the above review, please do the following:
|
… into bugfix-2.1.x-March2
@thinkyhead since you came up with moving ProUI to be with ExtUI, there is now the issue with printing. Selecting a .gcode file results in freeze and restart, which wasn't an issue before the move. So, this could take a review through. I point out since the initial ProUI => ExtUI commit there is an issue starting up (addressed and fixed with this PR), but now we have to find why its not letting us select a file to print. |
Description
this PR should fix issues with recent ExtUI for ProUI Follow up to "🚸 Update ProUI Plot graph (#26539)" #26563
SETUP_RUN(...)
in MarlinCore.cpp for fixdwinPopupContinue
to template likedwinPopupConfirm
tempcontrol_t
onPIDTuning()
switch w/#if ENABLED(...)
GET_TEXT_F
+MSG_
instead of literal strings in dwin.cppANY(MESH_BED_LEVELING, HAS_BED_PROBE)
toANY(BABYSTEPPING, PROBE_SELECTED)
PROBE_SELECTED
because:PROBE_OFFSET_ZMIN/MAX
gets undefined and is used when changing Z offset even without probe.HAS_ZOFFSET_ITEM
Related Issues