Marlin

Marlin copied to clipboard

Description

Edit changes made when ProUI added to be EXTENSIBLE_UI.

// Extensible UI serial touch screens. (See src/lcd/extui)

ProUI is DWIN / Ender-3V2 screen type, it should not fall in this category.

However... there are too many issues when this was changed. 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

Recent Changes

• added an extra parameter ['K'] to G29 - Bilinear (Marlin\src\gcode\bedlevel\abl\G29.cpp)

if (parser.boolval('K')) bedlevel.extrapolate_unprobed_bed_level();

this allows a user to fill in the missing/unprobed mesh values.

Moved from #26963

• ftdi_eve_extui.cpp
• malyan_extui.cpp

Related Issues

• #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.

I would like to say this is ready for merge. (11/9/24)

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

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

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

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

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.

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

I dont think you understand the code how it is related

That is exactly the problem. I don't understand how it is related.

I am in a position where I could potentially merge your changes, but I am not an expert in ProUI code. I need you to convince me or others in this thread that your changes belong together and are correct, as I don't have the capacity to become an expert for everything in every PR.

All I have to work with is your description, and the code changes. If it isn't clear to me how the two are related, then it's hard for me to want to spend time reviewing the changes.

• ProUI starting up had issues, reverted SETUP_RUN(...) in MarlinCore.cpp
• Move pause function to dwin_popup
• Tweak PID, revert enum tempcontrol_t
• Update other ExtUI onPIDTuning() switch w/ #if ENABLED(...)

These seem like unrelated changes to me, and this isn't even all of them.

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

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

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

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.

@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

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

ExtUI is not just for serial host controllers. It is for any controller that needs to interact with Marlin features and receive event callbacks whenever Marlin does something interesting that should update the display.

I intend to migrate every display that has callbacks sprinkled throughout the Marlin codebase to ExtUI so that there is only one callback at each of those locations instead of one for ProUI, one for ExtUI, another for JyersUI, etc. ExtUI is the correct API to use for this purpose.

If there is a bug with some ProUI behaviors that didn't exist before, I expect it should be very easy to locate and fix those glitches.

If there is a bug with some ProUI behaviors that didn't exist before, I expect it should be very easy to locate and fix those glitches.

yeah I mean it should be, however in my time testing I haven't been able to guess the couple of issues associated with it.

no doubt it would be nice to simplify the code in such call back instances, but in some cases, it looks like some aren't compatible. like for example when you have TERN_(EXTENSIBLE_UI, ExtUI::somecode(const eg)); and a variant for DWIN_LCD_PROUI doesn't exist, there may be unintended consequences.