Marlin
Marlin copied to clipboard
MarlinFirmware
Fix menus responsiveness when HAS_MARLINUI_U8GLIB.
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)
I just tested this on my machine (bugfix-2.1.x @ 06dc7f4f52, 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.
@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.
Just tested the latest changes with bugfix-2.1.x @ 22fc07d72b. 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! 😃
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.
@thisiskeithb Sorry if you're not the right person but can you please help get this fix PR a review?
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.
@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?
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 (https://github.com/MarlinFirmware/U8glib-HAL/pull/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
@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
@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.
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)
Sorry this has been hanging out so long. Everything here still stable with no interim changes stepping on anything?
