ronantakizawa/codereview-bench huggingface dataset:
Repository: raspberrypi/pico-sdk
File: src/boards/include/boards/pimoroni_motor2040.h (C/C++)

Code before:
#define MOTOR2040_ENCODER_C_B_PIN 13
#define MOTOR2040_ENCODER_D_A_PIN 14
#define MOTOR2040_ENCODER_D_B_PIN 15
#define MOTOR2040_NUM_ENCODERS 4

#if MOTOR2040_USE_DISTANCE_SENSOR
#define MOTOR2040_TRIG_PIN 16
#define MOTOR2040_ECHO_PIN 17
#else
#define MOTOR2040_UART 0
#define MOTOR2040_TX_PIN 16
#define MOTOR2040_RX_PIN 17
#endif

#define MOTOR2040_LED_DATA_PIN 18
#define MOTOR2040_NUM_LEDS 1

#define MOTOR2040_I2C 0
#define MOTOR2040_INT_PIN 19
#define MOTOR2040_SDA_PIN 20
#define MOTOR2040_SCL_PIN 21

#define MOTOR2040_USER_SW_PIN 23
#define MOTOR2040_ADC_ADDR_0_PIN 22
#define MOTOR2040_ADC_ADDR_1_PIN 24
#define MOTOR2040_ADC_ADDR_2_PIN 25

#define MOTOR2040_A0_PIN 26
#define MOTOR2040_A1_PIN 27
#define MOTOR2040_A2_PIN 28
#define MOTOR2040_NUM_ADC_PINS 3

#define MOTOR2040_SHARED_ADC_PIN 29

#define MOTOR2040_CURRENT_SENSE_A_ADDR 0b000
#define MOTOR2040_CURRENT_SENSE_B_ADDR 0b001
#define MOTOR2040_CURRENT_SENSE_C_ADDR 0b010
#define MOTOR2040_CURRENT_SENSE_D_ADDR 0b011
#define MOTOR2040_VOLTAGE_SENSE_ADDR 0b100
#define MOTOR2040_FAULT_SENSE_ADDR 0b101
#define MOTOR2040_SENSOR_1_ADDR 0b110
#define MOTOR2040_SENSOR_2_ADDR 0b111
#define MOTOR2040_NUM_SENSORS 2

// --- UART ---
#if MOTOR2040_USE_DISTANCE_SENSOR
// no PICO_DEFAULT_UART
#else
#ifndef PICO_DEFAULT_UART
#define PICO_DEFAULT_UART MOTOR2040_UART
#endif

Review comment (suggestion):
If I'm understanding things correctly, it _might_ make sense to move these to just below `MOTOR2040_SHARED_ADC_PIN` ? But that's just nit-picking :wink: 

Code after:
#define MOTOR2040_ENCODER_C_B_PIN 13
#define MOTOR2040_ENCODER_D_A_PIN 14
#define MOTOR2040_ENCODER_D_B_PIN 15
#define MOTOR2040_NUM_ENCODERS 4

#if MOTOR2040_USE_DISTANCE_SENSOR
#define MOTOR2040_TRIG_PIN 16
#define MOTOR2040_ECHO_PIN 17
#else
#define MOTOR2040_UART 0
#define MOTOR2040_TX_PIN 16
#define MOTOR2040_RX_PIN 17
#endif

#define MOTOR2040_LED_DATA_PIN 18
#define MOTOR2040_NUM_LEDS 1

#define MOTOR2040_I2C 0
#define MOTOR2040_INT_PIN 19
#define MOTOR2040_SDA_PIN 20
#define MOTOR2040_SCL_PIN 21

#define MOTOR2040_USER_SW_PIN 23

#define MOTOR2040_A0_PIN 26
#define MOTOR2040_A1_PIN 27
#define MOTOR2040_A2_PIN 28
#define MOTOR2040_NUM_ADC_PINS 3

#define MOTOR2040_SHARED_ADC_PIN 29
#define MOTOR2040_ADC_ADDR_0_PIN 22
#define MOTOR2040_ADC_ADDR_1_PIN 24
#define MOTOR2040_ADC_ADDR_2_PIN 25

