Add PIO support for RP2040 and RP2350 - #3449
Open
begeistert wants to merge 61 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Pico hardware API build and target enablement, discovers the new native package, registers its assembly, and implements PIO block, state-machine, IRQ, and target helper support for RP2040/RP2350 targets. ChangesPico hardware API, native PIO runtime, and IRQ dispatch
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
josesimoes
added
Type: enhancement
Area: Config-and-Build
Status: under review
Series: RPI Pico
Member
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CMake/Modules/FindNF_NativeAssemblies.cmake`:
- Line 45: The CMake metadata still uses the old Hardware.Rp2040 label in the
option/help text and nearby block comment, while this module now maps
API_Hardware.Rpi to nanoFramework.Hardware.Rpi. Update the option declaration
and the related comment text in FindNF_NativeAssemblies.cmake to use the new API
name consistently so cache entries and build logs match the actual surface,
including the same naming in the referenced block around the other affected
lines.
In `@Kconfig.apis`:
- Around line 116-120: Update the Kconfig prompt for API_HARDWARE_RPI so it
matches the symbol naming and broader hardware scope; the current
"Hardware.Rp2040" prompt is misleading and should be aligned with the existing
CMake mapping and peer prompts like API_HARDWARE_STM32. Adjust the prompt text
in the API_HARDWARE_RPI definition to use the same Hardware.* naming convention
and reflect that it covers both RP2040 and RP2350.
In
`@targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Rpi/nf_hardware_rpi_native_nanoFramework_Hardware_Rpi_Pio_PioBlock.cpp`:
- Around line 1-6: The file header in the new native PIO source uses a custom
copyright notice instead of the required standard repository header. Update the
top-of-file comment in PioBlock so it matches the project’s canonical .NET
Foundation and Contributors copyright text used across source files, keeping the
rest of the file unchanged.
- Around line 73-74: The PIO reset release is currently tied only to the first
`NativeAddProgram()` path via `PioEnsureOutOfReset(param0)`, so other
hardware-touching entry points can still write while the peripheral remains
reset. Move the reset-release logic into shared per-block initialization for the
`PioBlock` flow, and invoke it from every entry point that accesses the PIO
hardware, including `NativeInitGpio()`, the `PioStateMachine` register-writing
paths, and `PioIrqDriver`, so behavior no longer depends on call order.
- Around line 76-83: The copy logic around param1.GetBuffer(), param2, and the
INSTR_MEM write must validate the marshaled instruction array size before any
read occurs. Add a bounds check that ensures param1.GetSize() is at least length
(and keep the existing 1..32 validation) before entering the loop that reads
instr[i], and apply the same safeguard in the other instruction-loading block
around the same native handler to prevent reading past the buffer.
In
`@targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Rpi/nf_hardware_rpi_native_nanoFramework_Hardware_Rpi_Pio_PioBlock.h`:
- Around line 1-12: The generated PioBlock interop header is missing the
required .NET Foundation copyright header and starts directly with the warning
banner. Update the generator/template that emits nanoFramework.Hardware.Rpi PIO
headers so the standard project header is prepended before the autogenerated
warning, ensuring future outputs from the PioBlock header generation path
include the mandated copyright text.
In
`@targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Rpi/nf_hardware_rpi_native_nanoFramework_Hardware_Rpi_Pio_PioStateMachine.cpp`:
- Around line 233-260: NativeSetConsecutivePinDirs() currently accepts param2
and param3 without range checks, so invalid pin starts, non-positive counts, or
ranges that run past the GPIO limit can be written into PINCTRL and affect the
wrong pins. Add validation before entering the loop to reject negative pin
values, reject zero or negative pin counts, and ensure the entire pin span stays
within the valid RP2040/RP2350 GPIO range before programming the state machine;
keep the existing PioFromIndex()/sm checks and return CLR_E_INVALID_PARAMETER
for bad inputs.
- Around line 391-400: Validate the clock-divisor parts before writing
PIOStateMachine state: in PioStateMachine methods, check that param2 and param3
are within the valid unsigned ranges for the CLKDIV integer and fractional
fields before packing them into pio->SM[sm].CLKDIV. If either value is negative
or exceeds the field width, set hr to CLR_E_INVALID_PARAMETER and return instead
of letting the cast silently wrap.
- Around line 61-117: The PioStateMachine reconfiguration path only checks blob
length, so invalid config values can be silently truncated when packing
`PINCTRL`, `EXECCTRL`, and `SHIFTCTRL`, and `param2` is masked into
`SM[sm].INSTR` instead of being validated. Add explicit range checks before
writing the registers in the `PioStateMachine` setup method so each `PIO_CFG_*`
field and the start offset fit the hardware bit widths, and return
`CLR_E_INVALID_PARAMETER` when any value is out of range.
In
`@targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Rpi/nf_hardware_rpi_native_PioIrqDriver.cpp`:
- Around line 114-129: The PioIrqDriver enable/disable path in
PioIrqDriver::Enable currently returns success even when RP_PIO_REQUIRED
compiles out all IRQ wiring. Update this branch so the method reports failure or
otherwise prevents enabling the dispatcher when IRQ delivery is unavailable,
instead of leaving the no-op path to fall through with S_OK. Keep the normal
NVIC setup logic for the non-RP_PIO_REQUIRED case, and make the compiled-out
path in nf_hardware_rpi_native_PioIrqDriver.cpp explicitly signal that the
driver cannot be enabled.
- Around line 93-102: The PioIrqInitialize path currently overwrites
s_pioCtx[block] for an already-registered block, which allows a second
NativeEventDispatcher for the same PIO block to replace the first one. Update
PioIrqInitialize to detect when s_pioCtx[block] is already set and reject the
new registration with an invalid/occupied status instead of replacing it, while
preserving the existing context for BlockOfContext(), disable, and cleanup
flows.
In
`@targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Rpi/nf_hardware_rpi_native_target.h`:
- Around line 1-6: The top-of-file banner in nf_hardware_rpi_native_target.h
uses a custom contributor header instead of the repository’s standard copyright
block. Replace the current comment header with the accepted .NET Foundation
header, including both the .NET Foundation copyright line and the LICENSE line,
and keep the rest of the file content unchanged.
In
`@targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Rpi/nf_hardware_rpi_native.cpp`:
- Around line 1-3: The new source file is missing the required repository header
at the top. Update nf_hardware_rpi_native.cpp so it begins with the standard
.NET Foundation copyright and license comment block before any includes or other
content, keeping the existing native table code and symbols like
nf_hardware_rpi_native.h unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: ffec082d-8622-43fd-bacb-700d82a3db1c
📒 Files selected for processing (18)
CMake/Modules/FindNF_NativeAssemblies.cmakeCMake/Modules/FindnanoFramework.Hardware.Rpi.cmakeCMake/Modules/NF_Kconfig.cmakeKconfig.apistargets/ChibiOS/RP_PICO2_RP2350/defconfigtargets/ChibiOS/RP_PICO_RP2040/defconfigtargets/ChibiOS/RP_PICO_W_RP2040/defconfigtargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Rpi/nf_hardware_rpi_native.cpptargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Rpi/nf_hardware_rpi_native.htargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Rpi/nf_hardware_rpi_native_PioIrqDriver.cpptargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Rpi/nf_hardware_rpi_native_PioIrqHandlers.ctargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Rpi/nf_hardware_rpi_native_nanoFramework_Hardware_Rpi_Pio_PioBlock.cpptargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Rpi/nf_hardware_rpi_native_nanoFramework_Hardware_Rpi_Pio_PioBlock.htargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Rpi/nf_hardware_rpi_native_nanoFramework_Hardware_Rpi_Pio_PioBlock_mshl.cpptargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Rpi/nf_hardware_rpi_native_nanoFramework_Hardware_Rpi_Pio_PioStateMachine.cpptargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Rpi/nf_hardware_rpi_native_nanoFramework_Hardware_Rpi_Pio_PioStateMachine.htargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Rpi/nf_hardware_rpi_native_nanoFramework_Hardware_Rpi_Pio_PioStateMachine_mshl.cpptargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Rpi/nf_hardware_rpi_native_target.h
josesimoes
requested changes
Jun 26, 2026
josesimoes
left a comment
josesimoes
left a comment
Member
There was a problem hiding this comment.
Pretty neat! A few changes required, mostly with license headers and the fact that this is a core library.
Author
|
Thanks for the review! I really appreciate the feedback! It's pretty late over here, so I'll go through all the comments, apply the requested formatting changes, and push the updates first thing tomorrow morning 😃 |
josesimoes
reviewed
Jun 26, 2026
Open
3 tasks
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_nanoFramework_Hardware_Pico_Pio_PioStateMachine.cpp (2)
63-65: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate
offsetbefore programming the state-machine PC.
offsetis only masked at Line 120, so-1from a failedNativeAddProgrambecomes PC31instead of failing. Reject offsets outside0..31.Suggested fix
- if (pio == nullptr || sm < 0 || sm > 3 || blobArray == nullptr || + if (pio == nullptr || sm < 0 || sm > 3 || offset < 0 || offset > 31 || blobArray == nullptr || (int)blobArray->m_numOfElements < PIO_CFG_BLOB_LENGTH)Also applies to: 119-120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_nanoFramework_Hardware_Pico_Pio_PioStateMachine.cpp` around lines 63 - 65, The Pio state-machine setup in nanoFramework_Hardware_Pico_Pio_PioStateMachine is accepting an invalid program offset, so add validation before programming the PC to reject any offset outside 0..31 instead of relying on the later mask. Check the offset produced by NativeAddProgram and, if it is negative or greater than 31, fail the call before writing to the state machine registers; keep the existing blobArray and sm validation intact.
285-289: 🩺 Stability & Availability | 🔴 CriticalAddress potential signed integer overflow in PIN range validation
The expression
basePin + countcan overflow for large positiveintvalues, causing undefined behavior before the bounds check is applied. SincebasePinandcountoriginate from managed code (s4signed integers), they can reach values that exceedINT_MAXwhen summed, even if individually they seem small enough. The fix avoids addition by checking ifcountexceeds the remaining capacity (maxPins - basePin), which is safe becausebasePinis validated to be non-negative.Suggested fix
PIO_TypeDef *pio = PioFromIndex(block); `#if` defined(RP2350) - if (pio == nullptr || sm < 0 || sm > 3 || basePin < 0 || count < 0 || basePin + count > 48) + const int maxPins = 48; `#else` - if (pio == nullptr || sm < 0 || sm > 3 || basePin < 0 || count < 0 || basePin + count > 30) + const int maxPins = 30; `#endif` + if (pio == nullptr || sm < 0 || sm > 3 || basePin < 0 || count < 0 || basePin > maxPins || + count > maxPins - basePin)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_nanoFramework_Hardware_Pico_Pio_PioStateMachine.cpp` around lines 285 - 289, The PIN range guard in the PIO setup path can overflow because it evaluates basePin + count before validating the upper bound. Update the validation near PioFromIndex so it avoids direct addition and instead checks count against the remaining pin capacity after basePin has been confirmed non-negative, using the correct maximum for RP2350 vs other targets. Keep the existing sm and basePin sanity checks, but replace the overflow-prone range test in this native PIO state machine code with a safe bound check.targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_nanoFramework_Hardware_Pico_Pio_PioBlock.cpp (3)
137-143: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject removes for slots that were never allocated.
NativeRemoveProgramclears instruction memory andg_PioInstrUsedfor any legal range, even when the range was not fully allocated. That lets managed callers corrupt the native allocator state and erase unrelated PIO instructions.Suggested fix
+ unsigned int slotMask = PioSlotMask(offset, length); + if ((g_PioInstrUsed[block] & slotMask) != slotMask) + { + NANOCLR_SET_AND_LEAVE(CLR_E_INVALID_OPERATION); + } + // reclaim slots, blank to JMP-to-self so a stray enable can't run stale opcodes for (int i = 0; i < length; i++) { pio->INSTR_MEM[offset + i] = (unsigned short)((offset + i) & 0x1F); } - g_PioInstrUsed[block] &= ~PioSlotMask(offset, length); + g_PioInstrUsed[block] &= ~slotMask;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_nanoFramework_Hardware_Pico_Pio_PioBlock.cpp` around lines 137 - 143, NativeRemoveProgram is allowing any legal range to be reclaimed even if those slots were never allocated, which can corrupt g_PioInstrUsed and overwrite unrelated PIO instructions. Update NativeRemoveProgram to verify the requested range is fully allocated in g_PioInstrUsed before touching pio->INSTR_MEM or clearing the mask, and reject the removal if any slot in the range is not currently owned. Use the existing PioSlotMask helper and the g_PioInstrUsed/block bookkeeping in PioBlock to validate ownership before reclaiming slots.
130-131: 🩺 Stability & Availability | 🔴 CriticalValidate the removal range without overflowing.
Line 131 risks integer overflow on
offset + lengthwhenoffsetis a large positiveintandlengthis positive (e.g.,offset = INT_MAX,length = 1). The sum wraps to negative, bypassing the> 32validation.Use a subtractive bound check to prevent overflow:
- if (pio == nullptr || offset < 0 || length <= 0 || offset + length > 32) + if (pio == nullptr || offset < 0 || length <= 0 || offset > 32 - length)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_nanoFramework_Hardware_Pico_Pio_PioBlock.cpp` around lines 130 - 131, The range validation in PioBlock is vulnerable to overflow because offset + length can wrap before the > 32 check. Update the validation in the PioBlock native method to use a subtractive bounds check instead of summing offset and length, while still rejecting null pio, negative offset, and nonpositive length. Keep the fix localized to the PioBlock range check so the removal bounds are enforced safely.
80-82: 🩺 Stability & Availability | 🔴 CriticalFix signed integer overflow risk in fixed-origin range validation.
The check
origin + length <= 32at line 82 invokes undefined behavior iforiginis a large positiveint(nearINT_MAX), as the addition can overflow before the comparison. This allows invalid inputs to bypass the range check or produce unpredictable results.Use the overflow-safe form
origin <= 32 - lengthinstead, which enforces the same constraint without performing the risky addition.Suggested fix
- if (origin + length <= 32 && (g_PioInstrUsed[block] & PioSlotMask(origin, length)) == 0) + if (origin <= 32 - length && (g_PioInstrUsed[block] & PioSlotMask(origin, length)) == 0)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_nanoFramework_Hardware_Pico_Pio_PioBlock.cpp` around lines 80 - 82, The fixed-origin validation in PioBlock has a signed overflow risk because the range check currently adds origin and length before comparing to 32. Update the condition in the PioBlock slot-allocation logic to use the overflow-safe form origin <= 32 - length instead of origin + length <= 32, while keeping the existing g_PioInstrUsed and PioSlotMask checks unchanged. This change should be made in the same validation branch that handles origin >= 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_nanoFramework_Hardware_Pico_Pio_PioBlock.cpp`:
- Around line 137-143: NativeRemoveProgram is allowing any legal range to be
reclaimed even if those slots were never allocated, which can corrupt
g_PioInstrUsed and overwrite unrelated PIO instructions. Update
NativeRemoveProgram to verify the requested range is fully allocated in
g_PioInstrUsed before touching pio->INSTR_MEM or clearing the mask, and reject
the removal if any slot in the range is not currently owned. Use the existing
PioSlotMask helper and the g_PioInstrUsed/block bookkeeping in PioBlock to
validate ownership before reclaiming slots.
- Around line 130-131: The range validation in PioBlock is vulnerable to
overflow because offset + length can wrap before the > 32 check. Update the
validation in the PioBlock native method to use a subtractive bounds check
instead of summing offset and length, while still rejecting null pio, negative
offset, and nonpositive length. Keep the fix localized to the PioBlock range
check so the removal bounds are enforced safely.
- Around line 80-82: The fixed-origin validation in PioBlock has a signed
overflow risk because the range check currently adds origin and length before
comparing to 32. Update the condition in the PioBlock slot-allocation logic to
use the overflow-safe form origin <= 32 - length instead of origin + length <=
32, while keeping the existing g_PioInstrUsed and PioSlotMask checks unchanged.
This change should be made in the same validation branch that handles origin >=
0.
In
`@targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_nanoFramework_Hardware_Pico_Pio_PioStateMachine.cpp`:
- Around line 63-65: The Pio state-machine setup in
nanoFramework_Hardware_Pico_Pio_PioStateMachine is accepting an invalid program
offset, so add validation before programming the PC to reject any offset outside
0..31 instead of relying on the later mask. Check the offset produced by
NativeAddProgram and, if it is negative or greater than 31, fail the call before
writing to the state machine registers; keep the existing blobArray and sm
validation intact.
- Around line 285-289: The PIN range guard in the PIO setup path can overflow
because it evaluates basePin + count before validating the upper bound. Update
the validation near PioFromIndex so it avoids direct addition and instead checks
count against the remaining pin capacity after basePin has been confirmed
non-negative, using the correct maximum for RP2350 vs other targets. Keep the
existing sm and basePin sanity checks, but replace the overflow-prone range test
in this native PIO state machine code with a safe bound check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: f0aa24d8-dc8a-4194-9f5f-35eae4106529
📒 Files selected for processing (14)
CMake/Modules/FindNF_NativeAssemblies.cmakeCMake/Modules/FindnanoFramework.Hardware.Pico.cmakeCMake/Modules/NF_Kconfig.cmakeKconfig.apistargets/ChibiOS/RP_PICO2_RP2350/defconfigtargets/ChibiOS/RP_PICO_RP2040/defconfigtargets/ChibiOS/RP_PICO_W_RP2040/defconfigtargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native.cpptargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native.htargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_PioIrqDriver.cpptargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_PioIrqHandlers.ctargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_nanoFramework_Hardware_Pico_Pio_PioBlock.cpptargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_nanoFramework_Hardware_Pico_Pio_PioStateMachine.cpptargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_target.h
💤 Files with no reviewable changes (1)
- targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_PioIrqHandlers.c
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_nanoFramework_Hardware_Pico_Pio_PioBlock.cpp (1)
136-150: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftTrack allocation boundaries before clearing program slots.
Line 138 only proves every slot is occupied, not that the requested
(offset, length)matches one allocated program. A caller can remove an interior slice or span adjacent programs, and Line 150 then frees live instructions for reuse. Store exact allocation metadata or per-slot allocation IDs and require an exact match before clearing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_nanoFramework_Hardware_Pico_Pio_PioBlock.cpp` around lines 136 - 150, The reclaim logic in PioBlock currently only checks g_PioInstrUsed with PioSlotMask, which confirms occupancy but not that the requested (offset, length) belongs to one exact allocation. Update the allocation tracking in PioBlock to record boundaries or per-slot allocation IDs, then verify an exact match before clearing pio->INSTR_MEM and updating g_PioInstrUsed. Use the existing symbols PioSlotMask, PioEnsureOutOfReset, and g_PioInstrUsed to locate the reclaim path and prevent freeing interior slices or mixed ranges.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_nanoFramework_Hardware_Pico_Pio_PioStateMachine.cpp`:
- Around line 287-292: The validation in PioStateMachine currently allows RP2350
basePin values above 31 when maxPins is 48, but the later SET_BASE encoding in
the same flow only preserves 5 bits and aliases GPIO 32–47 to 0–15. Update the
range check in the PioStateMachine path to validate against the active GPIOBASE
window or reject any basePin/count combination that cannot be represented safely
by the SET_BASE field, using the existing basePin, count, and maxPins logic as
the place to fix it.
---
Outside diff comments:
In
`@targets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_nanoFramework_Hardware_Pico_Pio_PioBlock.cpp`:
- Around line 136-150: The reclaim logic in PioBlock currently only checks
g_PioInstrUsed with PioSlotMask, which confirms occupancy but not that the
requested (offset, length) belongs to one exact allocation. Update the
allocation tracking in PioBlock to record boundaries or per-slot allocation IDs,
then verify an exact match before clearing pio->INSTR_MEM and updating
g_PioInstrUsed. Use the existing symbols PioSlotMask, PioEnsureOutOfReset, and
g_PioInstrUsed to locate the reclaim path and prevent freeing interior slices or
mixed ranges.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 727554ec-10e1-4f7a-aa26-b27b05aeba32
📒 Files selected for processing (4)
targets/ChibiOS/RP_PICO2_RP2350/README.mdtargets/ChibiOS/RP_PICO_RP2040/README.mdtargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_nanoFramework_Hardware_Pico_Pio_PioBlock.cpptargets/ChibiOS/_nanoCLR/nanoFramework.Hardware.Pico/nanoFramework_hardware_pico_native_nanoFramework_Hardware_Pico_Pio_PioStateMachine.cpp
josesimoes
reviewed
Jul 1, 2026
| work->Buffer = buf; | ||
| work->Count = (unsigned int)count; | ||
|
|
||
| Events_Get(SYSTEM_EVENT_FLAG_PICOPIO); |
Member
There was a problem hiding this comment.
No need to get this event here...
Or is there a good reason for calling this API?
Author
There was a problem hiding this comment.
The Events_Get here drains any stale SYSTEM_EVENT_FLAG_PICOPIO before entering the WaitEvents loop — without it, a leftover flag from a previous DMA could cause a spurious wakeup. It follows the same pattern used in the SerialPort drivers (e.g. Events_Get(SYSTEM_EVENT_FLAG_COM_IN) with the comment // clear event by getting it).
Happy to change the approach if there's a preferred alternative though! 😄
| { | ||
| ch->channel->CTRL_TRIG &= ~DMA_CTRL_TRIG_EN; | ||
| DMA->CHAN_ABORT = (1u << ch->chnidx); | ||
| while (DMA->CHAN_ABORT & (1u << ch->chnidx)) |
Member
There was a problem hiding this comment.
I really don't like these infinite loops...
You'll tell me that this will always exit because the DMA will always abort the operation. Until the day it doesn't because of a reason and then we'll be stuck here until the watchdog kicks in...
Is this check really needed? or you're just being over thorough?
Author
There was a problem hiding this comment.
You are absolutely right, and I completely agree. Raw infinite while loops waiting for hardware registers are a recipe for watchdog resets if the AHB bus or silicon ever glitches.
The reason that check is there is because both datasheets explicitly mandate it. According to Table 149 (RP2040 Datasheet) and Table 1182 (RP2350 Datasheet) under the CHAN_ABORT register description:
"After writing, this register must be polled until it returns all-zero. Until this point, it is unsafe to restart the channel."
If we just abort and immediately proceed to platform_free(work->Buffer) without waiting for those in-flight transfers to flush, the DMA might write into freed memory, causing catastrophic heap corruption.
However, your point stands: we shouldn't trust the hardware infinitely. I will update both NativeRead and NativeWrite to use a guard counter (like PIO_FIFO_WAIT_LIMIT) for the abort loop, so if the DMA controller ever hangs, we gracefully exit instead of locking the thread.
Thanks for catching this! I'll push the fix shortly.
Member
|
@begeistert we have changed the directory and target names of the PICO to simplify things. |
begeistert
force-pushed
the
feat/rp2040-pio-hardware
branch
from
July 23, 2026 08:36
c907d24 to
2b747db
Compare
Author
|
Hi @Ellerbach! Apologies for the delay in getting back to you. I went down a rabbit hole optimizing my emulators so I could validate the PIO behavior faster locally. It worked out perfectly in the end, but it took me off the radar for a bit! (Fun side effect: I actually ended up getting nanoFramework + PIO compiling and running entirely in the browser via WebAssembly to test this! Here is a sneak peek of what I was building while missing running the nanoFramework uf2 with PIO support). 
Great news on the RP2350 WiFi merge! I appreciate the offer to assist with the rebase. I think I did it correctly and pushed the updates, but I really hope I got the new folder structure right! If something looks off or if I broke anything, I will definitely take you up on that offer and ask for your help. I am back on track now and actively working on @josesimoes's requested changes. Will update the PR in the next few days. Thanks! |
Author
|
Just a quick heads-up: I did a force push to update the branch after the rebase. I really hope this isn't considered a bad practice in this repository! To be completely honest, I didn't read the contributing guidelines thoroughly beforehand, but I am going to read them right now. Apologies in advance if I messed up the expected workflow! |
Member
|
No problem at all. |
Author
No problem @Ellerbach! I will rebase the times needed to have the changes in order And I just want to confirm, I am not absent, I've been testing the changes, and thinking on an answer for @josesimoes questions hehehe |
Introduce an `abortGuard` in DMA channel abort loops to prevent potential hangs if the channel fails to clear the abort flag. Update the PIO IRQ driver using modern C++ features like C++17 `if` initializers, `static_cast`, `const`, `constexpr`, and designated initializers for improved code style and type safety.
These source files and their native assembly registration are no longer required following the refactoring of PIO native methods and the implementation of managed IRQ eventing.
The callback's `flags` parameter includes all IRQ sources. Extracting only the state machine IRQ flags (IRQ0_INTS) ensures that managed events receive relevant information and that only the intended hardware IRQ bits are cleared, aligning with documentation.
…framework.Hardware.Pico
Converts PIO native methods from static utility functions to instance methods on the PioBlock object. This improves the object-oriented design by associating operations directly with a PioBlock instance. Additionally, the Add/Remove Program methods now directly accept a PioProgram object, simplifying their usage. The native assembly GUID is updated as a result of these API changes.
Converts `PioStateMachine` native methods from static functions, requiring explicit block and state machine indices, to instance methods. The block and state machine indices are now implicitly retrieved from the `PioStateMachine` object instance, simplifying the API and improving object-oriented design. The native assembly GUID is updated due to these API changes.
Integrates the ChibiOS PIOv1 Low-Level Driver (LLD) into the nanoCLR build system for RP2040 and RP2350 devices. This enables PIO hardware support by adding required source files, include paths, and the `RP_PIO_REQUIRED` compilation definition. Refines the native `PioBlock.AddProgram` method to improve robustness when handling `PioProgram` objects, ensuring correct access to program instructions and origin. Updates the `PIO_MIN_BLOCK` definition logic to depend on `TARGET_HAS_WIFI`, reserving PIO block 0 only when WiFi is present on the target.
Replaces manual DMA channel control register manipulation and abort logic with a call to the helper function `dmaChannelDisableX()`. This improves code readability and ensures a consistent approach to stopping DMA transfers in the PIO state machine.
The binding drove the RP2040/RP2350 registers by hand even though the PIOv1 and DMAv1 LLDs already expose the same operations with names, and the hand-written paths had accumulated real defects. It now calls the LLD for everything delicate -- the DMA abort sequence with the RP2040-E13 / RP2350-E5 workarounds, the state machine init order, the SET PINDIRS chunking, JMP relocation on program load -- and keeps only what belongs to this layer: turning a CLR_RT_StackFrame into a validated state machine handle, CLR event handling, and argument checks. PioStateMachine.cpp goes from 1102 to ~800 lines, and no raw register access is left outside the handful of documented LLD gaps in _target.h. Defects fixed along the way, verified in execution on the emulator unless noted: - the tree did not build: pioGetSmHandle() does not exist upstream. The handle now comes from g_AllocatedSMs, which also distinguishes claimed from released. - Read/Write leaked their DMA channel on any error exit: the eval stack deadline and the transfer are now released in the cleanup, guarded on CLR_E_THREAD_WAITING so a parked thread still resumes. A leak left the slot busy forever and every later transfer failed. - the transferred count was read AFTER the abort, which zeroes TRANS_COUNT, so a timed out transfer reported the full count and Read copied never-written bounce buffer. Measured: a 16 word Write into a stalled state machine now reports 4, the FIFO depth. - DrainTxFifo emitted PUSH (0x8000) where it meant PULL (0x8080), so without autopull it never drained and pushed the ISR into RX instead. Measured: TX goes 2 -> 0 words, RX stays at 0. - the PIO interrupt callback posted one managed event per claimed state machine, because ChibiOS broadcasts a block's interrupt to all of them. It is now idempotent on the live IRQ flags. - NativeUnclaim freed the state machine while a transfer was still writing into a bounce buffer it was about to release. - CTRL was driven with read-modify-write, which can lose a concurrent enable; the atomic SET/CLR aliases are used instead. - SetConsecutivePinDirs did not clear OUT_STICKY around the exec'd writes. - InitGpio only set FUNCSEL, leaving the pad untouched and, on the RP2350, the isolation latch set. - DMA error flags handed to the callback were discarded. - unbounded busy-wait in the blocking FIFO accessors. The teardown native tolerates an already-disposed instance, matching NativeDispose in the I2C binding: a prologue that rejects disposed objects cannot guard the one call whose whole purpose is to run at disposal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017y2cw98Pjsa3eMgsAHdYEu
The rest of the firmware stays at -Og, which makes gdb report "value has been optimized out" for locals in these files. Scoped to this folder to keep the image size unchanged elsewhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017y2cw98Pjsa3eMgsAHdYEu
…hines by index Put and Get held the CLR for up to a second. The native side waited with PioSpinWhile, which sleeps on the RTOS but never yields to the CLR scheduler, so every other managed thread stopped for the whole budget. They now follow the same re-entrant pattern the DMA transfers already use: arm the state machine's TXNFULL or RXNEMPTY interrupt, hand the thread back through WaitEvents with CLR_E_THREAD_WAITING, and finish the transfer when the interrupt brings it back. The block callback grew the FIFO half of the dispatch: it masks the asserted bits, which are level triggered and would otherwise storm, and posts the event. Read and Write are unchanged; the shared direction enum is renamed to PioFifoDir now that it describes both paths. Measured on the emulator with a probe that slows the state machine to about 2 kHz so the echo cannot be ready when Get looks: the callback fires, the call resumes, and a background thread ticks 25 times while Main sits in a Put that never completes -- with the spin it could not have ticked once. The same Put still gives up after the one second budget. Put, TryPut and TryGet read their first argument from Arg2, where an instance method keeps its second. The value written into the FIFO was whatever happened to sit in that slot, so a loopback round trip came back wrong; TryGet dereferenced the wrong slot for its byref. Left over from the move to instance members, when the block and state machine arguments went away and the indices were not shifted down. NativeClaimUnusedSm becomes NativeClaimSm, an instance method that takes the wanted index and reads the block from the object. Passing RP_PIO_SM_ID_ANY asks for whichever is free, so one native covers both pio_claim_unused_sm and pio_sm_claim, and the managed side no longer threads _index through every call. Anything that is neither ANY nor 0..3 is rejected before it reaches the allocator. Pio gains three statics that report what the firmware was built for -- the first block application code may use, how many exist, and the highest GPIO a block can drive -- so managed code stops hardcoding indices that differ between the RP2040, the RP2350 and the boards that reserve PIO0 for the radio.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist