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

Fix integer division causing issue #17968 #18094

Merged
merged 1 commit into from
May 25, 2020

Conversation

g3gg0
Copy link
Contributor

@g3gg0 g3gg0 commented May 24, 2020

Description

There is a bug in fast_pwm.cpp:277

_SET_OCRnQ(timer.OCRnQ, timer.q, v * float(top / v_size)); // Scale 8/16-bit v to top value

The intention was most likely (from what the comment says and what would make sense) to
scale the given duty cycle (based on 255 as maximum) to the capcom top value.
But the float cast happens after the integer division already happened, thus would be correct only if top==v_size or a multiple of it.
So this code assumes top is always a multiple of 255.

I was asked to make a PR for this instead of a bug report.

Details

When dividing two integers, the fraction will be dropped and the number rounded towards zero.
In this line you divide the maximums in integer domain, cast to float and then scale the value to the new maximum.

So the line should look like this:

_SET_OCRnQ(timer.OCRnQ, timer.q, v * top / v_size); // Scale 8/16-bit v to top value
or
_SET_OCRnQ(timer.OCRnQ, timer.q, v * float(top) / float(v_size)); // Scale 8/16-bit v to top value

The first might cause integer overflows during multiplication, so I'd go for the second variant which operates on floats immediately.

Benefits

The duty cycle values for AVR targets with FAST_PWM_FAN suddenly would make sense and go way up to 100%

Related Issues

#17968

@thinkyhead
Copy link
Member

@g3gg0 — Good catch, thanks!

@thinkyhead thinkyhead merged commit f776260 into MarlinFirmware:bugfix-2.0.x May 25, 2020
vgadreau pushed a commit to vgadreau/Marlin that referenced this pull request May 29, 2020
jmp0x0000 pushed a commit to jmp0x0000/Marlin that referenced this pull request Aug 7, 2020
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.

2 participants