#define MOTOR2040_CURRENT_SENSE_A_ADDR 0b000
#define MOTOR2040_CURRENT_SENSE_B_ADDR 0b001
#define MOTOR2040_CURRENT_SENSE_C_ADDR 0b010
#define MOTOR2040_CURRENT_SENSE_D_ADDR 0b011
#define MOTOR2040_VOLTAGE_SENSE_ADDR 0b100
#define MOTOR2040_FAULT_SENSE_ADDR 0b101
#define MOTOR2040_SENSOR_1_ADDR 0b110
#define MOTOR2040_SENSOR_2_ADDR 0b111
#define MOTOR2040_NUM_SENSORS 2

// --- UART ---
#if MOTOR2040_USE_DISTANCE_SENSOR
// no PICO_DEFAULT_UART
#else
#ifndef PICO_DEFAULT_UART
#define PICO_DEFAULT_UART MOTOR2040_UART
#endif
Repository: raspberrypi/pico-sdk
File: src/boards/include/boards/solderparty_rp2040_stamp_round_carrier.h (C/C++)

Code before:

#ifndef _BOARDS_SOLDERPARTY_RP2040_STAMP_ROUND_CARRIER_H
#define _BOARDS_SOLDERPARTY_RP2040_STAMP_ROUND_CARRIER_H

// For board detection
#define SOLDERPARTY_RP2040_STAMP_ROUND_CARRIER

// --- User LED ---
#ifndef PICO_DEFAULT_LED_PIN
#define PICO_DEFAULT_LED_PIN   25
#endif

// --- UART ---
#ifndef PICO_DEFAULT_UART_TX_PIN
#define PICO_DEFAULT_UART_TX_PIN 16
#endif
#ifndef PICO_DEFAULT_UART_RX_PIN
#define PICO_DEFAULT_UART_RX_PIN 17
#endif

// --- Neopixel ---
#ifndef PICO_DEFAULT_WS2812_PIN
#define PICO_DEFAULT_WS2812_PIN 24
#endif

// --- I2C ---
#ifndef PICO_DEFAULT_I2C_SDA_PIN
#define PICO_DEFAULT_I2C_SDA_PIN   0
#endif
#ifndef PICO_DEFAULT_I2C_SCL_PIN
#define PICO_DEFAULT_I2C_SCL_PIN   1
#endif

// --- SPI ---
#ifndef PICO_DEFAULT_SPI
#define PICO_DEFAULT_SPI 1
#endif
#ifndef PICO_DEFAULT_SPI_SCK_PIN
#define PICO_DEFAULT_SPI_SCK_PIN 10
#endif
#ifndef PICO_DEFAULT_SPI_TX_PIN
#define PICO_DEFAULT_SPI_TX_PIN 11
#endif
#ifndef PICO_DEFAULT_SPI_RX_PIN
#define PICO_DEFAULT_SPI_RX_PIN 8
#endif
#ifndef PICO_DEFAULT_SPI_CSN_PIN
#define PICO_DEFAULT_SPI_CSN_PIN 9
#endif

#include "solderparty_rp2040_stamp.h"

Review comment (suggestion):
Looking at the product-page you mentioned, I guess the on-board NeoPixel doesn't stop working when the Stamp is on the Carrier Board, so perhaps the  Carrier Board NeoPixel pin should get its own `#define`, rather than overriding the `PICO_DEFAULT_WS2812_PIN` from `solderparty_rp2040_stamp.h`? :shrug: 

Code after:

#ifndef _BOARDS_SOLDERPARTY_RP2040_STAMP_ROUND_CARRIER_H
#define _BOARDS_SOLDERPARTY_RP2040_STAMP_ROUND_CARRIER_H

// For board detection
#define SOLDERPARTY_RP2040_STAMP_ROUND_CARRIER

// --- User LED ---
#ifndef PICO_DEFAULT_LED_PIN
#define PICO_DEFAULT_LED_PIN   25
#endif

// --- UART ---
#ifndef PICO_DEFAULT_UART
#define PICO_DEFAULT_UART 0
#endif
#ifndef PICO_DEFAULT_UART_TX_PIN
#define PICO_DEFAULT_UART_TX_PIN 16
#endif
#ifndef PICO_DEFAULT_UART_RX_PIN
#define PICO_DEFAULT_UART_RX_PIN 17
#endif

// --- Neopixel Ring ---
#ifndef PICO_DEFAULT_WS2812_PIN
#define PICO_DEFAULT_WS2812_PIN 24
#endif

// --- On-Stamp Neopixel ---
#ifndef SOLDERPARTY_RP2040_STAMP_WS2812_PIN
#define SOLDERPARTY_RP2040_STAMP_WS2812_PIN 21
#endif

