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

[BUG] MINIPANEL wrong contrast defines leading to blank screen #14174

Closed
Str8t opened this issue May 29, 2019 · 13 comments
Closed

[BUG] MINIPANEL wrong contrast defines leading to blank screen #14174

Str8t opened this issue May 29, 2019 · 13 comments

Comments

@Str8t
Copy link

Str8t commented May 29, 2019

Description

After starting to use the MINIPANEL of the Creality Ender-2 on the SKR V1.3 I ran into several problems.

One being, that the display turned on (some pixels burning, like an the original Melzi board) - but then the screen turned blank.
There are several issues which sound related (like #13732) but didn´t have any real solutions.

After analyzing the actual SPI trace I found out after the initialization sequence was done, the contrast is set two times again.
The first time it is set to zero, so probably a reset. But the second time it is set to the very low value of 4, which explains why the display is blank.

The DEFAULT_LCD_CONTRAST value of the MINIPANEL is 17, if not defined otherwise. The LCD_CONTRAST_MIN is 0 and the LCD_CONTRAST_MAX is 63.
This is conform to the possible value range in the UC1701 datasheet.

But the problem is, the u8g lib right shifts the contrast value two times when sending out an U8G_DEV_MSG_CONTRAST message. That explains why I could see a value of 4 on the SPI trace (17 >> 2).

After setting the LCD_CONTRAST_MAX to 255 and the DEFAULT_LCD_CONTRAST to 156 (value of 0x27 used for the contrast in the initialization sequence, left shifted two times) the display works as expected.

For fun I also had a look at the SPI trace of the Melzi board. There were no re-init commands of the contrast sent. Turns out that on Marlin 1.1.x the MINIPANEL didn´t have the define HAS_LCD_CONTRAST, so the contrast was not set at all (except on init).

I will try to create a pull request in the next few days.

Steps to Reproduce

  1. Use MINIPANEL on Marlin 2.0
  2. Turn on the device

Expected behavior:
Screen should show the main menu after startup

Actual behavior:
Screen turned blank.

@dlitz
Copy link
Contributor

dlitz commented Jun 20, 2019

I'm also seeing this on the Creality CR-20 (not supported by Marlin yet, but I've got some local patches based on Creality's GPL code dump; it's similar to a RAMPS board, but with different pinouts).

Running bugfix-2.0.x on my CR20, setting "M250 C63" makes the screen very slightly visible if I look really hard. Under 1.1.x, the M250 command isn't even recognized. It seems that HAS_LCD_CONTRAST evaluated false for MINIPANEL under Marlin 1.1.x, but it's been enabled under 2.0.x

If I modify the 1.1.x code so that HAS_LCD_CONTRAST is forced true, the screen seems to start out fine, and M250 reports that the contrast is 32. But if I then issue M250 C32, which should produce no change, I get the same blank-screen effect as reported in this bug.

So, it seems like setting LCD contrast on MINIPANEL has never worked properly, but Marlin wasn't even trying to set it under 1.1.x.

Right now, the code in Conditionals_LCD.h looks pretty bogus. That #ifndef DEFAULT_LCD_CONTRAST block will never get executed:

#define HAS_LCD_CONTRAST (HAS_GRAPHICAL_LCD && defined(DEFAULT_LCD_CONTRAST))
#if HAS_LCD_CONTRAST
  #ifndef LCD_CONTRAST_MIN
    #define LCD_CONTRAST_MIN 0
  #endif
  #ifndef LCD_CONTRAST_MAX
    #define LCD_CONTRAST_MAX 63
  #endif
  #ifndef DEFAULT_LCD_CONTRAST
    #define DEFAULT_LCD_CONTRAST 32
  #endif
#endif

Under 1.1.x 1.1.x, HAS_LCD_CONTRAST was defined differently, so that ifndef actually made some sense:

  #define HAS_LCD_CONTRAST ( \
      ENABLED(MAKRPANEL) \
   || ENABLED(CARTESIO_UI) \
   || ENABLED(VIKI2) \
   || ENABLED(miniVIKI) \
   || ENABLED(ELB_FULL_GRAPHIC_CONTROLLER) \
  )

@Str8t I wonder what happens if you apply this patch as a workaround. It makes the LCD visible again on my CR-20. It doesn't fix LCD contrast handling, it just disables it:

