Skip to content

Make all float->int/frac conversions round to nearest by default #2065

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.

Sign up for GitHub

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

Merged
Parent: Remove unnecessary check in pio_encode_sideset_opt
merged 5 commits into from
Nov 21, 2024
Merged

Make all float->int/frac conversions round to nearest by default #2065

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4bf91aa
making pio_calculate_clkdiv8_from_float round to the neareset 1/256 (…
kilograham Nov 20, 2024
9ebaa64
chage rounding of all float clkdivs to round to neareset; default to …
kilograham Nov 20, 2024
3a693bc
fix copy/paste errors in PICO_CONFIG
kilograham Nov 20, 2024
c4c6256
Calculate size of FRAC field using its own MSB and LSB, rather than h…
lurch Nov 20, 2024
4b84880
Add a new REG_FIELD_WIDTH macro, and make the calculation of the cloc…
lurch Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/rp2040/pico_platform/include/pico/platform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,11 @@
#define PICO_RAM_VECTOR_TABLE_SIZE (VTABLE_FIRST_IRQ + NUM_IRQS)
#endif

// PICO_CONFIG: PICO_CLKDIV_ROUND_NEAREST, True if floating point clock divisors should be rounded to the nearest possible clock divisor by default rather than rounding down, type=bool, default=1, group-pico_platform
#ifndef PICO_CLKDIV_ROUND_NEAREST
#define PICO_CLKDIV_ROUND_NEAREST 1
#endif

#ifndef __ASSEMBLER__

/*! \brief No-op function for the body of tight loops
Expand Down
5 changes: 5 additions & 0 deletions src/rp2350/pico_platform/include/pico/platform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@
#define PICO_USE_STACK_GUARDS 0
#endif

// PICO_CONFIG: PICO_CLKDIV_ROUND_NEAREST, True if floating point clock divisors should be rounded to the nearest possible clock divisor by default rather than rounding down, type=bool, default=1, group-pico_platform
#ifndef PICO_CLKDIV_ROUND_NEAREST
#define PICO_CLKDIV_ROUND_NEAREST 1
#endif

#ifndef __ASSEMBLER__

/*! \brief No-op function for the body of tight loops
Expand Down
8 changes: 8 additions & 0 deletions src/rp2_common/hardware_adc/include/hardware/adc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,11 @@
#define ADC_TEMPERATURE_CHANNEL_NUM (NUM_ADC_CHANNELS - 1)
#endif

// PICO_CONFIG: PICO_ADC_CLKDIV_ROUND_NEAREST, True if floating point ADC clock divisors should be rounded to the nearest possible clock divisor rather than rounding down, type=bool, default=PICO_CLKDIV_ROUND_NEAREST, group-hardware_pio
#ifndef PICO_ADC_CLKDIV_ROUND_NEAREST
#define PICO_ADC_CLKDIV_ROUND_NEAREST PICO_CLKDIV_ROUND_NEAREST
#endif

#ifdef __cplusplus
extern "C" {
#endif
Expand Down Expand Up @@ -198,6 +203,9 @@ static inline void adc_run(bool run) {
*/
static inline void adc_set_clkdiv(float clkdiv) {
invalid_params_if(HARDWARE_ADC, clkdiv >= 1 << (ADC_DIV_INT_MSB - ADC_DIV_INT_LSB + 1));
#if PICO_ADC_CLKDIV_ROUND_NEAREST
clkdiv += 0.5f / (1 << ADC_DIV_INT_LSB); // round to the nearest fraction
#endif
adc_hw->div = (uint32_t)(clkdiv * (float) (1 << ADC_DIV_INT_LSB));
}

Expand Down
8 changes: 8 additions & 0 deletions src/rp2_common/hardware_clocks/include/hardware/clocks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,11 @@ extern "C" {
#else
#define PARAM_ASSERTIONS_ENABLED_HARDWARE_CLOCKS 0
#endif
#endif

// PICO_CONFIG: PICO_CLOCK_GPIO_CLKDIV_ROUND_NEAREST, True if floating point GPIO clock divisors should be rounded to the nearest possible clock divisor rather than rounding down, type=bool, default=PICO_CLKDIV_ROUND_NEAREST, group-hardware_pio
#ifndef PICO_CLOCK_GPIO_CLKDIV_ROUND_NEAREST
#define PICO_CLOCK_GPIO_CLKDIV_ROUND_NEAREST PICO_CLKDIV_ROUND_NEAREST
#endif

typedef clock_num_t clock_handle_t;
Expand Down Expand Up @@ -387,6 +392,9 @@ static inline void clock_gpio_init_int_frac(uint gpio, uint src, uint32_t div_in
static inline void clock_gpio_init(uint gpio, uint src, float div)
{
uint div_int = (uint)div;
#if PICO_CLOCK_GPIO_CLKDIV_ROUND_NEAREST
div += 0.5f / (1 << (CLOCKS_CLK_GPOUT0_DIV_FRAC_MSB + 1 - CLOCKS_CLK_GPOUT0_DIV_FRAC_LSB)); // round to the nearest fraction
#endif
#if CLOCKS_CLK_GPOUT0_DIV_FRAC_MSB - CLOCKS_CLK_GPOUT0_DIV_FRAC_LSB == 15
uint16_t frac = (uint16_t)((div - (float)div_int) * (1u << 16));
clock_gpio_init_int_frac16(gpio, src, div_int, frac);
Expand Down
9 changes: 9 additions & 0 deletions src/rp2_common/hardware_pio/include/hardware/pio.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@
#define PICO_PIO_VERSION 0
#endif
#endif

// PICO_CONFIG: PICO_PIO_CLKDIV_ROUND_NEAREST, True if floating point PIO clock divisors should be rounded to the nearest possible clock divisor rather than rounding down, type=bool, default=PICO_CLKDIV_ROUND_NEAREST, group-hardware_pio
#ifndef PICO_PIO_CLKDIV_ROUND_NEAREST
#define PICO_PIO_CLKDIV_ROUND_NEAREST PICO_CLKDIV_ROUND_NEAREST
#endif

/** \file hardware/pio.h
* \defgroup hardware_pio hardware_pio
*
Expand Down Expand Up @@ -488,6 +494,9 @@ static inline void sm_config_set_clkdiv_int_frac(pio_sm_config *c, uint16_t div_

static inline void pio_calculate_clkdiv8_from_float(float div, uint32_t *div_int, uint8_t *div_frac8) {
valid_params_if(HARDWARE_PIO, div >= 1 && div <= 65536);
#if PICO_PIO_CLKDIV_ROUND_NEAREST
div += 0.5f / 256; // round to the nearest 1/256
#endif
*div_int = (uint16_t)div;
// not a strictly necessary check, but if this changes, then this method should
// probably no longer be used in favor of one with a larger fraction
Expand Down
8 changes: 8 additions & 0 deletions src/rp2_common/hardware_pwm/include/hardware/pwm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ static_assert(DREQ_PWM_WRAP7 == DREQ_PWM_WRAP0 + 7, "");
})
#endif

// PICO_CONFIG: PICO_PWM_CLKDIV_ROUND_NEAREST, True if floating point PWM clock divisors should be rounded to the nearest possible clock divisor rather than rounding down, type=bool, default=PICO_CLKDIV_ROUND_NEAREST, group-hardware_pio
#ifndef PICO_PWM_CLKDIV_ROUND_NEAREST
#define PICO_PWM_CLKDIV_ROUND_NEAREST PICO_CLKDIV_ROUND_NEAREST
#endif

static inline void check_slice_num_param(__unused uint slice_num) {
valid_params_if(HARDWARE_PWM, slice_num < NUM_PWM_SLICES);
}
Expand Down Expand Up @@ -155,6 +160,9 @@ static inline void pwm_config_set_phase_correct(pwm_config *c, bool phase_correc
*/
static inline void pwm_config_set_clkdiv(pwm_config *c, float div) {
valid_params_if(HARDWARE_PWM, div >= 1.f && div < 256.f);
#if PICO_PWM_CLKDIV_ROUND_NEAREST
div += 0.5f / (1 << PWM_CH0_DIV_INT_LSB); // round to the nearest fraction
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry - why are we using PWM_CH0_DIV_INT_LSB here rather than PWM_CH0_DIV_FRAC_MSB+1 - PWM_CH0_DIV_FRAC_LSB. Isn't it coincidence that it's the right value? It confuses me because there are 8 bits for the pwm int.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's (almost) exactly what I asked earlier - see my resolved comments above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i used what was used in the code in the rest of the function... feel free to commit a new change to this PR to fix all of them to be more sensible

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, but see that the original code still uses the "wrong" values, we should fix all.

Perhaps worth adding a "const int frac_bit_count = (BLAH);" to each function

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wish I hadn't asked!

#endif
c->div = (uint32_t)(div * (float)(1u << PWM_CH0_DIV_INT_LSB));
}

Expand Down
Loading