// --- I2C ---
#ifndef PICO_DEFAULT_I2C
#define PICO_DEFAULT_I2C       0
#endif
#ifndef PICO_DEFAULT_I2C_SDA_PIN
#define PICO_DEFAULT_I2C_SDA_PIN   0
#endif
#ifndef PICO_DEFAULT_I2C_SCL_PIN
#define PICO_DEFAULT_I2C_SCL_PIN   1
#endif

// --- SPI ---
#ifndef PICO_DEFAULT_SPI
#define PICO_DEFAULT_SPI 1
#endif
#ifndef PICO_DEFAULT_SPI_SCK_PIN
#define PICO_DEFAULT_SPI_SCK_PIN 10
#endif
Repository: raspberrypi/pico-sdk
File: src/boards/include/boards/solderparty_rp2040_stamp_round_carrier.h (C/C++)

Code before:
#define SOLDERPARTY_RP2040_STAMP_ROUND_CARRIER

// --- User LED ---
#ifndef PICO_DEFAULT_LED_PIN
#define PICO_DEFAULT_LED_PIN   25
#endif

// --- UART ---
#ifndef PICO_DEFAULT_UART
#define PICO_DEFAULT_UART 0
#endif
#ifndef PICO_DEFAULT_UART_TX_PIN
#define PICO_DEFAULT_UART_TX_PIN 16
#endif
#ifndef PICO_DEFAULT_UART_RX_PIN
#define PICO_DEFAULT_UART_RX_PIN 17
#endif

// --- Neopixel Ring ---
#ifndef PICO_DEFAULT_WS2812_PIN
#define PICO_DEFAULT_WS2812_PIN 24
#endif

// --- On-Stamp Neopixel ---
#ifndef PICO_STAMP_WS2812_PIN
#define PICO_STAMP_WS2812_PIN 21
#endif

// --- I2C ---
#ifndef PICO_DEFAULT_I2C
#define PICO_DEFAULT_I2C       0
#endif
#ifndef PICO_DEFAULT_I2C_SDA_PIN
#define PICO_DEFAULT_I2C_SDA_PIN   0
#endif
#ifndef PICO_DEFAULT_I2C_SCL_PIN
#define PICO_DEFAULT_I2C_SCL_PIN   1
#endif

// --- SPI ---
#ifndef PICO_DEFAULT_SPI
#define PICO_DEFAULT_SPI 1
#endif
#ifndef PICO_DEFAULT_SPI_SCK_PIN
#define PICO_DEFAULT_SPI_SCK_PIN 10
#endif
#ifndef PICO_DEFAULT_SPI_TX_PIN
#define PICO_DEFAULT_SPI_TX_PIN 11
#endif
#ifndef PICO_DEFAULT_SPI_RX_PIN
#define PICO_DEFAULT_SPI_RX_PIN 8

Review comment (suggestion):
This should probably be something like `SOLDERPARTY_RP2040_STAMP_...` rather than `PICO_STAMP_...`, as it's a feature specific to this board and not to any other Pico boards.

Code after:
#define SOLDERPARTY_RP2040_STAMP_ROUND_CARRIER

// --- User LED ---
#ifndef PICO_DEFAULT_LED_PIN
#define PICO_DEFAULT_LED_PIN   25
#endif

// --- UART ---
#ifndef PICO_DEFAULT_UART
#define PICO_DEFAULT_UART 0
#endif
#ifndef PICO_DEFAULT_UART_TX_PIN
#define PICO_DEFAULT_UART_TX_PIN 16
#endif
#ifndef PICO_DEFAULT_UART_RX_PIN
#define PICO_DEFAULT_UART_RX_PIN 17
#endif

// --- Neopixel Ring ---
#ifndef PICO_DEFAULT_WS2812_PIN
#define PICO_DEFAULT_WS2812_PIN 24
#endif

// --- On-Stamp Neopixel ---
#ifndef SOLDERPARTY_RP2040_STAMP_WS2812_PIN
#define SOLDERPARTY_RP2040_STAMP_WS2812_PIN 21
#endif

// --- I2C ---
#ifndef PICO_DEFAULT_I2C
#define PICO_DEFAULT_I2C       0
#endif
#ifndef PICO_DEFAULT_I2C_SDA_PIN
#define PICO_DEFAULT_I2C_SDA_PIN   0
#endif
#ifndef PICO_DEFAULT_I2C_SCL_PIN
#define PICO_DEFAULT_I2C_SCL_PIN   1
#endif

