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

Streamline menu item logic #17664

Conversation

thinkyhead
Copy link
Member

@thinkyhead thinkyhead commented Apr 23, 2020

Background: Menu items are processed in a loop from the top of the screen to the bottom, so any visible menu items will be processed up to 5 times per menu screen. This wastes processing and may cause unwanted side-effects since any un-protected function calls inside of menus will also be repeated.

Solutions:

  • Add the MENU_ITEM_IF() macro to replace if clauses inside of menu screens. The clause essentially gets moved "inside" the menu item, so it will only be run when the current menu item is being evaluated instead of 4 or 5 times per screen handler call.
  • Get conditions for item display at the top of menus, outside of the loop, so they can be applied to more than one item with only the single call.

The compiler will optimize out the inner check for "is this the current menu item?" (implicit in MENU_ITEM(...) when the outer check is used, so there is no extra cost to using MENU_ITEM_IF.

@thinkyhead thinkyhead force-pushed the bf2_streamline_menu_logic_PR branch 20 times, most recently from 1118b1b to fc1bd9e Compare April 26, 2020 02:24
@thinkyhead thinkyhead marked this pull request as ready for review April 26, 2020 02:37
@thinkyhead thinkyhead force-pushed the bf2_streamline_menu_logic_PR branch 3 times, most recently from b81a3d2 to 53ac911 Compare April 27, 2020 11:35
@thinkyhead thinkyhead force-pushed the bf2_streamline_menu_logic_PR branch 3 times, most recently from 03dfbd8 to 0908f2d Compare April 28, 2020 03:39
@thinkyhead thinkyhead force-pushed the bf2_streamline_menu_logic_PR branch 2 times, most recently from 1ef6bf5 to e05000c Compare April 28, 2020 04:29
@thinkyhead thinkyhead merged commit 4f003fc into MarlinFirmware:bugfix-2.0.x Apr 28, 2020
@thinkyhead thinkyhead deleted the bf2_streamline_menu_logic_PR branch April 28, 2020 04:52
@Evg33
Copy link
Contributor

Evg33 commented Apr 29, 2020

SKR1.4T
pio upgrade
pio run -t clean
pio update
pio run

Compiling .pio\build\LPC1769\src\src\lcd\menu\menu_main.cpp.o
In file included from Marlin\src\lcd\menu\menu_info.cpp:31:
Marlin\src\lcd\menu\menu_info.cpp: In function 'void menu_info_printer()':
Marlin\src\lcd\menu\menu.h:443:50: error: expected unqualified-id before ')' token
  443 |     MenuItem_static::draw(_lcdLineNr, PLABEL, ##V);     \
      |                                                  ^
Marlin\src\lcd\menu\menu.h:448:5: note: in expansion of macro 'STATIC_ITEM_INNER_P'
  448 |     STATIC_ITEM_INNER_P(PLABEL, ##V);   \
      |     ^~~~~~~~~~~~~~~~~~~
Marlin\src\lcd\menu\menu.h:460:39: note: in expansion of macro 'STATIC_ITEM_P'
  460 | #define STATIC_ITEM(LABEL,      V...) STATIC_ITEM_P(  GET_TEXT(LABEL), ##V)
      |                                       ^~~~~~~~~~~~~
Marlin\src\lcd\menu\menu_info.cpp:246:5: note: in expansion of macro 'STATIC_ITEM'
  246 |     STATIC_ITEM(
      |     ^~~~~~~~~~~
*** [.pio\build\LPC1769\src\src\lcd\menu\menu_info.cpp.o] Error 1
============================================ [FAILED]

thinkyhead added a commit that referenced this pull request Apr 29, 2020
@thinkyhead
Copy link
Member Author

Thanks! That error got patched along the way.

@thisiskeithb
Copy link
Member

thisiskeithb commented Apr 29, 2020

@thinkyhead: Several items are missing from the menus after this (Home All/ PLA preheat, Cooldown, just to make a few). I haven’t compared every menu, but something broke with this PR.

@thinkyhead
Copy link
Member Author

Yes, I expected some things to fail. Please diagnose if you find the time. I have only time to patch issues and no time to test them.

@geekgamer2001
Copy link

geekgamer2001 commented Apr 29, 2020

here is a full list of menu options that i found to be missing in the current version after reverting to 4f003fc now my printer does not have some other things so these are just the options available to me

Auto Home
Preheat PLA
Cooldown
about printer

@thinkyhead
Copy link
Member Author

I've got a better refactor coming pretty soon, so the MENU_ITEM_IF stuff is reverted for now.

jmp0x0000 pushed a commit to jmp0x0000/Marlin that referenced this pull request Aug 7, 2020
jmp0x0000 pushed a commit to jmp0x0000/Marlin that referenced this pull request Aug 7, 2020
njibhu pushed a commit to njibhu/Marlin that referenced this pull request Aug 24, 2020
njibhu pushed a commit to njibhu/Marlin that referenced this pull request Aug 24, 2020
HairingX pushed a commit to HairingX/Marlin that referenced this pull request Jun 16, 2021
HairingX pushed a commit to HairingX/Marlin that referenced this pull request Jun 16, 2021
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