✨♻️ Big progress display revision

[EvilGremlin]

Description

Basically this is unification of print progress that makes it hardware-agnostic and all-granular, overall way more convenient to work with. All options are as independent as they can be, which allows for easy UI customisation.

new_progress.webm

Benefits

M73 doesn't depend on display existence anymore.

M73_REPORT Prusa-like M73 that report values back. This allow hosts to get M73 progress from SD printing for somewhat accurate time estimates. Octoprint plugin: https://github.com/arekm/octopi_eta_override

USE_M73_INTERACTION_TIME, SHOW_INTERACTION_TIME Adds "time to user interaction". Prusa use it for filament change, but it can be anything, i.e. "pause at layer" - this will require some additional support from slicers.

PRINT_PROGRESS_SHOW_DECIMALS now affect every UI progress display uniformly, and M73 too.

ROTATE_PROGRESS_DISPLAY is gone as it lost all meaning. However much progress string displays you enable can be rotated in any native UI, or no rotation if you leave only one. New function iterates over pointer array on blink, so each blink next string is redrawn. This also allow for completely arbitrary string placement on screen: all rotated in one place, all separate, in any groups... only thing to care about is to not leave artifacts from previous string/value. Same approach can be used for rotation of any screen element at all, i.e. multiple hotend temps on 1602 LCD, possibly even all in one array to save resources on problematic configurations, like AVR running delta kinematic.

Bugfixes and small stuff

Added long overdue fs.img to gitignore.

permyriadtostr4 now always 4-width.

Fixed progress bar drawing in LIGHTWEIGHT_UI ( thanks Scawty - "The code is relying on int being 16-bit so that the arithmetic right shift fills the left side with ones. On an ESP32 (and arm) it's 32-bit and not being sign extended" )

LIGHTWEIGHT_UI now redraw progress bar even less often which should be saving even more cycles.

Things to do

Possibly need to expand capabilities report.

Some options to beep/flash lights when interaction time approach zero.

HD44780 strings need rewrite similar to LIGHTWEIGHT_UI to avoid artifacts when timer goes down from hundreds of hours.
permyriadtostr4 need further touchups: whole single digit values don't show decimal, doesn't look nice.

Related Issues

Draft with some notes: #24714

[EvilGremlin]

I don't like prusa wording too, progmem not infinite... I think we should ask @arekm if he's ok with wording change, and @foosel in case she ever intend to support this natively

[thisiskeithb]

Just to follow-up our chat on Discord, I think the "otherwise use Marlin's estimate" wording on these options is not correct due to a sanity check requiring them:

Marlin/Marlin/Configuration_adv.h

Lines 1440 to 1441 in cab94bb

	//#define SET_PROGRESS_PERCENT // Add 'P' parameter to set percentage done, otherwise use Marlin's estimate
	//#define SET_REMAINING_TIME // Add 'R' parameter to set remaining time, otherwise use Marlin's estimate

"otherwise use Marlin's estimate" can never be the case:

Marlin/Marlin/src/inc/SanityCheck.h

Lines 918 to 920 in cab94bb

	#if ENABLED(SET_PROGRESS_MANUALLY) && NONE(SET_PROGRESS_PERCENT, SET_REMAINING_TIME, SET_INTERACTION_TIME)
	#error "SET_PROGRESS_MANUALLY requires at least one of SET_PROGRESS_PERCENT, SET_REMAINING_TIME, SET_INTERACTION_TIME to be enabled."
	#endif

Also, here's the updated screen with SD <val>% now being P: <val>%:

Soooo, fixed?

[thinkyhead]

"otherwise use Marlin's estimate" can never be the case

Marlin can estimate the remaining time using the elapsed time and the file size when printing from SD. However, if the remaining time is set explicitly, then it should use that. The flag determining to "don't use Marlin's calculated estimate" should be cleared at the start of a print job, and set as soon as M73 R arrives.