// --- SPI ---
#ifndef PICO_DEFAULT_SPI
#define PICO_DEFAULT_SPI 1
#endif
#ifndef PICO_DEFAULT_SPI_SCK_PIN
#define PICO_DEFAULT_SPI_SCK_PIN 10
#endif
#ifndef PICO_DEFAULT_SPI_TX_PIN
#define PICO_DEFAULT_SPI_TX_PIN 11
#endif
#ifndef PICO_DEFAULT_SPI_RX_PIN
#define PICO_DEFAULT_SPI_RX_PIN 8
Repository: raspberrypi/pico-sdk
File: src/common/pico_base/include/pico.h (C/C++)

Code before:
/*
 * Copyright (c) 2020 Raspberry Pi (Trading) Ltd.
 *
 * SPDX-License-Identifier: BSD-3-Clause
 */

#ifndef PICO_H_
#define PICO_H_

/** \file pico.h
 *  \defgroup pico_base pico_base
 *
 * Core types and macros for the Raspberry Pi Pico SDK. This header is intended to be included by all source code
 * as it includes configuration headers and overrides in the correct order
 *
 * This header may be included by assembly code
*/

#define	__PICO_STRING(x)	#x
#define	__PICO_XSTRING(x)	__PICO_STRING(x)

#include "pico/types.h"
#include "pico/version.h"

// PICO_CONFIG: PICO_CONFIG_HEADER, path to header include in place of the default pico/config.h which may be desirable for build systems which can't easily generate the config_autogen header, type=string, default=none, group=pico_base
#ifdef PICO_CONFIG_HEADER
#include __PICO_XSTRING(PICO_CONFIG_HEADER)
#else
#include "pico/config.h"
#endif
#include "pico/platform.h"
#include "pico/error.h"

#endif


Review comment (refactor):
When I run the `tools/extract_configs.py` script against this PR, it complains that `string` isn't a valid config type - it currently only expects `int`, `bool` or `enum`.
I _could_ add `string` as an additional config type, but https://github.com/raspberrypi/pico-project-generator/blob/master/pico_project.py#L547 (which is what makes use of the TSV files that  `extract_configs` creates) doesn't support `string` yet either.

Let me know if you want me to add `string` config items to `extract_configs` (would we also want minimum or maximum string-length validation?), and I'll add a corresponding issue to `pico-project-generator`.

Hmmm, although looking at the CONFIG descriptions, I wonder if `filename` would be a more suitable type than just `string`? :thinking: 

Code after:
/*
 * Copyright (c) 2020 Raspberry Pi (Trading) Ltd.
 *
 * SPDX-License-Identifier: BSD-3-Clause
 */

#ifndef PICO_H_
#define PICO_H_

/** \file pico.h
 *  \defgroup pico_base pico_base
 *
 * Core types and macros for the Raspberry Pi Pico SDK. This header is intended to be included by all source code
 * as it includes configuration headers and overrides in the correct order
 *
 * This header may be included by assembly code
*/

#define	__PICO_STRING(x)	#x
#define	__PICO_XSTRING(x)	__PICO_STRING(x)

#include "pico/types.h"
#include "pico/version.h"

// PICO_CONFIG: PICO_CONFIG_HEADER, unquoted path to header include in place of the default pico/config.h which may be desirable for build systems which can't easily generate the config_autogen header, default=none, group=pico_base
#ifdef PICO_CONFIG_HEADER
#include __PICO_XSTRING(PICO_CONFIG_HEADER)
#else
#include "pico/config.h"
#endif
#include "pico/platform.h"
#include "pico/error.h"

#endif
Repository: raspberrypi/pico-sdk
File: src/rp2_common/hardware_irq/irq.c (C)

Code before:
    for (uint i = 0; i < NUM_IRQS / 4; i++) {
        *p++ = prio4;
    }
#endif
}

#define FIRST_USER_IRQ (NUM_IRQS - NUM_USER_IRQS)

static uint get_user_irq_claim_index(uint irq_num) {
    invalid_params_if(IRQ, irq_num < FIRST_USER_IRQ || irq_num >= NUM_IRQS);
    // we count backwards from the last, to match the existing hard coded uses of user IRQs in the SDK which were previously using 31
    static_assert(NUM_IRQS - FIRST_USER_IRQ - 1 <= 8, ""); // we only use a single byte's worth of claim bits today.
    return NUM_IRQS - irq_num  - 1u;
}

