-
Notifications
You must be signed in to change notification settings - Fork 919
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
Change div_int_frac methods to be suffixed by the number of bits of fraction e.g. div_int_frac8 #1926
base: develop
Are you sure you want to change the base?
Conversation
…act; e.g. div_int_frac8
…le future h/w change regresions
…uint32_t for future proofing
@@ -354,9 +354,26 @@ void clocks_enable_resus(resus_callback_t resus_callback); | |||
* \param gpio The GPIO pin to output the clock to. Valid GPIOs are: 21, 23, 24, 25. These GPIOs are connected to the GPOUT0-3 clock generators. | |||
* \param src The source clock. See the register field CLOCKS_CLK_GPOUT0_CTRL_AUXSRC for a full list. The list is the same for each GPOUT clock generator. | |||
* \param div_int The integer part of the value to divide the source clock by. This is useful to not overwhelm the GPIO pin with a fast clock. this is in range of 1..2^24-1. | |||
* \param div_frac The fractional part of the value to divide the source clock by. This is in range of 0..255 (/256). | |||
* \param div_frac16 The fractional part of the value to divide the source clock by. This is in range of 0..65536 (/65536). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably the range is 0..65535
?
sm_config_set_clkdiv_int_frac8(c, div_int, div_frac8); | ||
} | ||
|
||
static inline void pio_calculate_clkdiv8_from_float(float div, uint32_t *div_int, uint8_t *div_frac8) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the function named sm_config_set_clkdiv_int_frac8
it's "obvious" that it's the fractional-part that is 8-bit, but for this function named pio_calculate_clkdiv8_from_float
IMHO it's not clear what the 8
is referring to? 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 8 does seem confusing. It's presumably referring to the frac8? This isn't presumably supposed to be part of the API even though it's in the header. So maybe it doesn't matter
} | ||
|
||
/** \brief Set PWM clock divider in a PWM configuration | ||
* \ingroup hardware_pwm | ||
* | ||
* \param c PWM configuration struct to modify | ||
* \param div Integer value to reduce counting rate by. Must be greater than or equal to 1. | ||
* \param div_int Integer value to reduce counting rate by. Must be greater than or equal to 1 annd less than 256. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
annd -> and
// PICO_CONFIG: CYW43_PIO_CLOCK_DIV_FRAC, Fractional part of the clock divider for communication with the wireless chip, type=bool, default=0, group=pico_cyw43_driver | ||
#ifndef CYW43_PIO_CLOCK_DIV_FRAC | ||
#define CYW43_PIO_CLOCK_DIV_FRAC 0 | ||
// PICO_CONFIG: CYW43_PIO_CLOCK_DIV_FRAC8, Fractional part of the clock divider for communication with the wireless chip 0-255, type=bool, default=0, group=pico_cyw43_driver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you could add min=0, max=255
here?
#define CYW43_PIO_CLOCK_DIV_FRAC 0 | ||
// PICO_CONFIG: CYW43_PIO_CLOCK_DIV_FRAC8, Fractional part of the clock divider for communication with the wireless chip 0-255, type=bool, default=0, group=pico_cyw43_driver | ||
#ifndef CYW43_PIO_CLOCK_DIV_FRAC8 | ||
#ifdef CYW43_PIO_CLOCK_DIV_FRAC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might not hurt to add a // backwards compatibility
comment here?
static inline void pwm_config_set_clkdiv_int(pwm_config *c, uint div) { | ||
valid_params_if(HARDWARE_PWM, div >= 1 && div < 256); | ||
pwm_config_set_clkdiv_int_frac(c, (uint8_t)div, 0); | ||
static inline void pwm_config_set_clkdiv_int(pwm_config *c, uint32_t div_int) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems curious that PWM has an integer-divider-only function, but none of the other hardware APIs do? 🤔
@@ -245,10 +245,20 @@ void clock_gpio_init_int_frac(uint gpio, uint src, uint32_t div_int, uint8_t div | |||
invalid_params_if(HARDWARE_CLOCKS, true); | |||
} | |||
|
|||
#if !PICO_RP2040 // assert currently broken on RP2040, but we know that hardware has 16-bit integer part |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see anything in the RP2040 datasheet that implies that this isn't a full 24-bit integer divider? (The only bit that looks odd is that the 24-bit CLK_GPOUT0_DIV.INT
field says "0 → divide by 2^16")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I've started an internal email thread to try and get clarity on this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conclusion of this email thread (for any curious onlookers) was that clock-dividers are indeed 24-bit integer, 8-bit fractional (24:8) on RP2040, but 16-bit integer, 16-bit fractional (16:16) on RP2350.
The snippet of the RP2040 datasheet I quoted above is apparently a typo, and setting the .INT
part of the divider to 0 does a full 2^24 divide.
@@ -354,9 +354,26 @@ void clocks_enable_resus(resus_callback_t resus_callback); | |||
* \param gpio The GPIO pin to output the clock to. Valid GPIOs are: 21, 23, 24, 25. These GPIOs are connected to the GPOUT0-3 clock generators. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that there's a
#if !PICO_RP2040
else if (gpio == 13) gpclk = clk_gpout0;
else if (gpio == 15) gpclk = clk_gpout1;
#endif
in the code, so do we need one of those \if rp2350_specific
things here?
(this is slightly tangential to the focus of this PR, sorry)
fixes #1924