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

Fix menus responsiveness when HAS_MARLINUI_U8GLIB. #26555

Merged
merged 3 commits into from
Jul 15, 2024

Conversation

mh-dm
Copy link
Contributor

@mh-dm mh-dm commented Dec 20, 2023

Description

Fix responsiveness when moving through marlin menus (U8GLIB). Affects menu_media / SD contents menu the most since that's slower to draw to begin with.

Also, for SCROLL_LONG_FILENAMES add guarding to avoid redrawing menu_media screen if there's nothing to scroll (further responsiveness improvement).

Benefits

Fixes this sort of bad responsiveness:
https://github.com/MarlinFirmware/Marlin/assets/299015/8481f501-3f48-4217-89c4-f821eaa73e12
Bad responsiveness that caused me to start the wrong print quite a few times.

The issue is caused by drawing_screen overriding the correct handling of button/encoder events (that call refresh(LCDVIEW_REDRAW_NOW)). This PR fixes that interference. There's still a corner case and that's handled by restarting the screen draw from the top (achieved with drawing_screen = false), prioritizing responsiveness.

Requirements

HAS_MARLINUI_U8GLIB / ENABLED(DOGLCD)

@XDA-Bam
Copy link
Contributor

XDA-Bam commented Jan 3, 2024

I just tested this on my machine (bugfix-2.1.x @ 06dc7f4, SKR Mini E3 v1.2) and I think I encountered a bug: When the last file name on the SD card is long enough that it should scroll, it will not scroll under certain circumstances. When moving down the file list from the top to the very bottom, the filename won't scroll. When scrolling up one item, then down again to the bottom one, it will then scroll the filename.

I have to mention that after the recent encoder improvements (#26501), my machine is affected by bug #26605. So I don't know if this is somehow connected to this PR. I can say, however, that even when the encoder direction changes and that encoder step is missed due to the bug, long filenames which are not at the very end of the list still do scroll fine.

@mh-dm mh-dm force-pushed the fix-responsiveness branch from 6a226dd to 46b595b Compare January 17, 2024 15:00
@mh-dm
Copy link
Contributor Author

mh-dm commented Jan 17, 2024

@XDA-Bam Thank you for trying it and finding that bug. It was due filename_scroll_hash which wasn't getting updated when moving from a non-file to a file entry. So if you were to go to a file entry then to a non-file entry then back to the original file entry, the filename_scroll_hash doesn't change so MarlinUI::scrolled_filename() doesn't reset itself to initiate scrolling. While I tried to fix it by also resetting the filename_scroll_hash on encoder changes, that's not a good fix. The big problem is that filename_scroll_hash is a hash and hashes have collisions, especially uint8_t hashes. So I replaced the whole hashing system with a more reliable mechanism around encoder changes. Also rebased on top of bugfix-2.1.x head and moved filename_scroll_* so it's only accessible within marlinui.cpp as it's an implementation detail that shouldn't be exposed.

@XDA-Bam
Copy link
Contributor

XDA-Bam commented Jan 22, 2024

Just tested the latest changes with bugfix-2.1.x @ 22fc07d. I can confirm that the filename scrolling bug is fixed and I encountered no problems at all dashing around the menus for a minute or two. Everything is nice and snappy now! 😃

@mh-dm
Copy link
Contributor Author

mh-dm commented Mar 29, 2024

This small patch has been on my printer with no issues and no wrong print starts for the last two months.
Including folks @dbuezas @thinkyhead that are involved in somewhat related efforts like #26723 encoder changes. This patch fixes display issues rather than encoder logic but some symptoms (scrolling issues) may superficially look similar. Note I'm happy to rebase and retest (and one minor cleanup) if merging this is possible soon.

@mh-dm mh-dm force-pushed the fix-responsiveness branch from 46b595b to 96d5213 Compare April 30, 2024 10:24
@mh-dm mh-dm force-pushed the fix-responsiveness branch from 96d5213 to f64c1c1 Compare April 30, 2024 11:22
@mh-dm
Copy link
Contributor Author

mh-dm commented Apr 30, 2024

Synced and retested. Ready to be merged.

@sjasonsmith sjasonsmith requested review from sjasonsmith and removed request for sjasonsmith May 2, 2024 04:02
@mh-dm
Copy link
Contributor Author

mh-dm commented May 11, 2024

@thisiskeithb Sorry if you're not the right person but can you please help get this fix PR a review?

@sjasonsmith sjasonsmith self-requested a review May 11, 2024 15:12
@sjasonsmith
Copy link
Contributor

This all makes sense to me, until I get to all the drawing_screen resets in the bottom of marlinui.cpp. At this point I'm not sure whether every instance is correct, as I don't know this code well.

You probably have studied it out and understand it better than me, but @thisiskeithb recommended that we hold off on merging this if there is a release imminent, as it needs some runtime to make sure it doesn't introduce new issues for anyone.

Only @thinkyhead knows what's coming up as far as releases, so while I think this is ready to go (with just my one minor suggestion), we'll need to wait for him to decide on the timing.

@sjasonsmith sjasonsmith requested a review from thinkyhead May 11, 2024 15:43
@sjasonsmith
Copy link
Contributor

@thisiskeithb how much runtime do you think we would need in bugfix to feel good that issues would have been reported? Is a week enough?

Co-authored-by: Jason Smith <jason.inet@gmail.com>
@dbuezas
Copy link
Contributor

dbuezas commented May 26, 2024

I'm testing this now and the difference is noticeable. It seems like it also somehow fixes the remaining occasional missed and reversed encoder steps.
I also have DOUBLE_LCD_FRAMERATE and the faster full buffer and delta updates LCD drawing method (MarlinFirmware/U8glib-HAL#31) so everything was very snappy already.

+1 for merge Noticed no issues, fixed bugs (encoder and long filename scrolling), and a more responsive menu

@dbuezas
Copy link
Contributor

dbuezas commented May 26, 2024

@XDA-Bam you should try this one now that the 2nd encoder fixes MR was merged. Both combined are fantastic. I'd be curious if you this also solves the remaining encoder hiccups you observed

@XDA-Bam
Copy link
Contributor

XDA-Bam commented May 31, 2024

@dbuezas Can confirm your findings for my CR10_STOCKDISPLAY after a quick test scrolling the menu. I can see no more (or extremely rare) double stepping, no or really minimal step losses regardless of scroll speed and inputs just feel snappy and precise. The difference in encoder precision compared to my last firmware from mid april is huge.

Currently running without DOUBLE_LCD_FRAMERATE and didn't have the time to print with this PR in place. The only downside seems to be that LCD updates are so fast that the former can't really keep up anymore and UI elements like lines light up "half way" during scrolling because the moment they start to get activated, the next LCD refresh already arrives and turns them off again. A bit hard to describe without a video, but as far as I can tell this is just a limitation of the crappy 12864 LCD on my modded Ender 3 Pro and not a problem with this PR.

@dbuezas
Copy link
Contributor

dbuezas commented May 31, 2024

The overlapping updates you observe are good news for this PR, the menu is updated while rendering.
You could try increasing the i2c clock speed or using double buffering if you have ram to spare (or both like I do)

@thinkyhead
Copy link
Member

Sorry this has been hanging out so long. Everything here still stable with no interim changes stepping on anything?

@thinkyhead thinkyhead merged commit ee99eed into MarlinFirmware:bugfix-2.1.x Jul 15, 2024
62 checks passed
@mh-dm mh-dm deleted the fix-responsiveness branch July 15, 2024 21:28
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.

6 participants