void user_irq_claim(uint irq_num) {
    hw_claim_or_assert(&user_irq_claimed[get_core_num()], get_user_irq_claim_index(irq_num), "User IRQ is already claimed");
}

void user_irq_unclaim(uint irq_num) {
    hw_claim_clear(&user_irq_claimed[get_core_num()], get_user_irq_claim_index(irq_num));
}

int user_irq_claim_unused(bool required) {
    int bit = hw_claim_unused_from_range(&user_irq_claimed[get_core_num()], required, 0, NUM_USER_IRQS-1, "No user IRQs are available");
    if (bit >= 0) bit =  (int)NUM_IRQS - bit -1;
    return bit;
}

bool user_irq_is_claimed(uint irq_num) {
    return hw_is_claimed(&user_irq_claimed[get_core_num()], get_user_irq_claim_index(irq_num));
}



Review comment (suggestion):
~~Is it worth reusing `get_user_irq_claim_index` here?~~
EDIT: Ah no, my mistake, I think what you're doing here is the _inverse_ of `get_user_irq_claim_index`, it's just that the maths is the same for both :wink:  (`5 - 1 = 4` and `5 - 4 = 1`)

(also, weird spacing here, with double space after `=` and no space between `-` and `1`)

Code after:
    for (uint i = 0; i < NUM_IRQS / 4; i++) {
        *p++ = prio4;
    }
#endif
}

#define FIRST_USER_IRQ (NUM_IRQS - NUM_USER_IRQS)

static uint get_user_irq_claim_index(uint irq_num) {
    invalid_params_if(IRQ, irq_num < FIRST_USER_IRQ || irq_num >= NUM_IRQS);
    // we count backwards from the last, to match the existing hard coded uses of user IRQs in the SDK which were previously using 31
    static_assert(NUM_IRQS - FIRST_USER_IRQ <= 8, ""); // we only use a single byte's worth of claim bits today.
    return NUM_IRQS - irq_num  - 1u;
}

void user_irq_claim(uint irq_num) {
    hw_claim_or_assert(&user_irq_claimed[get_core_num()], get_user_irq_claim_index(irq_num), "User IRQ is already claimed");
}

void user_irq_unclaim(uint irq_num) {
    hw_claim_clear(&user_irq_claimed[get_core_num()], get_user_irq_claim_index(irq_num));
}

int user_irq_claim_unused(bool required) {
    int bit = hw_claim_unused_from_range(&user_irq_claimed[get_core_num()], required, 0, NUM_USER_IRQS - 1, "No user IRQs are available");
    if (bit >= 0) bit =  (int)NUM_IRQS - bit - 1;
    return bit;
}

bool user_irq_is_claimed(uint irq_num) {
    return hw_is_claimed(&user_irq_claimed[get_core_num()], get_user_irq_claim_index(irq_num));
}
Repository: raspberrypi/pico-sdk
File: src/boards/include/boards/pico.h (C/C++)

Code before:
#endif

#ifndef PICO_FLASH_SIZE_BYTES
#define PICO_FLASH_SIZE_BYTES (2 * 1024 * 1024)
#endif

// Drive high to force power supply into PWM mode (lower ripple on 3V3 at light loads)
#define PICO_SMPS_MODE_PIN 23

#ifndef PICO_RP2040_B0_SUPPORTED
#define PICO_RP2040_B0_SUPPORTED 1
#endif

// GPIO pin get VBUS
#ifndef PICO_VBUS_GPIO_PIN
#define PICO_VBUS_GPIO_PIN 24
#endif

// GPIO pin used to monitor VSYS
#ifndef PICO_VSYS_GPIO_PIN
#define PICO_VSYS_GPIO_PIN 29
#endif

// ADC to use to monitor VSYS
#ifndef PICO_VSYS_ADC_NUM
#define PICO_VSYS_ADC_NUM 3
#endif

#endif


Review comment (suggestion):
I wonder if `PICO_VSYS_ADC_CHANNEL` might be less ambiguous, as a name ending in `_NUM` _might_ be misinterpreted to mean "count of"? (IYSWIM!)  :shrug: 

Code after:
/*
 * Copyright (c) 2020 Raspberry Pi (Trading) Ltd.
 *
 * SPDX-License-Identifier: BSD-3-Clause
 */