[thinkyhead]

I see M73_REPORT_PRUSA stays and is default on and hardcoded. Will adjust octoprint plugin.

@arekm — The Octoprint plugin should handle the shorter format as well. Most of Octoprint eschews regular expressions, but you should use them anyway.

[thinkyhead]

Late to the party, but I would certainly welcome it as well if we could all agree on one wording. I don't care whether that is Prusa's or Scott's, but personally I'd go with what has been around longer and the other having to adjust.

It is for the sake of hosts that the default was left as Prusa's format. I included the option for a shorter format because it is always better to be concise in Marlin to keep the binary as small as possible, especially for popular and/or required features. I know, it's 2022, but I still want to keep a thought for those 128K or less AVRs out there.

[arekm]

Prusa format and Marlin M73_REPORT_PRUSA formats are slightly different, so hosts will still need to be updated for it anyway. And I only know one other host software which parses Prusa format. PrusaLink... that's used only with prusa printers.

I really don't see any benefit for this but well... octoprint plugin handles three formats currently (prusa + 2x marlin):
https://github.com/arekm/octopi_eta_override/blob/devel/octoprint_PrusaETAOverride/__init__.py#L41-L55 (not finished, needs testing with real firmware).

(I guess I'll add detecting which format to use since doing 3x regex (in worst case) on each gcode line isn't optimal.)

[thinkyhead]

Prusa format and Marlin M73_REPORT_PRUSA formats are slightly different,

I thought your intention was to perfectly duplicate the Prusa format. If it is not an exact duplicate, that should probably be fixed so that hosts can properly parse it.

[arekm]

I would go with the better (short Marlin; almost) format and drop second (Prusa) one like:
#24767 (comment)

[thinkyhead]

I would go with the better (short Marlin; almost) format and drop second (Prusa) one like:

Maybe so…. I would still like to get the Prusa format right. I can check their source code, if needed. Our short format is of course better because of the NIH rule, and in spite of my concerns above, I think it should be okay to use the shorter form as the default. Hosts should add it anyway; it will be associated specifically with Marlin 2.1.2 and above.

[arekm]

Prusa format is ugly:

NORMAL MODE: Percent done: 21; print time remaining in mins: 33; Change in mins: -1
SILENT MODE: Percent done: 21; print time remaining in mins: 34; Change in mins: -1

no "echo" at beginning, "MODE" which is something Marlin doesn't have AFAIK; negative values for not set times. No conditionals (all values are always available).

https://github.com/prusa3d/Prusa-Firmware/blob/992845198d1a2509ac98e0d7d4b24117e2980c65/Firmware/Marlin_main.cpp#L6098

[pmjdebruijn]

It seems this introduced a bug, where the progress indication continually blinks on/off when only a single SHOW is enabled:

Configuration_adv.h://#define SHOW_INTERACTION_TIME
Configuration_adv.h://#define SHOW_PROGRESS_PERCENT
Configuration_adv.h://#define SHOW_ELAPSED_TIME
Configuration_adv.h: #define SHOW_REMAINING_TIME

[EvilGremlin]

what display/UI? didn't see this happen

[pmjdebruijn]

This is on a classic Ender-3 Pro, so DOGM I guess?

[pmjdebruijn]

And after enabling:

#define SET_PROGRESS_MANUALLY
#define SET_PROGRESS_PERCENT
#define SET_REMAINING_TIME

I also get this:

