gh-raspberrypi/pico-sdk
Watch
1
0
Fork
You've already forked pico-sdk
 0
Code Issues 262 Pull requests 63 Releases Activity

add some struct doxygen #3022

Merged
kilograham merged 7 commits from struct-doxygen into develop 2026-06-24 10:18:54 -05:00
Conversation 14 Commits 7 Files changed 32 +518 -243
kilograham commented 2026-06-21 10:36:07 -05:00 (Migrated from github.com)

thanks claude!

thanks claude!
will-v-pi (Migrated from github.com) reviewed 2026-06-22 05:15:09 -05:00
will-v-pi (Migrated from github.com) left a comment

I have a few comments - generally, I think the picobin/picoboot docs should use similar wording to the datasheet for consistency, either changing the datasheet to match these descriptions, or changing these descriptions to align with the datasheet

I have a few comments - generally, I think the picobin/picoboot docs should use similar wording to the datasheet for consistency, either changing the datasheet to match these descriptions, or changing these descriptions to align with the datasheet
src/common/boot_picobin_headers/include/boot/picobin.h
@ -147,16 +147,26 @@
#include <stdbool.h>
will-v-pi (Migrated from github.com) commented 2026-06-22 04:34:52 -05:00

This should also include descriptions for absolute load maps - also, should probably use the wording more similar to the datasheet, for consistency (eg relative to the address of this LOAD_MAP item rather than relative to the load map base)

This should also include descriptions for absolute load maps - also, should probably use the wording more similar to the datasheet, for consistency (eg `relative to the address of this LOAD_MAP item` rather than `relative to the load map base`)
will-v-pi (Migrated from github.com) commented 2026-06-22 04:36:14 -05:00

Also encodes the item type and the size of the item

Also encodes the item type and the size of the item
src/common/boot_picoboot_headers/include/boot/picoboot.h
@ -82,30 +82,48 @@ enum picoboot_status {
PICOBOOT_UNSUPPORTED_MODIFICATION = 17,
will-v-pi (Migrated from github.com) commented 2026-06-22 04:38:58 -05:00

This should note that dPC can only be 0 or a RAM address, and that dSP is ignored unless booting into a RAM address - maybe using similar wording to the datasheet?

This should note that `dPC` can only be 0 or a RAM address, and that `dSP` is ignored unless booting into a RAM address - maybe using similar wording to the datasheet?
will-v-pi (Migrated from github.com) commented 2026-06-22 04:39:44 -05:00

This should be RP2040 only

This should be RP2040 only
will-v-pi (Migrated from github.com) commented 2026-06-22 04:41:01 -05:00

This information probably isn't very helpful - the fields for REBOOT2 seem unrelated to the fields for REBOOT?

This information probably isn't very helpful - the fields for `REBOOT2` seem unrelated to the fields for `REBOOT`?
will-v-pi (Migrated from github.com) commented 2026-06-22 04:42:55 -05:00

Might be better documented as (not available on RP2040)?

Might be better documented as `(not available on RP2040)`?
will-v-pi (Migrated from github.com) commented 2026-06-22 04:43:57 -05:00

I think this is RP2040 only, as EXEC and VECTORIZE_FLASH aren't supported on RP2350

I think this is RP2040 only, as `EXEC` and `VECTORIZE_FLASH` aren't supported on RP2350
will-v-pi (Migrated from github.com) commented 2026-06-22 04:45:53 -05:00

This probably shouldn't appear in the docs - it's only here to preserve backwards compatibility with bootrom builds

This probably shouldn't appear in the docs - it's only here to preserve backwards compatibility with bootrom builds
will-v-pi (Migrated from github.com) commented 2026-06-22 04:48:20 -05:00

Same for other (RP2350 only) commands

Same for other `(RP2350 only)` commands
will-v-pi (Migrated from github.com) commented 2026-06-22 04:52:52 -05:00

These aren't documented in the datasheet, and don't seem to be used by picotool - are these dummy parameters?

These aren't documented in the datasheet, and don't seem to be used by `picotool` - are these dummy parameters?
src/common/boot_picoboot_headers/include/boot/picoboot.h
@ -124,2 +150,4 @@
};
/*! \brief Parameters for an OTP read or write command (not available on RP2040)
* \ingroup boot_picoboot_headers
will-v-pi (Migrated from github.com) commented 2026-06-22 04:47:10 -05:00

This enum shouldn't be deleted - it should be documented as it's referenced below

This enum shouldn't be deleted - it should be documented as it's referenced below
src/common/boot_uf2_headers/include/boot/uf2.h
@ -45,18 +45,24 @@
// 04 e3 57 99
will-v-pi (Migrated from github.com) commented 2026-06-22 04:56:22 -05:00

Should this be documented like the other fields? Presumbaly this should always be the family ID for RP2 UF2 blocks

Should this be documented like the other fields? Presumbaly this should always be the family ID for RP2 UF2 blocks
src/common/pico_base_headers/include/pico/types.h
will-v-pi (Migrated from github.com) commented 2026-06-22 04:57:48 -05:00

Does this need documenting? I'm pretty sure the doxygen build won't pick it up anyway, as PICO_OPAQUE_ABSOLUTE_TIME_T won't be set

Does this need documenting? I'm pretty sure the doxygen build won't pick it up anyway, as `PICO_OPAQUE_ABSOLUTE_TIME_T` won't be set
will-v-pi commented 2026-06-22 05:18:08 -05:00 (Migrated from github.com)

Is this PR supposed to update btstack (which is causing the other-bazel-checks failures), or should that be in a separate PR? I think the deleted picoboot_exclusive_type enum is causing the other CI failures.

Is this PR supposed to update `btstack` (which is causing the `other-bazel-checks` failures), or should that be in a separate PR? I think the deleted `picoboot_exclusive_type` enum is causing the other CI failures.
kilograham (Migrated from github.com) reviewed 2026-06-22 08:35:41 -05:00
src/common/boot_picobin_headers/include/boot/picobin.h
@ -147,16 +147,26 @@
#include <stdbool.h>
kilograham (Migrated from github.com) commented 2026-06-22 08:35:40 -05:00

can you please update/suggest

can you please update/suggest
will-v-pi (Migrated from github.com) reviewed 2026-06-22 08:43:55 -05:00
src/common/boot_picobin_headers/include/boot/picobin.h
@ -147,16 +147,26 @@
#include <stdbool.h>
will-v-pi (Migrated from github.com) commented 2026-06-22 08:43:55 -05:00 
    uint32_t storage_address_rel; ///< Storage address of the region - if absolute then an absolute address, otherwise relative to the address of the LOAD_MAP item containing this entry
    uint32_t runtime_address;     ///< Target runtime address to which the region is loaded
    uint32_t size;                ///< If absolute then the end of the runtime address region, otherwise the size of the region in bytes
```suggestion uint32_t storage_address_rel; ///< Storage address of the region - if absolute then an absolute address, otherwise relative to the address of the LOAD_MAP item containing this entry uint32_t runtime_address; ///< Target runtime address to which the region is loaded uint32_t size; ///< If absolute then the end of the runtime address region, otherwise the size of the region in bytes ```
will-v-pi (Migrated from github.com) reviewed 2026-06-22 09:12:32 -05:00
src/common/boot_picobin_headers/include/boot/picobin.h
@ -147,16 +147,26 @@
#include <stdbool.h>
will-v-pi (Migrated from github.com) commented 2026-06-22 09:12:32 -05:00

Have pushed an update which addresses most of my comments - also written using claude, but I gave it the datasheets so it had a source of truth to write the comments from

Have pushed an update which addresses most of my comments - also written using claude, but I gave it the datasheets so it had a source of truth to write the comments from
will-v-pi (Migrated from github.com) reviewed 2026-06-23 09:25:35 -05:00
src/common/boot_picoboot_headers/include/boot/picoboot.h
@ -82,30 +82,48 @@ enum picoboot_status {
PICOBOOT_UNSUPPORTED_MODIFICATION = 17,
will-v-pi (Migrated from github.com) commented 2026-06-23 09:25:34 -05:00

@kilograham this question still needs answering - are these unused parameters (and should therefore be documented as such), or are these used by some versions of get_info? As far as I can tell, picotool only uses the dParams below, and these bParam and wParam are unused.

@kilograham this question still needs answering - are these unused parameters (and should therefore be documented as such), or are these used by some versions of `get_info`? As far as I can tell, `picotool` only uses the `dParams` below, and these `bParam` and `wParam` are unused.
kilograham (Migrated from github.com) reviewed 2026-06-24 09:38:58 -05:00
src/common/boot_picoboot_headers/include/boot/picoboot.h
@ -82,30 +82,48 @@ enum picoboot_status {
PICOBOOT_UNSUPPORTED_MODIFICATION = 17,
kilograham (Migrated from github.com) commented 2026-06-24 09:38:35 -05:00 
    uint8_t bParam; ///< Unused
```suggestion uint8_t bParam; ///< Unused ```
kilograham (Migrated from github.com) commented 2026-06-24 09:38:46 -05:00 
    uint16_t wParam; ///< Unused
```suggestion uint16_t wParam; ///< Unused ```
will-v-pi (Migrated from github.com) approved these changes 2026-06-24 09:50:43 -05:00
lurch (Migrated from github.com) reviewed 2026-06-27 06:24:57 -05:00
src/common/pico_binary_info/include/pico/binary_info/structure.h
@ -134,3 +171,3 @@
typedef struct __packed _binary_info_pins64_with_func {
struct _binary_info_core core;
struct _binary_info_core core; ///< Common binary info header
// p6_8 : p5_8 : p4_8 : p3_8 : p2_8 : p1_8 : p0_8 : func_5 : 010_3 //individual pins p0,p1,p2 ... if fewer than 7 then duplicate p
lurch (Migrated from github.com) commented 2026-06-27 06:24:57 -05:00

Should the existing // style comment here have been converted into a ///< style comment? (same also on line 151)

Should the existing `// ` style comment here have been converted into a `///< ` style comment? (same also on line 151)
lurch (Migrated from github.com) reviewed 2026-06-27 06:40:50 -05:00
src/common/pico_binary_info/include/pico/binary_info/structure.h
@ -155,13 +198,19 @@ typedef struct __packed _binary_info_pins64_with_name {
#define BI_NAMED_GROUP_SORT_ALPHA 0x0004 // default is no sort
lurch (Migrated from github.com) commented 2026-06-27 06:40:50 -05:00

Unlike _binary_info_pins64_with_name (which has a uint64_t pin_mask) I don't think that this is limited to just "up to 64" pins? The comments on lines 173 and 174 implies that the pin numbers are encoded with 8 bits, so it seems like this could actually encode up to 256 pins?

Unlike `_binary_info_pins64_with_name` (which has a `uint64_t pin_mask`) I _don't_ think that this is limited to just "up to 64" pins? The comments on lines 173 and 174 implies that the pin numbers are encoded with 8 bits, so it seems like this could actually encode up to 256 pins?
lurch (Migrated from github.com) reviewed 2026-06-27 07:10:47 -05:00
src/common/pico_time/include/pico/timeout_helper.h
@ -16,0 +18,4 @@
*
* Holds the deadline and an optional per-iteration parameter used by
* the check_timeout_fn callbacks returned by the init functions.
*/
lurch (Migrated from github.com) commented 2026-06-27 07:10:46 -05:00

