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

DOGM Display Screen Sleep #23927

Closed
wants to merge 11 commits into from

Conversation

borland1
Copy link
Contributor

Description

Adds Screen Sleep feature for U8g DOGM displays which supports Sleep and Wake functions.

  • #define SCREEN_TIMEOUT in Configuration.h file is, by default, commented out (disabled).
  • Adds to LCD Menu under Configuration Menu, a timeout setting menu.
  • User can disable screen timeout feature by setting timeout value to '0'.
  • Timeout setting changes will be saved to EEPROM with other setting changes being saved.
  • Supports timeout times between 1 and 99 minutes in 1 minute increments.
  • Rotation of rotary encoder, or press of rotary encoder button, wakes display when in display is in sleep mode.

Requirements

U8g DOGM supported displays.

Benefits

Helps reduce energy consumption during printing. Reduces pixel burn-in with OLED displays.

Configurations

Configuration.h file has been modified and is included with this PR.

Related Issues

None found.

Add Screen Timeout Feature for DOGM displays.
Adding #define HAS_SCREEN_TIMEOUT 1
Add to list: LSTR MSG_SCREEN_TIMEOUT  = _UxGT("ScreenTimeout(m)");
Add settings for SCREEN_TIMEOUT feature.
Add declarations for SCREEN_TIMEOUT feature.
Add SCREEN_TIMEOUT feature
Add menu EDIT_ITEM: SCREEN_TIMEOUT
Add  ui.sleep_on() and ui.sleep_off() for SCREEN_TIMEOUT feature.
@thisiskeithb
Copy link
Member

This would be good, but I think it should be extra clear in the config/header that this won't work on the most common 12864 displays (RepRapDiscount Full Graphic Smart Controller & Mini 12864 LCDs) because I can already see the support requests coming in asking why it doesn't work.

Adding some sanity checks to limit use would probably also help prevent hopeful LCD configs.

@borland1
Copy link
Contributor Author

Thanks for the good comments Keith, I don't know all the displays supported by Marlin, or the number of sales with each, but can, like you suggested, add some sanity checks in the SanityChecks.h file for a few of the displays listed in the Configuration.h file.

This PR's code does rely on or interfere with any code related to the TOUCH_IDLE_SLEEP or LCD_BACKLIGHT_TIMEOUT features currently in bugfix-2.0.x, and those are available to anyone with those type of displays needing those features.

I did place the #defineenabling line just below the OLED Displays heading, so it is somewhat hidden. I did write a lengthy paragraph explaining that the feature is only for displays that meet a specific hardware requirement, but needed to also explain functionality too, so I don't think it's appropriate to add more words there. What I provided in the comments should be sufficient.

@luxflow
Copy link
Contributor

luxflow commented Mar 20, 2022

I think adding sanity check would be sufficient

if filtering those display with driver type
https://github.com/MarlinFirmware/Marlin/blob/bugfix-2.0.x/Marlin/src/lcd/dogm/marlinui_DOGM.h

  • only for DOGLCD display (don't support HD44780 based display such as reprapdiscount smartcontroller 2004 lcd, e3v2)

DOGLCD always should be defined (since we use u8glib sleep feature)

  • for ssd1306

    • U8GLIB_SSD1306
    • MKS_12864OLED_SSD1306
  • for ssd1309

    • U8GLIB_SSD1309
    • FYSETC_242_OLED_12864, K3D_242_OLED_CONTROLLER
    • ZONESTAR_12864OLED_SSD1306
  • for sh1106

    • MKS_12864OLED, ZONESTAR_12864OLED
    • U8GLIB_SH1106_EINSTART
    • U8GLIB_SH1106
  • for st7565

    • IS_U8GLIB_ST7565_64128N
  • for lm6059

    • IS_U8GLIB_LM6059_AF

should be defined or enabled

I think these will cover most of use case isn't it?

Fixed typo on multi line comments, missing closing "*/" characters.
Merge with FR MarlinFirmware#23896  Sanity Checks for Screen Support Feature,  Changes adds checks for unsupported displays.
#error "SCREEN_TIMEOUT feature not supported by Reprap Discount Full Graphics Smart Controller using ST7920"
#elif ENABLED(CARTESIO_UI)
#error "SCREEN_TIMEOUT feature not supported by CARTESIO_UI LCD"
#elif IS_U8GLIB_LM6059_AF
Copy link
Contributor

@luxflow luxflow Mar 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I add these displays to the updates at borland1#1 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is ok to add

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, If you don't mind, could you merge u8glib-hal for supporting additional devices?

@luxflow
Copy link
Contributor

luxflow commented Mar 22, 2022

I PR to add some display sleep support for u8glib-hal which is backported from u8g2
After this PR is merged, we can add support for ST7920 and MINI12864 based display which are quite commonly used (reprap full graphic smart controller, mini12864, minipanel)

@thinkyhead
Copy link
Member

For some reason I'm unable to add commits directly to this PR so please merge borland1#1 to apply my updates.

In Screen Timeout section, delete sanity checks for 'HAS_MARLINUI_HD44780' and  'CARTESIO_UI'.  Reason: Screen Timeout feature is for DOGM displays, but HD44780 is not DOGM, while CARTESIO_UI displays are supported by Screen Timeout feature.   Merge changes with PR MarlinFirmware#23927
borland1 added a commit to thinkyhead/Marlin that referenced this pull request Mar 23, 2022
In Display Sleep sanity checks, removed check for 'HAS_MARLINUI_HD44780 which is not DOGM.  Also removed check for CARTESIO_UI which is supported by sleep/wake function.    Merge with PR MarlinFirmware#23927
@thinkyhead thinkyhead changed the title Add Screen Sleep feature for DOGM Displays (Bugfix 2.0.x), FR Report ( #23896 ) DOGM Display Screen Sleep Mar 24, 2022
@thinkyhead
Copy link
Member

All you need to do to merge my PR into this PR is to press the "Merge" button on the page borland1#1.

@borland1
Copy link
Contributor Author

Sorry for the untimely response, but I have not had time to do the merge because there are some problems with the changes you are proposing. Right now, I don't have spare time to work on a solution that implements a compromise. Some of your changes appear either out of scope, not well thought out, or are trivial. I should have something worked up to merge in three or four days.

@thinkyhead
Copy link
Member

Some of your changes appear either out of scope, not well thought out, or are trivial.

I've reviewed the changes in my cleanup and extension PR, and I consider them all pretty much mandatory. I would prefer that you merge my PR and then afterward apply your adjustments.

@thinkyhead thinkyhead closed this Apr 3, 2022
@thinkyhead thinkyhead mentioned this pull request Apr 3, 2022
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