diff --git a/Marlin/src/inc/Conditionals_LCD.h b/Marlin/src/inc/Conditionals_LCD.h
index 54cbcacf6..c536ea977 100644
--- a/Marlin/src/inc/Conditionals_LCD.h
+++ b/Marlin/src/inc/Conditionals_LCD.h
@@ -169,12 +169,16 @@
 
 #endif
 
-#if EITHER(MAKRPANEL, MINIPANEL)
+#if ENABLED(MAKRPANEL)
   #define DOGLCD
   #define ULTIPANEL
   #ifndef DEFAULT_LCD_CONTRAST
     #define DEFAULT_LCD_CONTRAST 17
   #endif
+#elif ENABLED(MINIPANEL)
+  #define DOGLCD
+  #define ULTIPANEL
+  #undef DEFAULT_LCD_CONTRAST   // fixme: contrast handling currently broken; see https://github.com/MarlinFirmware/Marlin/issues/14174
 #endif
 
 #if ENABLED(ULTI_CONTROLLER)
@@ -353,6 +357,9 @@
  */
 #define HAS_LCD_CONTRAST (HAS_GRAPHICAL_LCD && defined(DEFAULT_LCD_CONTRAST))
 #if HAS_LCD_CONTRAST
+  #if ENABLED(MINIPANEL)
+    #error "This error should not be triggered" // See https://github.com/MarlinFirmware/Marlin/issues/14174
+  #endif
   #ifndef LCD_CONTRAST_MIN
     #define LCD_CONTRAST_MIN 0
   #endif

@0x3333
Copy link

0x3333 commented Aug 11, 2019

Same issue here. CR-20 Pro. My display doesn't work. Stock firmware. The above patch doesn't work. Printer is functional via Serial, but display in blank.

@0x3333
Copy link

0x3333 commented Aug 11, 2019

I made it work, here is my diff. I had to disable lcd contrast for my MKS_MINI_12864. I'll check why and eventually submit a PR.

diff --git a/Marlin/src/inc/Conditionals_LCD.h b/Marlin/src/inc/Conditionals_LCD.h
index b9d112321..c028edefb 100644
--- a/Marlin/src/inc/Conditionals_LCD.h
+++ b/Marlin/src/inc/Conditionals_LCD.h
@@ -128,8 +128,8 @@
 #elif ENABLED(MKS_MINI_12864)

   #define MINIPANEL
-  #define DEFAULT_LCD_CONTRAST 150
-  #define LCD_CONTRAST_MAX 255
+  // #define DEFAULT_LCD_CONTRAST 150
+  // #define LCD_CONTRAST_MAX 255

 #elif ANY(FYSETC_MINI_12864_X_X, FYSETC_MINI_12864_1_2, FYSETC_MINI_12864_2_0, FYSETC_MINI_12864_2_1)

@@ -190,9 +190,9 @@
   #if ENABLED(MAKRPANEL)
     #define U8GLIB_ST7565_64128N
   #endif
-  #ifndef DEFAULT_LCD_CONTRAST
-    #define DEFAULT_LCD_CONTRAST 17
-  #endif
+  // #ifndef DEFAULT_LCD_CONTRAST
+  //   #define DEFAULT_LCD_CONTRAST 17
+  // #endif
 #endif

 #if ENABLED(IS_U8GLIB_SSD1306)

@Str8t
Copy link
Author

Str8t commented Aug 12, 2019

First off I have to apologize that I did not upload my changes yet. I will try to do this the following days.

@dlitz Like I already wrote, I made the contrast handling work by adjusting the LCD_CONTRAST defines (you can read in my first post why the defaults are wrong it this case).
If you don´t need it you can just disable it of course.

@0x3333
Copy link

0x3333 commented Aug 12, 2019

@Str8t I've tried changing the defines. But didn't worked out to me. As I don't need the contrast at this time, removing it fixed for now.

@thisiskeithb
Copy link
Member

Is there any downside to implementing this change so all configs with MINIPANEL (and subsequently MKS_MINI_12864) will use these values?

@thinkyhead
Copy link
Member

thinkyhead commented Aug 14, 2019

The HAS_LCD_CONTRAST define only refers to an adjustable contrast. Most (if not all) graphical display variants get initialized with a contrast value, provided by DEFAULT_LCD_CONTRAST. It makes sense to revert the definition of HAS_LCD_CONTRAST back to the more explicit:

  #define HAS_LCD_CONTRAST( \
      ENABLED(MAKRPANEL) \
   || ENABLED(CARTESIO_UI) \
   || ENABLED(VIKI2) \
   || ENABLED(miniVIKI) \
   || ENABLED(ELB_FULL_GRAPHIC_CONTROLLER) \
  )

