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

Clock Divider integer should not be an int but uint8_t #2046

Closed
geprge opened this issue Nov 16, 2024 · 6 comments
Closed

Clock Divider integer should not be an int but uint8_t #2046

geprge opened this issue Nov 16, 2024 · 6 comments

Comments

@geprge
Copy link

geprge commented Nov 16, 2024

In the file src/rp2_common/hardware_pwm/include/hardware/pwm.h

Second arguement of the below function should be uint8_t

static inline void pwm_config_set_clkdiv_int(pwm_config *c, uint32_t div_int) {
pwm_config_set_clkdiv_int_frac4(c, div_int, 0);
}

static inline void pwm_config_set_clkdiv_int_frac4(pwm_config *c, uint32_t div_int, uint8_t div_frac4) {
static_assert(PWM_CH0_DIV_INT_MSB - PWM_CH0_DIV_INT_LSB == 7, "");
valid_params_if(HARDWARE_PWM, div_int >= 1 && div_int < 256);
static_assert(PWM_CH0_DIV_FRAC_MSB - PWM_CH0_DIV_FRAC_LSB == 3, "");
valid_params_if(HARDWARE_PWM, div_frac4 < 16);
c->div = (((uint)div_int) << PWM_CH0_DIV_INT_LSB) | (((uint)div_frac4) << PWM_CH0_DIV_FRAC_LSB);
}

@lurch
Copy link
Contributor

lurch commented Nov 16, 2024

See #1926 which is the big PR where all the clock-divider code got tweaked recently...

@geprge
Copy link
Author

geprge commented Nov 17, 2024

I don't see the change either in master or in develop branch. I see some updates for the other functions; but not for pwm_set_config_clkdiv_int().
I know it does not create any functional impact for an expert user; but it can mislead the user, who could try to provide a value greater than 255 ( and surprised seeing the clock gets divided by a different factor)...I was one of them.

@lurch
Copy link
Contributor

lurch commented Nov 17, 2024

If you ran your code in Debug mode, I believe it should have triggered the valid_params_if assertion?

@geprge
Copy link
Author

geprge commented Nov 17, 2024

Yes. Ofcourse.
Does it stop us from correcting an obvious misleading function prototype? or if I may ask.....What is the advantage of retaining this arguement as uint, when the value range is limited within uint8_t ?

@lurch
Copy link
Contributor

lurch commented Nov 18, 2024

Based on all the other changes that happened in #1926 I think the intention is that the "higher level function" pwm_config_set_clkdiv_int probably has a deliberately-broad function prototype, so that if any future chips support more than an 8-bit PWM integer divisor, the function prototype won't need to be changed. And then the "more specific function" (in this case pwm_config_set_clkdiv_int_frac4) actually checks the specific range of values (via debug asserts).

The Doxygen-comment for pwm_config_set_clkdiv_int https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/hardware_pwm/include/hardware/pwm.h#L185 does say "\param div_int Integer value to reduce counting rate by. Must be greater than or equal to 1 and less than 256." and we can obviously modify this Doxygen-comment in future (if needed) without "breaking" the function prototype.

@kilograham
Copy link
Contributor

yes; we clarified all the int_frac APIs to be int_fracN since RP2040/RP2350 differ in fractional bits in some cases, and so the frac part was confusing. the int part is not uint32_t as @lurch says to provide consistency with possible future h/w changes (and between APIs)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants