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

MPC Edit Menu compilation error when compiling for multi-hotends #24049

Conversation

zeleps
Copy link
Contributor

@zeleps zeleps commented Apr 15, 2022

Description

Compiling the latest bugfix with MPC_EDIT_MENU, multiple hotends, produces the following errors:

Marlin\src\lcd\menu\menu_advanced.cpp:355:13: error: 'c' is not captured
Marlin\src\lcd\menu\menu_advanced.cpp:369:61: error: 'mpc_edit_hotend' is not captured

Lambdas need byref capturing to use variables c / mpc_edit_hotend, but capturing lambdas have a different signature and are not convertible to screenFunc_t type. Converted mpc_edit_hotend to a static function and created a static index to the currently edited hotend in order to fix the issue.

I'm not very experienced with C++ lambdas, if there's another way to fix this, I'd love to see it.

Related Items

PR #23984

@zeleps zeleps mentioned this pull request Apr 15, 2022
@tombrazier
Copy link
Contributor

tombrazier commented Apr 15, 2022

This code is thinkyhead's improvement on what I wrote. I suspect mpc_edit_hotend should actually be a #define rather than a function. Menus are very clever but they still leave me a bit confused. What happens if you replace mpc_edit_hotend with

#define MPC_EDIT_HOTEND(N) \
  MPC_EDIT_DEFS(N); \
  START_MENU(); \
  BACK_ITEM(MSG_TEMPERATURE); \
  MPC_EDIT_ITEMS(N); \
  END_MENU();

and then use

#define MPC_ENTRY(N) SUBMENU_N(N, MSG_MPC_EDIT, []{ MPC_EDIT_HOTEND(MenuItemBase::itemIndex); });

@zeleps
Copy link
Contributor Author

zeleps commented Apr 15, 2022

I'm getting this now:

Marlin\src\lcd\menu\menu_advanced.cpp:373:1: error: macro "SUBMENU_N_P" passed 4 arguments, but takes just 3

It seems there is an issue with the expansion of macros START_MENU() and END_MENU() inside the lambda body. Still, when I remove these, I'm getting local variable not captured errors.

@thinkyhead thinkyhead force-pushed the bugfix-2.0.x.-FixMPCMenuMultiHotend branch from bbf9a19 to a12a0ce Compare April 18, 2022 04:16
@thinkyhead thinkyhead force-pushed the bugfix-2.0.x.-FixMPCMenuMultiHotend branch from a12a0ce to 97e3720 Compare April 18, 2022 04:28
@@ -366,7 +366,7 @@ void menu_backlash();
MPC_EDIT_ITEMS(e);
END_MENU();
};
#define MPC_ENTRY(N) SUBMENU_N(N, MSG_MPC_EDIT, []{ mpc_edit_hotend(MenuItemBase::itemIndex); });
#define MPC_ENTRY(N) SUBMENU_N(N, MSG_MPC_EDIT, [&]{ mpc_edit_hotend(MenuItemBase::itemIndex); });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only change needed is to use the same default capture for the outer lambda as the inner lambda.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that first, along with line 354, doesn't compile:

Marlin\src\lcd\menu\menu_item.h:275:40: error: cannot convert 'menu_advanced_temperature()::<lambda()>' to 'screenFunc_t' {aka 'void (*)()'}

@thinkyhead
Copy link
Member

What happens if you replace mpc_edit_hotend with…

You end up with a distinct function for each hotend, instead of just one function that takes the hotend index.

@thinkyhead thinkyhead merged commit c58c5b0 into MarlinFirmware:bugfix-2.0.x Apr 18, 2022
@zeleps
Copy link
Contributor Author

zeleps commented Apr 18, 2022

@thinkyhead, this still doesn't compile:

Marlin\src\lcd\menu\menu_advanced.cpp:355:54: error: 'c' is not captured
Marlin\src\lcd\menu\menu_item.h:275:40: error: cannot convert 'menu_advanced_temperature()::<lambda()>' to 'screenFunc_t' {aka 'void (*)()'}

image

@tombrazier
Copy link
Contributor

Confirmed. I get the same compile error with MPC_EDIT_MENU and HAS_MULTI_HOTEND.

@thisiskeithb
Copy link
Member

@zeleps: You'll need to submit a new PR since this one is already merged.

@thinkyhead
Copy link
Member

thinkyhead commented Apr 20, 2022

Missing capture? Usually you can just change []{ ... } to [&]{ ... } and that solves it. But, that may not work for local autos.

- EDIT_ITEM_FAST_N(float43, N, MSG_MPC_AMBIENT_XFER_COEFF_FAN_E, &editable.decimal, 0, 1, []{ \
+ EDIT_ITEM_FAST_N(float43, N, MSG_MPC_AMBIENT_XFER_COEFF_FAN_E, &editable.decimal, 0, 1, [&]{ \

@zeleps
Copy link
Contributor Author

zeleps commented Apr 20, 2022

Missing capture? Usually you can just change []{ ... } to [&]{ ... } and that solves it. But, that may not work for local autos.

- EDIT_ITEM_FAST_N(float43, N, MSG_MPC_AMBIENT_XFER_COEFF_FAN_E, &editable.decimal, 0, 1, []{ \
+ EDIT_ITEM_FAST_N(float43, N, MSG_MPC_AMBIENT_XFER_COEFF_FAN_E, &editable.decimal, 0, 1, [&]{ \

Of course I tried that (I mention it in the PR description), but it doesn't work:

Marlin\src\lcd\menu\menu_item.h:275:40: error: cannot convert 'menu_advanced_temperature()::<lambda()>' to 'screenFunc_t' {aka 'void (*)()'}

If I use [&] anywhere, the signature becomes incompatible with the ::action() method (explicitly casting it also fails). If I leave it as is, capture issues occur.

I did a lot of different tests before ending up with the static function as a last resort.

Configuration-wise, enable MPC edit menu and #define HAS_MULTI_HOTEND 1 at some point before the function (e.g. @256). This commit demonstrates the problem.

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