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

Increase Default MKS MINI 12864 Init Contrast #15139

Conversation

thisiskeithb
Copy link
Member

@thisiskeithb thisiskeithb commented Sep 2, 2019

Description

The default init LCD contrast of 150 for the MKS_MINI_12864 is still too low. This PR bumps it to 195.

Benefits

Increases the default contrast to make the LCD readable without having to increase it via LCD or gcode.

Related Issues

#14174, #14889

Also, custom bootscreens still aren't shown, but I don't think that's related to this LCD contrast issue See my comment below.

@thisiskeithb
Copy link
Member Author

thisiskeithb commented Sep 2, 2019

Upon further testing, without _LCD_CONTRAST_MIN, custom bootscreens don't show up. Adding it and giving it the same value as _LCD_CONTRAST_INIT, custom bootscreens show up (and with the correct contast)!

...but then users can't change the contrast below 195.

Is there a way for Marlin to use _LCD_CONTRAST_INIT while booting instead of _LCD_CONTRAST_MIN?

@thinkyhead
Copy link
Member

thinkyhead commented Sep 3, 2019

Is there a way for Marlin to use _LCD_CONTRAST_INIT while booting instead of _LCD_CONTRAST_MIN?

Try M502 followed by M500 to get your updated contrast to take effect on boot.

@thinkyhead thinkyhead merged commit cdd5056 into MarlinFirmware:bugfix-2.0.x Sep 3, 2019
@thisiskeithb
Copy link
Member Author

That’s part of my flashing procedure in OctoPrint (using the FirmwareUpdater plugin), but I’ll give another go manually.

@thisiskeithb
Copy link
Member Author

thisiskeithb commented Sep 3, 2019

@thinkyhead: It's still not working correctly with this commit (but at least the custom boot screen is showing up), even after running M502 & M500.

It's using the _LCD_CONTRAST_MIN value until part way through the Marlin logo and then it'll set the correct contrast using the _LCD_CONTRAST_INIT value.

Here's a video of what happens: https://youtu.be/_98_RRU3d28

@thinkyhead
Copy link
Member

_LCD_CONTRAST_INIT is not used anywhere in the code directly. You'll have to add #error lines here and there to see where DEFAULT_LCD_CONTRAST is being defined and maybe add some logging to figure out where in the process the contrast is being set / initialized.

@thisiskeithb
Copy link
Member Author

Printers with MINIPANEL also suffer from this issue of custom bootscreens not showing up, so it'd be worth tracking down.

I'll see if I can figure out where this is happening in the code and more importantly, if I can fix it. 😄

@thisiskeithb thisiskeithb deleted the Increase-MKS_MINI_12864-Init-Contrast branch September 3, 2019 01:04
@ManuelMcLure
Copy link
Contributor

ManuelMcLure commented Sep 3, 2019

It looks like the jump in contrast half way through the second boot screen is probably occurring at line 2468 of configuration_store.cpp:

2467   #if HAS_LCD_CONTRAST
2468     ui.set_contrast(DEFAULT_LCD_CONTRAST);
2469   #endif

That's in the configuration reset so it ends up being stored in EEPROM. Then when the configuration is loaded:

1632       //
1633       // LCD Contrast
1634       //
1635       {
1636         _FIELD_TEST(lcd_contrast);
1637 
1638         int16_t lcd_contrast;
1639         EEPROM_READ(lcd_contrast);
1640         #if HAS_LCD_CONTRAST
1641           ui.set_contrast(lcd_contrast);
1642         #endif
1643       }

@thisiskeithb
Copy link
Member Author

It looks like the jump in contrast half way through the second boot screen is probably occurring at line 2468 of configuration_store.cpp

I was just sifting through all the "contrast" results, so thanks for hunting that down @ManuelMcLure 😄I'll play with the code and see what I can come up with.

@ManuelMcLure
Copy link
Contributor

GNU Global helps a lot in tracking down this type of thing.

@thisiskeithb
Copy link
Member Author

I mainly stick to board pins & printer configs, but I'm slowly branching out to other parts of Marlin. I'll work on it tonight and report back.

@thisiskeithb
Copy link
Member Author

It's above my paygrade. I can't seem to get the correct contrast setting to initialize sooner than it already is, so I'll have to leave it to someone else to figure out down the road.

Thanks for trying to help @ManuelMcLure!

@Patag
Copy link

Patag commented Sep 3, 2019

FWIW
I'm currently using this workaround in ultralcd_DOGM.cpp at line 300 or so.

  #if HAS_LCD_CONTRAST
-   refresh_contrast();
+   set_contrast(DEFAULT_LCD_CONTRAST);
  #endif

May be this will help to track down the root cause.

@thinkyhead
Copy link
Member

thinkyhead commented Sep 4, 2019

It looks like the jump in contrast half way through

EEPROM reset is only done if no EEPROM exists or the saved data could not be loaded.

I'm currently using this workaround in ultralcd_DOGM.cpp at line 300 or so.

That's a reasonable solution. The default contrast will be used if the EEPROM has not yet loaded. And if it has loaded, then the last-saved contrast will be used.

Here's another proposal:

  #if HAS_LCD_CONTRAST

-   int16_t MarlinUI::contrast; // Initialized by settings.load()
+   int16_t MarlinUI::contrast = DEFAULT_LCD_CONTRAST;

    void MarlinUI::set_contrast(const int16_t value) {
      contrast = constrain(value, LCD_CONTRAST_MIN, LCD_CONTRAST_MAX);
      u8g.setContrast(contrast);
    }

  #endif

@thisiskeithb
Copy link
Member Author

I'm currently using this workaround in ultralcd_DOGM.cpp at line 300 or so.

I haven't tested it yet, but it sounds good to me!

There's still a bunch of LCD issues going on over in #15141, so we'll see.

@thinkyhead
Copy link
Member

More likely SPI issues.

thinkyhead added a commit that referenced this pull request Sep 4, 2019
@thisiskeithb
Copy link
Member Author

thisiskeithb commented Sep 4, 2019

FWIW
I'm currently using this workaround in ultralcd_DOGM.cpp at line 300 or so.

  #if HAS_LCD_CONTRAST
-   refresh_contrast();
+   set_contrast(DEFAULT_LCD_CONTRAST);
  #endif

May be this will help to track down the root cause.

This works great! @thinkyhead: Should I put in a PR for this change?

Edit: Just saw the commit.

@thisiskeithb
Copy link
Member Author

060b360 also fixes the blank custom bootscreen issue on MINIPANEL as well. Thanks @Patag!

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.

4 participants