Marlin

Marlin copied to clipboard

Description

This PR adds a menu item in ProUI whichs offers the user the ability to toggle encoder direction.
PROUI_ITEM_ENC

~~This also rearranges the menu layout to be less clumped. Before there was 20+ menu items in one. I have split them accordingly.~~ Moved to https://github.com/MarlinFirmware/Marlin/pull/26980

• Add reverse encoder option in menu

Reverted / Moved to #26917

• ftdi_eve_extui.cpp
• malyan_extui.cpp

Requirements

Benefits

Configurations

Related Issues

It's been requested before, but you should be sending PRs to @mriscoc's ProUI fork so changes & fixes can be consolidated & brought upstream once they are ready.

Is there a reason why you're not doing that?

It's been requested before, but you should be sending PRs to @mriscoc's ProUI fork so changes & fixes can be consolidated & brought upstream once they are ready.

Is there a reason why you're not doing that?

he has stated his fork is too far different. his changes are too far great to be brought here. he has also said to put any changes upstream here. also, he doesn't seem to do pull requests as ive made some and nothing has been done.

plus anyone using ProUI will use his fork for the extra special features which do not exist here because of the missing proprietary library.

changes here to the UI aren't likely going to make it there, and vice versa.

he has also said to put any changes upstream here.

Is that a new directive? I was going off these comments:

Good PR! @classicrocker883 thanks for your contributions, feel free to port the features that you consider helpful from my fork to Marlin, I think that this will help me henceforth to fully focus in my fork.

Originally posted by @mriscoc in https://github.com/MarlinFirmware/Marlin/issues/26086#issuecomment-1694792355

...and..

Note that after we get through this set of changes you should submit any changes we've made to mainline Marlin to the @mriscoc fork so that we are relatively in sync. In the long run it will be better for you to first submit any changes and fixes to that fork, and then we can coordinate with Miguel to bring those changes over en masse to this repo when he reaches a good milestone. The same goes for any other forks that are actively under development.

Originally posted by @thinkyhead in https://github.com/MarlinFirmware/Marlin/issues/26563#issuecomment-1868598784

---

I think there comes a point where it's no longer "Pro UI by MRiscoC" if code has diverged so far that they can't be reconciled.

I think there comes a point where it's no longer "Pro UI by MRiscoC" if code has diverged so far that they can't be reconciled

you're thinking it the other way around. there's "MRiscoC's ProUI" and there's "Marlin's ProUI". if he wants to include or add anything to Marlin's, he can do so. otherwise he has his own fork doing his own thing and that's fine.

as he has already said whatever changes happen here will be considered there (his fork). for instance, if you compare forks, there are major changes to the menu code (menu.h) where menu.cpp simply 'doesn't exist'/inaccessible. same with the trammingwizard, some features have been moved to the PROUI_EX library. Marlin source code doesn't have this library.

so there's no reason not to make pull requests.

edit: If I come across anything that might be a helpful change, I'll make it so - thinking of the end user this side of the fork.

if the layout differs, then that's fine. it doesn't matter much anyway because I don't see the major changes from his fork making it way here any time soon, especially when that library is required.

you're thinking it the other way around. there's "MRiscoC's ProUI" and there's "Marlin's ProUI"

No, we literally call it out as "Pro UI by MRiscoC" since he created that version:

https://github.com/MarlinFirmware/Marlin/blob/1bb4a042e26af49602816ef33fcd2f3f4f728329/Marlin/Configuration.h#L3405

you're thinking it the other way around. there's "MRiscoC's ProUI" and there's "Marlin's ProUI"

No, we literally call it out as "Pro UI by MRiscoC" since he created that version:

https://github.com/MarlinFirmware/Marlin/blob/1bb4a042e26af49602816ef33fcd2f3f4f728329/Marlin/Configuration.h#L3405

so whats your point? Marlin by thinkyhead, since he created it. JyersUI by Jyers, since he created it. ok now what? no one but the creator is allow to approve changes?

so whats your point? Marlin by thinkyhead, since he created it. JyersUI by Jyers, since he created it. ok now what? no one but the creator is allow to approve changes?

The ecosystem is complex and you have "maintainers" and "contributors" more than individual creators, most of the time.

The challenge is that nobody is an expert with everything. It is hard to approve changes, unless they are very clearly correct or we have experts on hand who are able to review them and approve.

This is why I have been stressing the creation of very small, targeted, and well explained pull requests. Your job as a contributor is to convince me or another maintainer that your change is worth included in the codebase. If I can't readily understand the change, it will just sit or be rejected. I don't have the time to spelunk and explore every line of code that was touched. I also don't have the risk tolerance to just blindly merge things without understanding them.

As for the specific of all these different UIs...its even worse because I don't use any of them nor have I worked in their code. I'm largely just avoiding them unless they are extremely clear and simple, because they are the only ones I can readily evaluate.

@classicrocker883 can you explain why this option is useful? Is there something unique to ProUI that makes it more likely to need an encoder reversal that other types of screens with encoders?

@classicrocker883 can you explain why this option is useful? Is there something unique to ProUI that makes it more likely to need an encoder reversal that other types of screens with encoders?

some might like having the preference. maybe they will like the encoder go one way or go another. and well sometimes users get a different LCD and the encoder knob is reversed. for instance, Voxelab Aquila may be identical to Ender-3 V2, except for the LCD encoder direction. say they need to replace their LCD and they can substitute with a Creality, except in the firmware is reversed. so instead of recompiling / flashing firmware, there is the UI option to choose to change the direction.

having this code only allocates ~150 bytes

Runtime encoder direction change & other features like this shouldn’t be done for just one UI. They should be universal / at a higher level so all supported hardware and UIs can use the feature.

Due to the need to save stuff to EEPROM and affect portions of the code unrelated to ProUI, I refactored the new option as a more general conditional that currently only applies to ProUI. It should be pretty straightforward to implement for other displays with encoders based on this example, if there is some interest in doing so. I have no personal need to "occasionally reverse the encoder" but apparently Andrew likes a good practical joke.

Note that reversing the encoder will also affect the direction of editing, so clockwise will decrease values and counter-clockwise will increase. To mitigate this problem, what you really want is an option to reverse the menu direction, not the entire encoder.

I went ahead and modified this to only affect the direction of menu selection, keeping the direction of editing the same. Users should configure their encoders so clockwise increases an edited value (even if an encoder is installed to the left of the screen), and so clockwise moves down in the menus when reverse_encoder is false.

I have no personal need to "occasionally reverse the encoder" but apparently Andrew likes a good practical joke.

lol well youre not wrong @thinkyhead. a bit of context-backstory... so one of the users of my firmware mentioned his encoder knob started going out and Voxelab Aquila LCD's use encoder backwards from Ender-3V2 - despite being a near identical clone otherwise. and apparently if he were to replace the encoder (solder job) it would read reversed. at least that is what he had gathered from such forum posts like reddit.

I said I might be able to just make him some custom firmware with that one change if need to. but then I wanted to see if I could at least have it change on the fly, just for fun.

and well, here we are with the PR

I have also made code that you can change the encoder rate as well. I guess this can be just as helpful.

let me know if you think I should add it, here is a snippet:

encoder.cpp

      #if ENABLED(ENC_MENU_ITEM)
        uint16_t a = ui.enc_rateA,
                 b = ui.enc_rateB;
      #endif
...

        if (ENCODER_100X_STEPS_PER_SEC > 0 && encoderStepRate >= ENCODER_100X_STEPS_PER_SEC)
          encoder_multiplier = TERN(ENC_MENU_ITEM, a, 135);
        else if (ENCODER_10X_STEPS_PER_SEC > 0 && encoderStepRate >= ENCODER_10X_STEPS_PER_SEC)
          encoder_multiplier = TERN(ENC_MENU_ITEM, b, 25);
        else if (ENCODER_5X_STEPS_PER_SEC > 0 && encoderStepRate >= ENCODER_5X_STEPS_PER_SEC)
          encoder_multiplier = 5;