// -----------------------------------------------------
// NOTE: THIS HEADER IS ALSO INCLUDED BY ASSEMBLER SO
//       SHOULD ONLY CONSIST OF PREPROCESSOR DIRECTIVES
// -----------------------------------------------------

// This header may be included by other board headers as "boards/pico.h"

#ifndef _BOARDS_PICO_H
#define _BOARDS_PICO_H

// For board detection
#define RASPBERRYPI_PICO

// --- UART ---
#ifndef PICO_DEFAULT_UART
#define PICO_DEFAULT_UART 0
#endif
#ifndef PICO_DEFAULT_UART_TX_PIN
#define PICO_DEFAULT_UART_TX_PIN 0
#endif
#ifndef PICO_DEFAULT_UART_RX_PIN
#define PICO_DEFAULT_UART_RX_PIN 1
#endif

// --- LED ---
#ifndef PICO_DEFAULT_LED_PIN
#define PICO_DEFAULT_LED_PIN 25
#endif
// no PICO_DEFAULT_WS2812_PIN

// --- I2C ---
#ifndef PICO_DEFAULT_I2C
#define PICO_DEFAULT_I2C 0
#endif
#ifndef PICO_DEFAULT_I2C_SDA_PIN
#define PICO_DEFAULT_I2C_SDA_PIN 4
#endif
#ifndef PICO_DEFAULT_I2C_SCL_PIN
#define PICO_DEFAULT_I2C_SCL_PIN 5
#endif

// --- SPI ---
#ifndef PICO_DEFAULT_SPI
#define PICO_DEFAULT_SPI 0
#endif
#ifndef PICO_DEFAULT_SPI_SCK_PIN
#define PICO_DEFAULT_SPI_SCK_PIN 18
#endif
#ifndef PICO_DEFAULT_SPI_TX_PIN
#define PICO_DEFAULT_SPI_TX_PIN 19
#endif
#ifndef PICO_DEFAULT_SPI_RX_PIN
#define PICO_DEFAULT_SPI_RX_PIN 16
#endif
#ifndef PICO_DEFAULT_SPI_CSN_PIN
#define PICO_DEFAULT_SPI_CSN_PIN 17
#endif

// --- FLASH ---

#define PICO_BOOT_STAGE2_CHOOSE_W25Q080 1

#ifndef PICO_FLASH_SPI_CLKDIV
#define PICO_FLASH_SPI_CLKDIV 2
#endif

#ifndef PICO_FLASH_SIZE_BYTES
#define PICO_FLASH_SIZE_BYTES (2 * 1024 * 1024)
#endif

// Drive high to force power supply into PWM mode (lower ripple on 3V3 at light loads)
#define PICO_SMPS_MODE_PIN 23

#ifndef PICO_RP2040_B0_SUPPORTED
#define PICO_RP2040_B0_SUPPORTED 1
#endif

// Pin get VBUS
#ifndef PICO_VBUS_PIN
#define PICO_VBUS_PIN 24
#endif

// Pin used to monitor VSYS using ADC
#ifndef PICO_VSYS_PIN
#define PICO_VSYS_PIN 29
#endif

#endif
Repository: raspberrypi/pico-sdk
File: src/boards/include/boards/pico_w.h (C/C++)

Code before:
#endif

#ifndef CYW43_PIN_WL_HOST_WAKE
#define CYW43_PIN_WL_HOST_WAKE 24
#endif

#ifndef CYW43_PIN_WL_REG_ON
#define CYW43_PIN_WL_REG_ON 23
#endif

#ifndef CYW43_WL_GPIO_COUNT
#define CYW43_WL_GPIO_COUNT 3
#endif

#ifndef CYW43_WL_GPIO_LED_PIN
#define CYW43_WL_GPIO_LED_PIN 0
#endif

// CYW43 GPIO to get VBUS
#ifndef CYW43_WL_GPIO_VBUS_PIN
#define CYW43_WL_GPIO_VBUS_PIN 2
#endif

// VSYS pin is shared with CYW43
#define CYW43_USES_VSYS_PIN 1

// Pin used to monitor VSYS
#ifndef PICO_VSYS_PIN
#define PICO_VSYS_PIN 29
#endif

// ADC to use to monitor VSYS
#ifndef PICO_VSYS_ADC_INPUT
#define PICO_VSYS_ADC_INPUT 3
#endif

#endif


