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.

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

Open
wants to merge 27 commits into
base: bugfix-2.1.x
Choose a base branch
from

Conversation

classicrocker883
Copy link
Contributor

@classicrocker883 classicrocker883 commented Mar 28, 2024

Description

this PR should fix issues with recent ExtUI for ProUI Follow up to "🚸 Update ProUI Plot graph (#26539)" #26563

  • ProUI starting up has issues, reverted SETUP_RUN(...) in MarlinCore.cpp for fix
  • Move pause function to dwin_popup
  • Change dwinPopupContinue to template like dwinPopupConfirm
  • Tweak PID, revert enum tempcontrol_t
  • Update other ExtUI onPIDTuning() switch w/ #if ENABLED(...)
  • Update / rearrange language_en.h
    • Change to GET_TEXT_F + MSG_ instead of literal strings in dwin.cpp
    • Remove redundants
  • In Conditionals_post.h under Bed Probe dependencies, ANY(MESH_BED_LEVELING, HAS_BED_PROBE) to ANY(BABYSTEPPING, PROBE_SELECTED)
    • This uses PROBE_SELECTED because:
       #if ANY(HAS_BED_PROBE, PROBE_MANUALLY, MESH_BED_LEVELING)
        #define PROBE_SELECTED 1
    • This change is needed when using ProUI without any leveling, because PROBE_OFFSET_ZMIN/MAX gets undefined and is used when changing Z offset even without probe.
    • I suppose another workaround could incorporate using HAS_ZOFFSET_ITEM

Related Issues

  • Follow up to "🚸 Update ProUI Plot graph (#26539)" #26563
  • Issues arose as a result of swapping ProUI to ExtUI namespace:
    1. Since then, LCD crashes and restarts when selecting .gcode file to Print.
    2. Not sure the cause, this PR does fix the main issue when starting (as stated above), but selecting file to print most definitely is related to the original commit. unfortunately it has many changes, and it seems to be correct. Aside from that issue it works fine.

@ellensp
Copy link
Contributor

ellensp commented Mar 28, 2024

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();
 

@classicrocker883
Copy link
Contributor Author

@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;

@classicrocker883 classicrocker883 changed the title Update encoder.cpp LED, remove whitespace Follow up to #26563 - ExtUI tweaks, update encoder.cpp LED, remove whitespace Apr 5, 2024
@classicrocker883
Copy link
Contributor Author

classicrocker883 commented Apr 5, 2024