As other panels come along with adjustable contrast, they can be added to the list.

@thisiskeithb
Copy link
Member

thisiskeithb commented Aug 14, 2019

Continuation of #13732...

For the Tevo Michelangelo config (BOARD_MKS_GEN_L/MKS_MINI_12864 LCD), changing the HAS_LCD_CONTRAST define as you outlined above brings back the bootscreen, but there's no option to configure the contrast (Unknown command: "M250 C200") and the LCD is readable, but very dim.

Adding another || option for MKS_MINI_12864 brings back the LCD contrast option, but it's also very dim ("LCD Contrast: M250 C150" according to M503 after a clean flash) and the the bootscreen is no longer shown, but the LCD contrast is configurable.

M502/M500 was ran after each firmware flash to ensure default settings were loaded.

The contrast should be set to ~200 for the LCD to look "normal".

Just for good measure, here's the block of code I was working with in Conditionals_LCD.h for review in case I stuffed it up:

/**
 * Default LCD contrast for Graphical LCD displays
 */
//#define HAS_LCD_CONTRAST (HAS_GRAPHICAL_LCD && defined(DEFAULT_LCD_CONTRAST))
//#define HAS_LCD_CONTRAST (ENABLED(MAKRPANEL) || ENABLED(CARTESIO_UI) || ENABLED(VIKI2) || ENABLED(miniVIKI)  || ENABLED(MKS_MINI_12864))
//#define HAS_LCD_CONTRAST (ENABLED(MAKRPANEL) || ENABLED(CARTESIO_UI) || ENABLED(VIKI2) || ENABLED(miniVIKI) || ENABLED(MINIPANEL))
#define HAS_LCD_CONTRAST (ENABLED(MAKRPANEL) || ENABLED(CARTESIO_UI) || ENABLED(VIKI2) || ENABLED(miniVIKI))

@thisiskeithb
Copy link
Member

thisiskeithb commented Aug 14, 2019

For the Wanhao Duplicator i3 Mini config (BOARD_WANHAO_ONEPLUS w/ integrated LCD, now defined as MINIPANEL), changing the HAS_LCD_CONTRAST define as you outlined above brings back the bootscreen, but there's no option to configure the contrast (Unknown command: "M250 C200") and the LCD is readable, but very dim.

Adding another || option for MINIPANEL does not bring back the LCD contrast option, and the LCD is too dim to read ("LCD Contrast: M250 C17" according to M503 after a clean flash) so I can't verify if the bootscreen is shown or not.

@thinkyhead: You answered my questions with some options in my PR/#14908, so troubleshooting this config further might be irrelevant.

M502/M500 was ran after each firmware flash to ensure default settings were loaded.

The contrast should be set to max/255 for the LCD to look "normal" since it's a black LCD with white text.

Just for good measure, here's the block of code I was working with in Conditionals_LCD.h for review in case I stuffed it up:

/**
 * Default LCD contrast for Graphical LCD displays
 */
//#define HAS_LCD_CONTRAST (HAS_GRAPHICAL_LCD && defined(DEFAULT_LCD_CONTRAST))
//#define HAS_LCD_CONTRAST (ENABLED(MAKRPANEL) || ENABLED(CARTESIO_UI) || ENABLED(VIKI2) || ENABLED(miniVIKI)  || ENABLED(MKS_MINI_12864))
//#define HAS_LCD_CONTRAST (ENABLED(MAKRPANEL) || ENABLED(CARTESIO_UI) || ENABLED(VIKI2) || ENABLED(miniVIKI) || ENABLED(MINIPANEL))
#define HAS_LCD_CONTRAST (ENABLED(MAKRPANEL) || ENABLED(CARTESIO_UI) || ENABLED(VIKI2) || ENABLED(miniVIKI))

@thinkyhead
Copy link
Member

The contrast should be set to max/255 for the LCD to look "normal" since it's a black LCD with white text.

From all this it sounds like all MKS_MINI_12864 ought to have adjustable contrast … and maybe all MINIPANEL should have adjustable contrast?

@thisiskeithb
Copy link
Member

I would think so, but I don’t have more hardware to do further testing. Hopefully others will report their findings as well.

@thinkyhead
Copy link
Member

Hopefully the now-merged PR will do the trick. If not we can re-open this.

@github-actions
Copy link

github-actions bot commented Jul 4, 2020

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants