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

[BUG] Setting fan speed by LCD requires several clicks of the knob to move the setting by 1% #14975

Closed
LMF5000 opened this issue Aug 17, 2019 · 7 comments

Comments

@LMF5000
Copy link

LMF5000 commented Aug 17, 2019

Description

Running SKR E3 on CR-10 mini stock LCD. Encoder (knob) works fine in all menu items, one click moves one menu item, and for example when setting temperature one click changes the value by 1°C as expected. However when setting fan speed under Temperature -> Fan, it takes four encoder clicks to change speed by 1%.

Steps to Reproduce

  1. Open Temperature -> Fan speed from LCD
  2. Try increasing fan speed by 1%

Expected behavior:
Rotating the knob one "click" increases fan speed by 1%

Actual behavior:
The knob must be turned several to increment fan speed by 1%. Sometimes it takes 2 clicks, sometimes 3, sometimes up to 4 clicks.

Additional Information

The other menu items (like nozzle temperature and bed temperature) are working as expected (one knob click per value) so this doesn't seem to be a global setting, whatever it is just affects the fan setting (and possibly other things I haven't stumbled upon yet)

Configuration.zip

.

  • Include a ZIP file containing your Configuration.h and Configuration_adv.h files.
  • Provide pictures or links to videos that clearly demonstrate the issue.
  • See How Can I Contribute for additional guidelines.
@LMF5000 LMF5000 changed the title Setting fan speed by LCD requires 4 knob clicks per percent speed Setting fan speed by LCD requires several clicks of the knob to move the setting by 1% Aug 17, 2019
@AnHardt
Copy link
Member

AnHardt commented Aug 17, 2019

Formerly the fan value was in 0-127 or 0-255 - i don't really remember.
Someone improved that to show % recently.
Probably untested this was merged.
I guess the increasing/decreasing is still in the old scale. Only the display is in %.

@boelle boelle changed the title Setting fan speed by LCD requires several clicks of the knob to move the setting by 1% [BUG] Setting fan speed by LCD requires several clicks of the knob to move the setting by 1% Aug 17, 2019
@Ludy87
Copy link
Contributor

Ludy87 commented Aug 17, 2019

Change from 0-255 to 0% -100% was in March
#13519

I do not suppose that it is because of this change, my guess would be to manipulate the ENCODER settings of the last days

@AnHardt
Copy link
Member

AnHardt commented Aug 17, 2019

I'd look exactly here.

-      MENU_MULTIPLIER_ITEM_EDIT_CALLBACK(uint8, MSG_FAN_SPEED FAN_SPEED_1_SUFFIX, &thermalManager.lcd_tmpfan_speed[0], 0, 255, thermalManager.lcd_setFanSpeed0);
+      MENU_MULTIPLIER_ITEM_EDIT_CALLBACK(percent, MSG_FAN_SPEED FAN_SPEED_1_SUFFIX, &thermalManager.lcd_tmpfan_speed[0], 0, 255, thermalManager.lcd_setFanSpeed0);

It's exactly what i deducted from the described behavior.

@marcio-ao
Copy link
Contributor

Someone improved that to show % recently.
Probably untested this was merged.

This was me. For an end-user which isn't a programmer, a percentage makes a lot more sense than 255.

I understand that now one encoder click does not increment the number by one, which is true; however I would still consider the new behavior to be an improvement over the old. If there is disagreement on the merits of the trade-off, perhaps this could be made into a configurable option.

As for actually fixing it so it is both a percentage and the encoder wheel increments by one, I suspect this would be a very difficult thing to pull off. Either:

  1. Marlin needs to be modified so fan speed are stored internally as a value from 0 to 100.
  2. Or the LCD encoder multiplier needs to be changed for that particular menu item only.

Option one will probably require changes many places in Marlin, and will cause problems with M106. Option two would require the ultralcd menu code to be modified for this specific case.

Personally, I think both of these options add complexity to the code that probably ought not to be there. The current behavior is an acceptable tradeoff, in my opinion.

marcio-ao added a commit to marcio-ao/Marlin that referenced this issue Aug 20, 2019
- Now one click corresponds to an increase of 1%
@marcio-ao
Copy link
Contributor

Personally, I think both of these options add complexity to the code that probably ought not to be there. The current behavior is an acceptable tradeoff, in my opinion.

Okay. I'll eat my words. Turns out the fix was trivial once I looked at the code more closely. There already is a facility for scaling based on particular data types.

@LMF5000
Copy link
Author

LMF5000 commented Aug 21, 2019

Awesome! Thanks all for getting it fixed :)

@github-actions
Copy link

github-actions bot commented Jul 4, 2020

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants