prusa3d / Prusa-Firmware

Firmware for Original Prusa i3 3D printer by PrusaResearch
GNU General Public License v3.0
1.99k stars 1.05k forks source link

ultralcd: merge two functions into one as cleanup #4645

Closed gudnimg closed 3 months ago

gudnimg commented 3 months ago

lcd_show_fullscreen_message_yes_no_and_wait_P and lcd_show_multiscreen_message_yes_no_and_wait_P are the same function. Let's drop one of them so we only have one symbol for the function.

Noticed this when reviewing https://github.com/prusa3d/Prusa-Firmware/pull/4644

No change in memory

github-actions[bot] commented 3 months ago

All values in bytes. Δ Delta to base

Target ΔFlash ΔSRAM Used Flash Used SRAM Free Flash Free SRAM
MK3S_MULTILANG 0 0 247228 5653 6724 2539
MK3_MULTILANG -4 0 246506 5662 7446 2530
3d-gussner commented 3 months ago

Can't we also remove lcd_show_yes_no_and_wait as it is only used once during rare crash. Not showing the ? after Resume saves 28 bytes Before

  0123456789012356789
0|Resume print?
1|
2|
3|>Yes      No

after

  0123456789012356789
0|Resume print
1|
2|
3|>Yes      No

[Uploading MK3_Resume_print_during_crash.diff.txt…]()

Alternative is to replace lcd_show_yes_no_and_wait in crashdet_detected function to lcd_show_multiscreen_message_yes_no_and_wait_P(NULL, false, LCD_LEFT_BUTTON_CHOICE); that saves 8 additional bytes

gudnimg commented 3 months ago

Can't we also remove lcd_show_yes_no_and_wait as it is only used once during rare crash. Not showing the ? after Resume saves 28 bytes Before

  0123456789012356789
0|Resume print?
1|
2|
3|>Yes      No

after

  0123456789012356789
0|Resume print
1|
2|
3|>Yes      No

Uploading MK3_Resume_print_during_crash.diff.txt…

Alternative is to replace lcd_show_yes_no_and_wait in crashdet_detected function to lcd_show_multiscreen_message_yes_no_and_wait_P(NULL, false, LCD_LEFT_BUTTON_CHOICE); that saves 8 additional bytes

We could do that, but I feel that should be a separate PR. I'm not sure if its worth it to lose the ?

This PR should not be changing any UI and is only cleanup

3d-gussner commented 3 months ago

@gudnimg This PR got conflicts after the move of all _i() messages, please fix it or let me know if should do that.

3d-gussner commented 3 months ago

@gudnimg Rebased it, please double check and feel free to merge.

gudnimg commented 3 months ago

Rebase looks correct to me, merging