@thinkyhead hey I was able to fix the issue when starting ProUI with ExtUI by "reverting" SETUP_RUN() in MarlinCore.cpp, but there is something peculiar to note:
when testing and first booting ProUI (before fix), the LCD status would be all jumbled, but what is odd is the status text would give a strange message, like "Probing Point ...#/#". but more like showing several messages at once, then the display would sort of reset, and kind of be usable but half the icons/font not show.

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: ui.status_printf(0, F(S_FMT " %i/%i"), ..., but when hovering over this function the little window popups up and shows:

static <error-type> MarlinUI::status_printf<<error-type>, grid_count_t, <error-type>>(<error-type>, <error-type>, <error-type>)

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 status_printf() like "Tilting Point..." because everything shows up almost all at once so it's hard to tell.

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 GET_TEXT while the majority use GET_TEXT_F, does that make a difference in this instance? because in a similar bug that I mentioned, it uses the same status_printf() and GET_TEXT (without _F):
in module/endstops.cpp

      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
typedef bits_t(TEMPCONTROL_COUNT) tempcontrol_t; in "enum TempControl"? surely the enum doesn't need to increase bit size when there is more to count, right?

@sjasonsmith
Copy link
Contributor

@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 and in the description.

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.

@classicrocker883
Copy link
Contributor Author

@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 and in the description.

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.

@classicrocker883 classicrocker883 changed the title Follow up to #26563 - ExtUI tweaks, update encoder.cpp LED, remove whitespace Follow up to #26563 - ExtUI tweaks Apr 8, 2024
@sjasonsmith
Copy link
Contributor

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.

@classicrocker883
Copy link
Contributor Author

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

@classicrocker883
Copy link
Contributor Author

you know you didnt have to close this. I am editing it now. wtf?

@sjasonsmith
Copy link
Contributor

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.

@sjasonsmith sjasonsmith reopened this Apr 8, 2024
@sjasonsmith
Copy link
Contributor

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.

@classicrocker883
Copy link
Contributor Author

I would recommend one small targeted PR that fixes the regression you mentioned in SETUP_RUN, then follow-up PRs changing other things.

I dont think you understand the code how it is related

@classicrocker883
Copy link
Contributor Author

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

@sjasonsmith
Copy link
Contributor

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.

@ellensp
Copy link
Contributor

ellensp commented Apr 15, 2024

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

Compiling .pio/build/STM32F103RE_creality/src/src/lcd/extui/ui_api.cpp.o
Marlin/src/lcd/e3v2/proui/dwin.cpp: In function 'void hmiSDCardUpdate()':
Marlin/src/lcd/e3v2/proui/dwin.cpp:1045:46: error: 'ExtUI' has not been declared
 1045 |     if (!DWIN_lcd_sd_status && sdPrinting()) ExtUI::stopPrint();  // Media removed while printing
      |                                              ^~~~~
Marlin/src/lcd/e3v2/proui/dwin.cpp: In function 'void onClickPauseOrStop()':
Marlin/src/lcd/e3v2/proui/dwin.cpp:1169:55: error: 'ExtUI' has not been declared
 1169 |     case PRINT_PAUSE_RESUME: if (hmiFlag.select_flag) ExtUI::pausePrint(); break; // Confirm pause
      |                                                       ^~~~~
Marlin/src/lcd/e3v2/proui/dwin.cpp:1170:47: error: 'ExtUI' has not been declared
 1170 |     case PRINT_STOP: if (hmiFlag.select_flag) ExtUI::stopPrint(); break; // Stop confirmed then abort print
      |                                               ^~~~~
Marlin/src/lcd/e3v2/proui/dwin.cpp: In function 'void hmiPrinting()':
Marlin/src/lcd/e3v2/proui/dwin.cpp:1204:11: error: 'ExtUI' has not been declared
 1204 |           ExtUI::resumePrint();
      |           ^~~~~
Marlin/src/lcd/e3v2/proui/dwin.cpp: At global scope:
Marlin/src/lcd/e3v2/proui/dwin.cpp:1854:15: error: 'ExtUI' has not been declared
 1854 | static_assert(ExtUI::eeprom_data_size >= sizeof(hmi_data_t), "Insufficient space in EEPROM for UI parameters");
      |               ^~~~~
*

My test Configs that trigger this error
Configuration.zip

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"
Without it extui/ui_api.h is not loaded

Added a PR to fix this issue

@classicrocker883
Copy link
Contributor Author

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

Compiling .pio/build/STM32F103RE_creality/src/src/lcd/extui/ui_api.cpp.o
Marlin/src/lcd/e3v2/proui/dwin.cpp: In function 'void hmiSDCardUpdate()':
Marlin/src/lcd/e3v2/proui/dwin.cpp:1045:46: error: 'ExtUI' has not been declared
 1045 |     if (!DWIN_lcd_sd_status && sdPrinting()) ExtUI::stopPrint();  // Media removed while printing
      |                                              ^~~~~
Marlin/src/lcd/e3v2/proui/dwin.cpp: In function 'void onClickPauseOrStop()':
Marlin/src/lcd/e3v2/proui/dwin.cpp:1169:55: error: 'ExtUI' has not been declared
 1169 |     case PRINT_PAUSE_RESUME: if (hmiFlag.select_flag) ExtUI::pausePrint(); break; // Confirm pause
      |                                                       ^~~~~
Marlin/src/lcd/e3v2/proui/dwin.cpp:1170:47: error: 'ExtUI' has not been declared
 1170 |     case PRINT_STOP: if (hmiFlag.select_flag) ExtUI::stopPrint(); break; // Stop confirmed then abort print
      |                                               ^~~~~
Marlin/src/lcd/e3v2/proui/dwin.cpp: In function 'void hmiPrinting()':
Marlin/src/lcd/e3v2/proui/dwin.cpp:1204:11: error: 'ExtUI' has not been declared
 1204 |           ExtUI::resumePrint();
      |           ^~~~~
Marlin/src/lcd/e3v2/proui/dwin.cpp: At global scope:
Marlin/src/lcd/e3v2/proui/dwin.cpp:1854:15: error: 'ExtUI' has not been declared
 1854 | static_assert(ExtUI::eeprom_data_size >= sizeof(hmi_data_t), "Insufficient space in EEPROM for UI parameters");
      |               ^~~~~
*

My test Configs that trigger this error Configuration.zip

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" Without it extui/ui_api.h is not loaded

Added a PR to fix this issue

I had originally thought about including that #include because in my tests for some reason this error would come and go and it confused me why sometimes it compiled fine, even in the github Actions test was ok.

Copy link
Contributor

@sjasonsmith sjasonsmith left a 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.

Marlin/src/lcd/e3v2/proui/dwin.cpp Show resolved Hide resolved
Marlin/src/lcd/e3v2/proui/proui_extui.cpp Outdated Show resolved Hide resolved
Marlin/src/lcd/extui/ui_api.h Outdated Show resolved Hide resolved
@sjasonsmith
Copy link
Contributor

@classicrocker883 along with answering the questions in the above review, please do the following:

  1. Update the title to something more descriptive then "ExtUI Tweaks"

  2. For the items listed in the description, please add the following:

    • Why you changed them
    • What you did
  3. Add a description of your testing to the description

@classicrocker883 classicrocker883 changed the title Follow up to #26563 - ExtUI tweaks Follow up to #26563 - ExtUI tweaks to make it work better May 11, 2024
@classicrocker883 classicrocker883 changed the title Follow up to #26563 - ExtUI tweaks to make it work better Follow up to #26563 - ExtUI tweaks to ProUI - make it work better May 11, 2024
@classicrocker883
Copy link
Contributor Author

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

@thisiskeithb thisiskeithb linked an issue Jun 2, 2024 that may be closed by this pull request
1 task
@thisiskeithb thisiskeithb changed the title Follow up to #26563 - ExtUI tweaks to ProUI - make it work better Misc. ProUI fixes & cleanup Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] DWIN_LCD_PROUI won't draw screen
6 participants