Skip to content

esp32: Bound PWM duty value. #6809

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

Closed
wants to merge 2 commits into from
Closed

esp32: Bound PWM duty value. #6809

wants to merge 2 commits into from

Conversation

jej
Copy link

@jej jej commented Jan 28, 2021

I had a potential out-of-boundary bug on an esp8266 device when setting the PWM duty value to control fan speed (negative value or >1023). On esp8266 the machine.PWM bounds the duty value:

if (duty < 1) {
pwm.duty[channel] = 0;
} else if (duty >= PWM_DEPTH) {
pwm.duty[channel] = PWM_DEPTH;
} else {
pwm.duty[channel] = duty;
}

...so I didn't notice it. The PWM behaviour was normal, just bounded to 0 and 1023.

When I switched my code to an esp32, the fan behaviour was really strange, because the PWM duty value is only binary ANDed:

duty &= ((1 << PWRES) - 1);

Negative values may be higher and bigger values lower. Combined with issue #5737 I thought first about a hardware problem...

This PR solves the problem and makes the esp32 PWM to have the same behaviour as esp8266/stm32 (AFAIK), by bounding the duty value between 0 and max PWM resolution. My previous buggy code works again :)

@jej jej changed the title esp32: Bound PWM duty value esp32: Bound PWM duty value. Feb 21, 2021
@EddieParis
Copy link
Contributor

Was about to send the same patch ! Commit message has been updated, and is now correct. For me it can be merged.

@jej
Copy link
Author

jej commented Feb 25, 2021

Yes, it's an API inconsistency among ports... I deleted my git fork but I can repull if needed.

@EddieParis
Copy link
Contributor

I don't know if it required to do the actual merge ? If Yes, I would be glad you repull, because I have another ongoing pull request for ESP8266.

@jej
Copy link
Author

jej commented Feb 26, 2021

Here it is: #6968
I close this.

@jej jej closed this Feb 26, 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.

3 participants