add rom_reset_usb_boot_extra which supports >32 pins and ACTIVE_LOW #2084

2024-11-21T10:55:27-06:00

kilograham commented

2024-11-21 10:55:27 -06:00

(Migrated from github.com)

I noticed this in passing - this change also adds relevant PICO_CONFIG to specify the ACTIVE_LOW default (note if the ACTIVITY_LED is not specified, the OTP defaults will be used on RP2350)

Wren6991 (Migrated from github.com) reviewed

2024-11-21 10:55:27 -06:00

peterharperuk (Migrated from github.com) approved these changes

2024-11-21 13:09:11 -06:00

peterharperuk (Migrated from github.com) left a comment

Seems to work!

lurch (Migrated from github.com) reviewed

2024-11-21 13:35:17 -06:00

src/rp2_common/pico_bootrom/include/pico/bootrom.h

					
				@ -240,0 +244,4 @@

				 * This function reboots the device into the BOOTSEL mode ('usb boot").

				 *

				 * Facilities are provided to enable an "activity light" via GPIO attached LED for the USB Mass Storage Device,

				 * and to limit the USB interfaces exposed.

This chunk of doxygen is identical to that for rom_reset_usb_boot, which will probably look pretty confusing in the rendered Doxygen output? (both https://datasheets.raspberrypi.com/pico/raspberry-pi-pico-c-sdk.pdf and https://www.raspberrypi.com/documentation/pico-sdk/ )

Is the addition of active_low the only difference between rom_reset_usb_boot and rom_reset_usb_boot_extra ?
EDIT: Ah, according to the PR's title rom_reset_usb_boot_extra also supports more than 32 pins 😂

This chunk of doxygen is **identical** to that for `rom_reset_usb_boot`, which will probably look pretty confusing in the rendered Doxygen output? (both https://datasheets.raspberrypi.com/pico/raspberry-pi-pico-c-sdk.pdf and https://www.raspberrypi.com/documentation/pico-sdk/ ) Is the addition of active_low the only difference between `rom_reset_usb_boot` and `rom_reset_usb_boot_extra` ? EDIT: Ah, according to the PR's title `rom_reset_usb_boot_extra` also supports more than 32 pins :joy:

lurch (Migrated from github.com) reviewed

2024-11-21 13:38:37 -06:00

src/rp2_common/pico_bootrom/include/pico/bootrom.h

					
				@ -240,0 +254,4 @@

				 *  - 2 To disable the USB PICOBOOT Interface

				 * \param usb_activity_gpio_pin_active_low Activity GPIO is active low (ignored on RP2040)

				 */

				void __attribute__((noreturn)) rom_reset_usb_boot_extra(int usb_activity_gpio_pin, uint32_t disable_interface_mask, bool usb_activity_gpio_pin_active_low);

rom_reset_usb_boot has uint32_t usb_activity_gpio_pin_mask, whereas rom_reset_usb_boot_extra has int usb_activity_gpio_pin. This might be a stupid question, but given the similarity of these two functions, would it make sense to modify rom_reset_usb_boot_extra to accept a pin_mask value too?

`rom_reset_usb_boot` has `uint32_t usb_activity_gpio_pin_mask`, whereas `rom_reset_usb_boot_extra` has `int usb_activity_gpio_pin`. This might be a stupid question, but given the similarity of these two functions, would it make sense to modify `rom_reset_usb_boot_extra` to accept a `pin_mask` value too?

peterharperuk (Migrated from github.com) reviewed

2024-11-21 13:56:40 -06:00

src/rp2_common/pico_bootrom/include/pico/bootrom.h

					
				@ -240,0 +244,4 @@

				 * This function reboots the device into the BOOTSEL mode ('usb boot").

				 *

				 * Facilities are provided to enable an "activity light" via GPIO attached LED for the USB Mass Storage Device,

				 * and to limit the USB interfaces exposed.

It doesn't use a mask

lurch commented

2024-11-21 13:59:23 -06:00

(Migrated from github.com)

(note if the ACTIVITY_LED is not specified, the OTP defaults will be used on RP2350)

Given that the OTP BOOTSEL_LED_CFG has both PIN and ACTIVELOW fields, should we make PICO_BOOTSEL_VIA_DOUBLE_RESET_ACTIVITY_LED_ACTIVE_LOW and PICO_STDIO_USB_RESET_BOOTSEL_FIXED_ACTIVITY_LED_ACTIVE_LOW optional-and-unset-by-default (just like their _ACTIVITY_LED counterparts), rather than forcing them to default to 0 ?

> (note if the ACTIVITY_LED is not specified, the OTP defaults will be used on RP2350) Given that the OTP `BOOTSEL_LED_CFG` has both `PIN` and `ACTIVELOW` fields, should we make `PICO_BOOTSEL_VIA_DOUBLE_RESET_ACTIVITY_LED_ACTIVE_LOW` and `PICO_STDIO_USB_RESET_BOOTSEL_FIXED_ACTIVITY_LED_ACTIVE_LOW` optional-and-unset-by-default (just like their `_ACTIVITY_LED` counterparts), rather than forcing them to default to 0 ?

kilograham commented

2024-11-21 16:48:01 -06:00

(Migrated from github.com)

(note if the ACTIVITY_LED is not specified, the OTP defaults will be used on RP2350)

Given that the OTP BOOTSEL_LED_CFG has both PIN and ACTIVELOW fields, should we make PICO_BOOTSEL_VIA_DOUBLE_RESET_ACTIVITY_LED_ACTIVE_LOW and PICO_STDIO_USB_RESET_BOOTSEL_FIXED_ACTIVITY_LED_ACTIVE_LOW optional-and-unset-by-default (just like their _ACTIVITY_LED counterparts), rather than forcing them to default to 0 ?

they are only used when the corresponding PIN define is set

> > (note if the ACTIVITY_LED is not specified, the OTP defaults will be used on RP2350) > > Given that the OTP `BOOTSEL_LED_CFG` has both `PIN` and `ACTIVELOW` fields, should we make `PICO_BOOTSEL_VIA_DOUBLE_RESET_ACTIVITY_LED_ACTIVE_LOW` and `PICO_STDIO_USB_RESET_BOOTSEL_FIXED_ACTIVITY_LED_ACTIVE_LOW` optional-and-unset-by-default (just like their `_ACTIVITY_LED` counterparts), rather than forcing them to default to 0 ? they are only used when the corresponding PIN define is set

👍 1

lurch (Migrated from github.com) reviewed

2024-11-25 04:16:48 -06:00

src/rp2_common/pico_stdio_usb/include/pico/stdio_usb.h

					
				@ -75,1 +75,4 @@

				// PICO_CONFIG: PICO_STDIO_USB_RESET_BOOTSEL_ACTIVITY_LED_ACTIVE_LOW, Whether pin to use as bootloader activity LED when BOOTSEL mode is entered via USB (either VIA_BAUD_RATE or VIA_VENDOR_INTERFACE) is active low, type=bool, default=0, group=pico_stdio_usb

				#ifndef PICO_STDIO_USB_RESET_BOOTSEL_FIXED_ACTIVITY_LED_ACTIVE_LOW

				#define PICO_STDIO_USB_RESET_BOOTSEL_FIXED_ACTIVITY_LED_ACTIVE_LOW 0