Compiling .pio/build/BIGTREE_E3_RRF/src/src/lcd/menu/menu_filament.cpp.o
In file included from PlatformIO/packages/framework-arduinoststm32/cores/arduino/WString.h:29,
                 from PlatformIO/packages/framework-arduinoststm32/cores/arduino/Print.h:27,
                 from PlatformIO/packages/framework-arduinoststm32/cores/arduino/Stream.h:26,
                 from PlatformIO/packages/framework-arduinoststm32/cores/arduino/HardwareSerial.h:29,
                 from PlatformIO/packages/framework-arduinoststm32/cores/arduino/WSerial.h:5,
                 from PlatformIO/packages/framework-arduinoststm32/cores/arduino/wiring.h:47,
                 from PlatformIO/packages/framework-arduinoststm32/cores/arduino/Arduino.h:36,
                 from Marlin/src/lcd/dogm/../../inc/../HAL/./STM32/../shared/Marduino.h:36,
                 from Marlin/src/lcd/dogm/../../inc/../HAL/./STM32/HAL.h:28,
                 from Marlin/src/lcd/dogm/../../inc/../HAL/HAL.h:30,
                 from Marlin/src/lcd/dogm/../../inc/MarlinConfig.h:31,
                 from Marlin/src/lcd/dogm/dogm_Statusscreen.h:31,
                 from Marlin/src/lcd/dogm/status_screen_DOGM.cpp:32:
Marlin/src/lcd/dogm/../../module/../libs/duration_t.h: In static member function 'static void MarlinUI::draw_status_screen()':
Marlin/src/lcd/dogm/../../module/../libs/duration_t.h:161:30: warning: '%02hu' directive writing 2 bytes into a region of size between 0 and 4 [-Wformat-overflow=]
  161 |       sprintf_P(buffer, PSTR("%hud %02hu:%02hu"), d, h % 24, m);  // 1d 23:45
PlatformIO/packages/framework-arduinoststm32/cores/arduino/avr/pgmspace.h:71:40: note: in definition of macro 'sprintf_P'
   71 | #define sprintf_P(s, ...) sprintf((s), __VA_ARGS__)
      |                                        ^~~~~~~~~~~
Marlin/src/lcd/dogm/../../module/../libs/duration_t.h:161:25: note: in expansion of macro 'PSTR'
  161 |       sprintf_P(buffer, PSTR("%hud %02hu:%02hu"), d, h % 24, m);  // 1d 23:45
      |                         ^~~~
Marlin/src/lcd/dogm/../../module/../libs/duration_t.h:161:30: note: directive argument in the range [0, 59]
  161 |       sprintf_P(buffer, PSTR("%hud %02hu:%02hu"), d, h % 24, m);  // 1d 23:45
PlatformIO/packages/framework-arduinoststm32/cores/arduino/avr/pgmspace.h:71:40: note: in definition of macro 'sprintf_P'
   71 | #define sprintf_P(s, ...) sprintf((s), __VA_ARGS__)
      |                                        ^~~~~~~~~~~
Marlin/src/lcd/dogm/../../module/../libs/duration_t.h:161:25: note: in expansion of macro 'PSTR'
  161 |       sprintf_P(buffer, PSTR("%hud %02hu:%02hu"), d, h % 24, m);  // 1d 23:45
      |                         ^~~~
PlatformIO/packages/framework-arduinoststm32/cores/arduino/avr/pgmspace.h:71:34: note: 'sprintf' output between 9 and 13 bytes into a destination of size 10
   71 | #define sprintf_P(s, ...) sprintf((s), __VA_ARGS__)
      |                           ~~~~~~~^~~~~~~~~~~~~~~~~~
Marlin/src/lcd/dogm/../../module/../libs/duration_t.h:161:7: note: in expansion of macro 'sprintf_P'
  161 |       sprintf_P(buffer, PSTR("%hud %02hu:%02hu"), d, h % 24, m);  // 1d 23:45
      |       ^~~~~~~~~

My Ender-3 Pro has a SKR E3 RFF now, which is an STM32 (M4) based board...

[EvilGremlin]

yeah, overflow, i know

[thisiskeithb]

My Ender-3 Pro has a SKR E3 RFF now, which is an STM32 (M4) based board...

Same thing happens on LPC / other HALs, but I also alerted EvilGremlin via Discord. It's on their to-do list 🙂

[EvilGremlin]

continues here #24879