Review comment (suggestion):
Apologies if this is a stupid question, but should this also be inside an `#ifndef` ?

Alternatively, given that this is the header-file specifically for Pico W, and there's no way to change which pins the CYW43 is connected to, perhaps _none_ of these settings should be inside an `#ifndef` ? :shrug: 

Code after:
#endif

#ifndef CYW43_PIN_WL_HOST_WAKE
#define CYW43_PIN_WL_HOST_WAKE 24
#endif

#ifndef CYW43_PIN_WL_REG_ON
#define CYW43_PIN_WL_REG_ON 23
#endif

#ifndef CYW43_WL_GPIO_COUNT
#define CYW43_WL_GPIO_COUNT 3
#endif

#ifndef CYW43_WL_GPIO_LED_PIN
#define CYW43_WL_GPIO_LED_PIN 0
#endif

// CYW43 GPIO to get VBUS
#ifndef CYW43_WL_GPIO_VBUS_PIN
#define CYW43_WL_GPIO_VBUS_PIN 2
#endif

// VSYS pin is shared with CYW43
#ifndef CYW43_USES_VSYS_PIN
#define CYW43_USES_VSYS_PIN 1
#endif

// Pin used to monitor VSYS using ADC
#ifndef PICO_VSYS_PIN
#define PICO_VSYS_PIN 29
#endif

#endif
Repository: raspberrypi/pico-sdk
File: src/rp2040/hardware_regs/include/hardware/platform_defs.h (C/C++)

Code before:

// PICO_CONFIG: XOSC_MHZ, The crystal oscillator frequency in MHz, type=int, default=12, advanced=true, group=hardware_base
#ifndef XOSC_MHZ
#define XOSC_MHZ _u(12)
#endif

 // Check for standard set-up
#if (XOSC_MHZ == 12 && SYS_CLK_MHZ == 125)
/// \tag::pll_settings[]
// Configure PLLs
//                   REF     FBDIV VCO            POSTDIV
// PLL SYS: 12 / 1 = 12MHz * 125 = 1500MHz / 6 / 2 = 125MHz
// PLL USB: 12 / 1 = 12MHz * 100 = 1200MHz / 5 / 5 =  48MHz
/// \end::pll_settings[]
#define PLL_SYS_VCO_FREQ_MHZ                1500
#define PLL_SYS_POSTDIV1                    6
#define PLL_SYS_POSTDIV2                    2

#define PLL_USB_VCO_FREQ_MHZ                1200
#define PLL_USB_POSTDIV1                    5
#define PLL_USB_POSTDIV2                    5

// Note: Do not trivially change this
#define PLL_COMMON_REFDIV                   1
#else
#error Use vcocalc.py to calculate correct values for the revised XOSC and/or system clock frequencies and define here.
#endif

#define FIRST_USER_IRQ (NUM_IRQS - NUM_USER_IRQS)
#define VTABLE_FIRST_IRQ 16

#endif



Review comment (suggestion):
this should only be a #error if the six/seven things aren't defined (we do not expect people to edit this header, we expect them to set them in their compile_defintions or via a board header)

also,

separate out the definition checks  USB since it seems to be independent of SYS_CLK_MHZ, i.e. two separate checks for 3 defines, vs 1 for 6.

also presumably PLL_COMMON_REFDIV maybe should be just #ifndef-ed above all of this and defaulted to 1.



Code after:
#endif

// PICO_CONFIG: SYS_CLK_KHZ, The system operating frequency in kHz, type=int, default=125000, advanced=true, group=hardware_base
#ifndef SYS_CLK_KHZ
#define SYS_CLK_KHZ _u(125000)
#endif

// PICO_CONFIG: USB_CLK_KHZ, USB clock frequency. Must be 48MHz for the USB interface to operate correctly, type=int, default=48000, advanced=true, group=hardware_base
#ifndef USB_CLK_KHZ
#define USB_CLK_KHZ _u(48000)
#endif

// For backwards compatibility define XOSC_MHZ if the frequency is indeed an integer number of Mhz.
#if defined(XOSC_KHZ) && !defined(XOSC_MHZ) && (XOSC_KHZ % 1000 == 0)
#define XOSC_MHZ (XOSC_KHZ / 1000)
#endif

// For backwards compatibility define SYS_CLK_MHZ if the frequency is indeed an integer number of Mhz.
#if defined(SYS_CLK_KHZ) && !defined(SYS_CLK_MHZ) && (SYS_CLK_KHZ % 1000 == 0)
#define SYS_CLK_MHZ (SYS_CLK_KHZ / 1000)
#endif