Would have been nice to add doxygen for init_single_timeout_until and init_per_iteration_timeout_us too 😉

Would have been nice to add doxygen for `init_single_timeout_until` and `init_per_iteration_timeout_us` too :wink:
lurch (Migrated from github.com) reviewed 2026-06-27 07:24:35 -05:00
src/common/pico_util/include/pico/util/queue.h
@ -32,0 +32,4 @@
/*! \brief A multi-core and IRQ safe queue instance
* \ingroup queue
*
* Stores a fixed number of elements of a fixed size. All operations are
lurch (Migrated from github.com) commented 2026-06-27 07:24:35 -05:00

The "Stores a fixed number of elements" seems to be in contrast with the queue_try_add and queue_try_remove functions? 😉
Perhaps this was supposed to say "Stores a maximum number of elements"?

The "Stores a fixed number of elements" seems to be in contrast with the `queue_try_add` and `queue_try_remove` functions? :wink: Perhaps this was supposed to say "Stores a maximum number of elements"?
lurch (Migrated from github.com) reviewed 2026-06-27 07:36:43 -05:00
src/rp2_common/hardware_interp/include/hardware/interp.h
lurch (Migrated from github.com) commented 2026-06-27 07:36:43 -05:00

This "for lanes 0, 1 and 2" comment implies that there are 3 lanes, which isn't the case? (From a quick look at the RP2040 Datasheet, it looks like BASE2 is just a "mix" of both lanes?)

This "for lanes 0, 1 and 2" comment implies that there are 3 lanes, which isn't the case? (From a quick look at the RP2040 Datasheet, it looks like BASE2 is just a "mix" of both lanes?)
lurch commented 2026-06-27 17:46:29 -05:00 (Migrated from github.com)

Please ignore my post-merge comments here - I've now created #3038 instead.

Please ignore my post-merge comments here - I've now created #3038 instead.
Sign in to join this conversation.
Reviewers
No reviewers
will-v-pi
Labels
Clear labels   
bazel

   
bluetooth

   
board settings

   
boot_stage2

   
bug
Something isn't working

  
build

   
c++

   
can't reproduce
Can not reproduce the bug

  
clang

   
clib-interface

   
cmsis

   
code cleanliness

   
code size

   
compiler version issues

   
cyw43_driver

   
debug

   
discussion

   
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
dynamic_clock

   
elf2uf2

   
enhancement
New feature or request

  
errata

   
filesystem

   
flash layout

   
floating point

   
FreeRTOS

   
hardware_adc

   
hardware_claim

   
hardware_clocks

   
hardware_dma

   
hardware_exception

   
hardware_flash

   
hardware_gpio

   
hardware_i2c

   
hardware_irq

   
hardware_pio

   
hardware_pwm

   
hardware_rcp

   
hardware_rtc

   
hardware_spi

   
hardware_sync

   
hardware_uart

   
hardware_vreg

   
hardware_watchdog

   
hardware_xosc

   
help wanted
Extra attention is needed

  
host

   
IAR

   
installation

   
invalid
This doesn't seem right

  
linker scripts

   
lto

   
lwip

   
mbedtls

   
missing API

   
needs reporter detail

   
networking

   
newlib

   
newlib-multicore

   
non-c

   
non-cmake

   
non-gcc

   
openocd

   
orphan PR

   
OS integration

   
pico_async_context

   
pico_base

   
pico_binary_info

   
pico_flash

   
picolibc

   
pico_lwip

   
pico_malloc

   
pico_multicore

   
pico_printf

   
pico_runtime

   
pico_stdio

   
pico_stdio_usb

   
pico_sync

   
pico_time

   
pico_util

   
pioasm

   
port
issues or discussions of porting code/libraries to rp2040

  
question
Further information is requested

  
review

   
review2

   
rtos interop
Consideration for FreeRTOS co-existence

  
runtime init

   
static library

   
svd

   
test

   
tinyusb

   
vscode

   
wifi integration

   
wontfix
This will not be worked on

No labels
bazel
bluetooth
board settings
boot_stage2
bug
build
c++
can't reproduce
clang
clib-interface
cmsis
code cleanliness
code size
compiler version issues
cyw43_driver
debug
discussion
documentation
duplicate
dynamic_clock
elf2uf2
enhancement
errata
filesystem
flash layout
floating point
FreeRTOS
hardware_adc
hardware_claim
hardware_clocks
hardware_dma
hardware_exception
hardware_flash
hardware_gpio
hardware_i2c
hardware_irq
hardware_pio
hardware_pwm
hardware_rcp
hardware_rtc
hardware_spi
hardware_sync
hardware_uart
hardware_vreg
hardware_watchdog
hardware_xosc
help wanted
host
IAR
installation
invalid
linker scripts
lto
lwip
mbedtls
missing API
needs reporter detail
networking
newlib
newlib-multicore
non-c
non-cmake
non-gcc
openocd
orphan PR
OS integration
pico_async_context
pico_base
pico_binary_info
pico_flash
picolibc
pico_lwip
pico_malloc
pico_multicore
pico_printf
pico_runtime
pico_stdio
pico_stdio_usb
pico_sync
pico_time
pico_util
pioasm
port
question
review
review2
rtos interop
runtime init
static library
svd
test
tinyusb
vscode
wifi integration
wontfix
Milestone
Clear milestone
No items
No milestone
2.3.0
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-raspberrypi/pico-sdk!3022
No description provided.