proui/dwin.cpp

#if ENABLED(ENC_MENU_ITEM)
  void setEncRateA() { setPIntOnClick(ui.enc_rateB + 1, 1000); }
  void setEncRateB() { setPIntOnClick(11, ui.enc_rateA - 1); }
#endif

    #if ALL(ENCODER_RATE_MULTIPLIER, ENC_MENU_ITEM)
      EDIT_ITEM_F(ICON_Motion, "Enc steps/sec 100x", onDrawPIntMenu, setEncRateA, &ui.enc_rateA);
      EDIT_ITEM_F(ICON_Motion, "Enc steps/sec 10x", onDrawPIntMenu, setEncRateB, &ui.enc_rateB);
    #endif

It's actually a part of the Marlin paradigm that when you change hardware, you generally need to recompile. Obviously we cannot make every type of display "hot swappable" so we don't typically go down these roads. If there is a general need under some circumstances to be able to reverse the encoder direction at the hardware level (i.e., swapping the EN1/EN2 pins) it may be better to just make it a "hidden" feature through an existing or new G-code, rather than expose it in a menu item.

In any case, at this time, anyone who wants to use that Voxelab display is going to need to build and install a new Marlin anyway, and at that point the menu item may be somewhat moot, since they'll want to recompile with the correct default for their new hardware (and we want to encourage compiling the most sensible defaults).

it may be better to just make it a "hidden" feature through an existing or new G-code, rather than expose it in a menu item.

that actually sounds better

anyone who wants to use that Voxelab display is going to need to build and install a new Marlin anyway

this may be true. I get having such a feature may never ever be used except in such few cases, and even so how often would that be used? I'm trying to picture myself being in that situation... unable to compile my own firmware, wishing the encoder knob went the other way. and for whatever reason some people just cant help themselves out of a paper bag.

I like the idea in proui/dwin.cpp constexpr int advItems = 3 + COUNT_ENABLED like in JyersUI, what I don't really get is... so MenuItemTotal in menuItemsPrepare() uses basically the totalitems = advItems - but previously without COUNT_ENABLED we could set it to an arbitrary number like 24, we could set it to 30 it wouldn't matter, just as long as it is at least the number of enabled menu items. what gives? wouldn't having all that COUNT_ENABLED code use up some unnecessary bytes?

I dont know if you have checked out MRiscoC's ProUI recently

but basically he was able to completely omit that out

you think we could somehow follow suit?

here's a bit of that for example of how that looks:

void drawAdvancedSettingsMenu() {
  if (notCurrentMenu(advancedSettings)) {
    BACK_ITEM(gotoMainMenu);
    #if ENABLED(EEPROM_SETTINGS)
      MENU_ITEM(ICON_WriteEEPROM, MSG_STORE_EEPROM, onDrawMenuItem, writeEeprom);
    #endif
...
    #if ENABLED(PRINTCOUNTER)
      MENU_ITEM(ICON_PrintStats, MSG_INFO_STATS_MENU, onDrawSubMenu, gotoPrintStats);
      MENU_ITEM(ICON_PrintStatsReset, MSG_INFO_PRINT_COUNT_RESET, onDrawSubMenu, printStatsReset);
    #endif
    #if HAS_LOCKSCREEN
      MENU_ITEM(ICON_Lock, MSG_LOCKSCREEN, onDrawMenuItem, dwinLockScreen);
    #endif
...
  }
  SET_MENU(advancedSettings, MSG_ADVANCED_SETTINGS);
  ui.reset_status(true);
}

menus.h

bool setMenu(Menu* &menu, FSTR_P fTitle);
inline bool setMenu(Menu* &menu) { return setMenu(menu, nullptr); };