// For backwards compatibility define USB_CLK_MHZ if the frequency is indeed an integer number of Mhz.
#if defined(USB_CLK_KHZ) && !defined(USB_CLK_MHZ) && (USB_CLK_KHZ % 1000 == 0)
#define USB_CLK_MHZ (USB_CLK_KHZ / 1000)
#endif

#define FIRST_USER_IRQ (NUM_IRQS - NUM_USER_IRQS)
#define VTABLE_FIRST_IRQ 16

#endif
Repository: raspberrypi/pico-sdk
File: src/rp2040/hardware_regs/include/hardware/platform_defs.h (C/C++)

Code before:
#define SYS_CLK_MHZ _u(125)
#endif

// PICO_CONFIG: XOSC_MHZ, The crystal oscillator frequency in MHz, type=int, default=12, advanced=true, group=hardware_base
#ifndef XOSC_MHZ
#define XOSC_MHZ _u(12)
#endif

 // Check for standard set-up
#if (XOSC_MHZ == 12 && SYS_CLK_MHZ == 125)
/// \tag::pll_settings[]
// Configure PLLs
//                   REF     FBDIV VCO            POSTDIV
// PLL SYS: 12 / 1 = 12MHz * 125 = 1500MHz / 6 / 2 = 125MHz
// PLL USB: 12 / 1 = 12MHz * 100 = 1200MHz / 5 / 5 =  48MHz
/// \end::pll_settings[]
#define PLL_SYS_VCO_FREQ_MHZ                1500
#define PLL_SYS_POSTDIV1                    6
#define PLL_SYS_POSTDIV2                    2

#define PLL_USB_VCO_FREQ_MHZ                1200
#define PLL_USB_POSTDIV1                    5
#define PLL_USB_POSTDIV2                    5

// Note: Do not trivially change this
#define PLL_COMMON_REFDIV                   1
#else
#error Use vcocalc.py to calculate correct values for the revised XOSC and/or system clock frequencies and define here.
#endif

#define FIRST_USER_IRQ (NUM_IRQS - NUM_USER_IRQS)
#define VTABLE_FIRST_IRQ 16

#endif



Review comment (suggestion):
https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/hardware_pll/pll.c#L55 seems to suggest that each PLL has its own REFDIV, rather than there being a single "global" REFDIV? I.e. should there be separate defines for `PLL_SYS_REFDIV` and `PLL_USB_REFDIV` ? :man_shrugging: 

Code after:
#ifndef XOSC_KHZ
#define XOSC_KHZ _u(12000)
#endif

// PICO_CONFIG: SYS_CLK_KHZ, The system operating frequency in kHz, type=int, default=125000, advanced=true, group=hardware_base
#ifndef SYS_CLK_KHZ
#define SYS_CLK_KHZ _u(125000)
#endif

// PICO_CONFIG: USB_CLK_KHZ, USB clock frequency. Must be 48MHz for the USB interface to operate correctly, type=int, default=48000, advanced=true, group=hardware_base
#ifndef USB_CLK_KHZ
#define USB_CLK_KHZ _u(48000)
#endif

// For backwards compatibility define XOSC_MHZ if the frequency is indeed an integer number of Mhz.
#if defined(XOSC_KHZ) && !defined(XOSC_MHZ) && (XOSC_KHZ % 1000 == 0)
#define XOSC_MHZ (XOSC_KHZ / 1000)
#endif

// For backwards compatibility define SYS_CLK_MHZ if the frequency is indeed an integer number of Mhz.
#if defined(SYS_CLK_KHZ) && !defined(SYS_CLK_MHZ) && (SYS_CLK_KHZ % 1000 == 0)
#define SYS_CLK_MHZ (SYS_CLK_KHZ / 1000)
#endif

// For backwards compatibility define USB_CLK_MHZ if the frequency is indeed an integer number of Mhz.
#if defined(USB_CLK_KHZ) && !defined(USB_CLK_MHZ) && (USB_CLK_KHZ % 1000 == 0)
#define USB_CLK_MHZ (USB_CLK_KHZ / 1000)
#endif

#define FIRST_USER_IRQ (NUM_IRQS - NUM_USER_IRQS)
#define VTABLE_FIRST_IRQ 16

#endif