Skip to content

Compiler warnings checks and fixes #316

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
merged 1 commit into from
Apr 7, 2021
Merged

Compiler warnings checks and fixes #316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 3 additions & 2 deletions src/rp2_common/hardware_dma/dma.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,10 @@ void print_dma_ctrl(dma_channel_hw_t *channel) {
ctrl & DMA_CH0_CTRL_TRIG_HIGH_PRIORITY_BITS ? 1 : 0,
ctrl & DMA_CH0_CTRL_TRIG_EN_BITS ? 1 : 0);
}
#endif

void check_dma_channel_param_impl(uint channel) {
#if PARAM_ASSERTIONS_ENABLED(DMA)
void check_dma_channel_param_impl(uint __unused channel) {
valid_params_if(DMA, channel < NUM_DMA_CHANNELS);
}

#endif
2 changes: 1 addition & 1 deletion src/rp2_common/hardware_gpio/gpio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ int gpio_get_pad(uint gpio) {
// This also clears the input/output/irq override bits.
void gpio_set_function(uint gpio, enum gpio_function fn) {
invalid_params_if(GPIO, gpio >= NUM_BANK0_GPIOS);
invalid_params_if(GPIO, fn << IO_BANK0_GPIO0_CTRL_FUNCSEL_LSB & ~IO_BANK0_GPIO0_CTRL_FUNCSEL_BITS);
invalid_params_if(GPIO, ((uint32_t)fn << IO_BANK0_GPIO0_CTRL_FUNCSEL_LSB) & ~IO_BANK0_GPIO0_CTRL_FUNCSEL_BITS);
// Set input enable on, output disable off
hw_write_masked(&padsbank0_hw->io[gpio],
PADS_BANK0_GPIO0_IE_BITS,
Expand Down
4 changes: 2 additions & 2 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 @@ -320,7 +320,7 @@ static inline void sm_config_set_out_shift(pio_sm_config *c, bool shift_right, b
* \param join Specifies the join type. \see enum pio_fifo_join
*/
static inline void sm_config_set_fifo_join(pio_sm_config *c, enum pio_fifo_join join) {
valid_params_if(PIO, join >= PIO_FIFO_JOIN_NONE && join <= PIO_FIFO_JOIN_RX);
valid_params_if(PIO, join == PIO_FIFO_JOIN_NONE || join == PIO_FIFO_JOIN_TX || join == PIO_FIFO_JOIN_RX);
c->shiftctrl = (c->shiftctrl & (uint)~(PIO_SM0_SHIFTCTRL_FJOIN_TX_BITS | PIO_SM0_SHIFTCTRL_FJOIN_RX_BITS)) |
(((uint)join) << PIO_SM0_SHIFTCTRL_FJOIN_TX_LSB);
}
Expand Down Expand Up @@ -350,7 +350,7 @@ static inline void sm_config_set_out_special(pio_sm_config *c, bool sticky, bool
* \param status_n parameter for the mov status operation (currently a bit count)
*/
static inline void sm_config_set_mov_status(pio_sm_config *c, enum pio_mov_status_type status_sel, uint status_n) {
valid_params_if(PIO, status_sel >= STATUS_TX_LESSTHAN && status_sel <= STATUS_RX_LESSTHAN);
valid_params_if(PIO, status_sel == STATUS_TX_LESSTHAN || status_sel == STATUS_RX_LESSTHAN);
c->execctrl = (c->execctrl
& ~(PIO_SM0_EXECCTRL_STATUS_SEL_BITS | PIO_SM0_EXECCTRL_STATUS_N_BITS))
| ((((uint)status_sel) << PIO_SM0_EXECCTRL_STATUS_SEL_LSB) & PIO_SM0_EXECCTRL_STATUS_SEL_BITS)
Expand Down
18 changes: 12 additions & 6 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 @@ -47,10 +47,10 @@ extern "C" {
*/
enum pwm_clkdiv_mode
{
PWM_DIV_FREE_RUNNING, ///< Free-running counting at rate dictated by fractional divider
PWM_DIV_B_HIGH, ///< Fractional divider is gated by the PWM B pin
PWM_DIV_B_RISING, ///< Fractional divider advances with each rising edge of the PWM B pin
PWM_DIV_B_FALLING ///< Fractional divider advances with each falling edge of the PWM B pin
PWM_DIV_FREE_RUNNING = 0, ///< Free-running counting at rate dictated by fractional divider
PWM_DIV_B_HIGH = 1, ///< Fractional divider is gated by the PWM B pin
PWM_DIV_B_RISING = 2, ///< Fractional divider advances with each rising edge of the PWM B pin
PWM_DIV_B_FALLING = 3 ///< Fractional divider advances with each falling edge of the PWM B pin
};

enum pwm_chan
Expand Down Expand Up @@ -144,7 +144,10 @@ static inline void pwm_config_set_clkdiv_int(pwm_config *c, uint div) {
* high level, rising edge or falling edge of the B pin input.
*/
static inline void pwm_config_set_clkdiv_mode(pwm_config *c, enum pwm_clkdiv_mode mode) {
valid_params_if(PWM, mode >= PWM_DIV_FREE_RUNNING && mode <= PWM_DIV_B_FALLING);
valid_params_if(PWM, mode == PWM_DIV_FREE_RUNNING ||
mode == PWM_DIV_B_RISING ||
mode == PWM_DIV_B_HIGH ||
mode == PWM_DIV_B_FALLING);
Comment on lines +147 to +150
Copy link
Contributor

Choose a reason for hiding this comment

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

a) it probably makes sense to list these in the same order they're listed in the enum? (i.e. swap round PWM_DIV_B_RISING and PWM_DIV_B_HIGH)

b) as this is a multi-line chunk of code repeated in both pwm_config_set_clkdiv_mode and pwm_set_clkdiv_mode it's probably worth factoring it out to an(other) inline function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a) don't care enough to change it.
b) thought about it at the time, but decided against because it is non functional code... and i was just working around a compiler limitation

c->csr = (c->csr & ~PWM_CH0_CSR_DIVMODE_BITS)
| (((uint)mode) << PWM_CH0_CSR_DIVMODE_LSB);
}
Expand Down Expand Up @@ -414,7 +417,10 @@ static inline void pwm_set_output_polarity(uint slice_num, bool a, bool b) {
*/
static inline void pwm_set_clkdiv_mode(uint slice_num, enum pwm_clkdiv_mode mode) {
check_slice_num_param(slice_num);
valid_params_if(PWM, mode >= PWM_DIV_FREE_RUNNING && mode <= PWM_DIV_B_FALLING);
valid_params_if(PWM, mode == PWM_DIV_FREE_RUNNING ||
mode == PWM_DIV_B_RISING ||
mode == PWM_DIV_B_HIGH ||
mode == PWM_DIV_B_FALLING);
hw_write_masked(&pwm_hw->slice[slice_num].csr, ((uint)mode) << PWM_CH0_CSR_DIVMODE_LSB, PWM_CH0_CSR_DIVMODE_BITS);
}

Expand Down
3 changes: 2 additions & 1 deletion test/kitchen_sink/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ target_compile_options(kitchen_sink_options INTERFACE
-Wfloat-equal
-Wmissing-format-attribute
-Wconversion
-Wsign-compare
$<$<COMPILE_LANGUAGE:C>:-Wstrict-prototypes>

-Wno-inline
Expand All @@ -85,7 +86,7 @@ target_compile_options(kitchen_sink_options INTERFACE
)

target_compile_definitions(kitchen_sink_libs INTERFACE
NDEBUG
PARAM_ASSERTIONS_ENABLE_ALL=1 # want to check all the assertions for compilation warnings
Copy link
Contributor

@lurch lurch Apr 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Haha, I'd never noticed that this file is manually specifying NDEBUG! That explains why I couldn't get the compiler to throw errors at me when I deliberately put syntax-errors inside valid_params_if / invalid_params_if macros 🤣 (even when I was building in Debug mode and had enabled PARAM_ASSERTIONS_ENABLE_ALL)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup; i have no idea why that was like that :-)

PICO_AUDIO_DMA_IRQ=1
)

Expand Down
40 changes: 33 additions & 7 deletions test/kitchen_sink/kitchen_sink.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,44 @@
*/

#include <stdio.h>
#include "pico/stdlib.h"
#include "pico/time.h"
// Include all headers to check for compiler warnings
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth having a build-time script do this automatically, as it's likely that additional headers will get added to the SDK in future?

Copy link
Contributor

Choose a reason for hiding this comment

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

Partially added in #319 🙂

#include "hardware/adc.h"
#include "hardware/claim.h"
#include "hardware/clocks.h"
#include "hardware/divider.h"
#include "hardware/dma.h"
#include "pico/bit_ops.h"
#include "hardware/flash.h"
#include "hardware/gpio.h"
#include "hardware/i2c.h"
#include "hardware/pwm.h"
#include "hardware/pio.h"
#include "hardware/interp.h"
#include "hardware/irq.h"
#include "hardware/pio.h"
#include "hardware/pll.h"
#include "hardware/pwm.h"
#include "hardware/resets.h"
#include "hardware/rtc.h"
#include "hardware/spi.h"
#include "hardware/sync.h"
#include "hardware/timer.h"
#include "pico/divider.h"
#include "pico/critical_section.h"
#include "hardware/uart.h"
#include "hardware/vreg.h"
#include "hardware/watchdog.h"
#include "hardware/xosc.h"
#include "pico/binary_info.h"
#include "pico/bit_ops.h"
#include "pico/bootrom.h"
#include "pico/divider.h"
#include "pico/double.h"
#include "pico/fix/rp2040_usb_device_enumeration.h"
#include "pico/float.h"
#include "pico/int64_ops.h"
#include "pico/malloc.h"
#include "pico/multicore.h"
#include "pico/printf.h"
#include "pico/stdlib.h"
#include "pico/sync.h"
#include "pico/time.h"
#include "pico/unique_id.h"

bi_decl(bi_block_device(
BINARY_INFO_MAKE_TAG('K', 'S'),
Expand Down