Conversation
tannewt
left a comment
There was a problem hiding this comment.
You'll need to rebase onto my bootloader stuff. Sorry! (It's a good example of another Zephyr project that would benefit from a separate neopixel module.)
4463fd6 to
b5d2d13
Compare
I rebased. The port Kconfig default changes (see the original post) were moved into the new |
|
@tannewt: A design question on nRF54L: All three PWM instances there ( We could fall back to bit-bang, with bit-bang's limitations that interrupts are locked for the frame, and the NeoPixel pattern is resent when the BLE radio preempts it. Should we silently fall back to bit-bang for those pins? (Claude with rewrite by @dhalbert) |
Please make your own decision. I don't need to make it. You have all of the context I have. |
Transmit with a PWM instance's EasyDMA sequence playback, as ports/nordic does. The instance is allocated from iobroker for the duration of each write and released afterwards (new iobroker_pwm_allocate() and a pwm pool in the generated board tables) so that pwmio can share the instances later. iobroker gains a register-address column in the instance tables and iobroker_instance_reg_addr() for callers that drive an allocated instance directly, and iobroker_package_pin_soc_pad() becomes public. The module is enabled where CONFIG_PWM_NRFX=y. When every PWM instance is busy, fall back to a DWT-timed bit-bang with interrupts locked, as ports/nordic does; a bit that runs more than twice its nominal length marks the frame as preempted and it is resent, up to 8 attempts. Two port Kconfig defaults for nRF go with it and also protect the port's other critical sections: ZERO_LATENCY_IRQS so the BLE controller's radio ISR bypasses irq_lock(), and BT_CTLR_ASSERT_OVERHEAD_START=n so a radio event delayed past the controller's prepare budget is skipped instead of halting the system. Status NeoPixel: STATUS_NEOPIXEL and STATUS_NEOPIXEL_POWER keys in circuitpython.toml name [pins] entries and emit MICROPY_HW_NEOPIXEL and CIRCUITPY_STATUS_LED_POWER into the generated mpconfigboard.h. Enabled on Feather nRF52840 Express, Feather Bluefruit Sense and CLUE. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Review follow-up. The nRF PWM sequence playback and bit-bang fallback move from common-hal/neopixel_write/nrf.c into internal-modules/neopixel, a Zephyr module with a CircuitPython-free API (NEOPIXEL_PATTERN_BUFFER_SIZE(), neopixel_send()) so other Zephyr projects can share it. common-hal keeps the latch guard and the stack-or-heap pattern buffer choice. iobroker gains iobroker_pwm_allocate_unrouted(pin, ...), which takes the pin in and claims only the instance for a caller that owns the pin and drives the registers itself; the register-address column and iobroker_instance_reg_addr() go away, the module keeps its own devicetree-derived register table instead. The bit-bang cites Zephyr's fast GPIO RFC (zephyrproject-rtos/zephyr#106831) as its eventual replacement. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
On nRF54L, peripherals and GPIO controllers are grouped in power domains and a peripheral can only drive pads of its own domain; the domain is encoded in the register addresses. Carry those addresses in iobroker's instance and GPIO controller tables (generated from the devicetree) and check them in the nRF routing code, so that an allocate call whose pins no instance can reach fails with -ENXIO instead of routing to a pad the peripheral cannot drive. busio maps -ENXIO to the invalid-pins ValueError; neopixel_write already raises the invalid-pin one. Enable PWM instances 21 and 22 for runtime allocation on the nRF54LM20 DK (instance 20 stays the board's PWM LED), which turns neopixel_write on there. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
internal-modules/neopixel/zephyr/module.yml was never committed: the port's .gitignore ignores any path named zephyr (the west workspace), which also swallows the in-tree modules' descriptor directories. Without it Zephyr rejects the module as invalid, which failed every zephyr-cp CI build. Re-include internal-modules/*/zephyr/ so this cannot recur. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The nRF54L reachability commit emitted DT_REG_ADDR() for every GPIO controller and every allocatable instance, which does not compile for nodes without a reg property: native_sim's gpio_emul and the Pico W's PIO-based SPI. Emit 0 for those; the address is only consumed by the nRF54L power-domain check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On nRF54L, PWM20-22 drive only P1 and P3. Fall back to bit-banging for P2 pads too, but not for P0: its GPIO is in the low-power domain, where a CPU register write stalls about 65 cycles at 128 MHz (measured on the nRF54LM20A), longer than a zero bit's high time. P0 pads still raise ValueError. Verified on the nRF54LM20 DK with a 16-pixel RGBW ring on PORT2_08 (bit-bang), PORT3_00 (PWM and forced bit-bang) and PORT0_02 (ValueError). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Enable PWM instances 21 and 22 as deferred-init instances with disconnected outputs, as on the nRF54LM20 DK; instance 20 stays the board's PWM LED. Verified with a 16-pixel RGBW ring (DK VDD set to 3.3 V) on P1_11 (PWM) and P2_08 (bit-bang); P0_02 raises ValueError. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…uted neopixel_write calls iobroker_pwm_allocate_unrouted() on every write and bit-bangs when no PWM instance can reach the pad, so on nRF54L P2 pads the warning flooded the console. That is an expected case, so log it at debug level. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Enable PWM instances 1-3 as deferred-init instances with disconnected outputs; instance 0 stays the board's PWM LED. Build-tested only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
8fb92de to
1783066
Compare
|
I thought I needed #11484 for this, but it's really independent. It's just that it was harder to test NeoPixels without functioning REPL on nRF54LM20. |
|
@tannewt this is now ready for review. |
tannewt
left a comment
There was a problem hiding this comment.
Looking better! Just a few more questions.
Conflicts with adafruit#11481 (analogio), resolved by keeping both sides: - Feather nRF52840, Feather Sense and CLUE board.conf: CONFIG_PWM=y and CONFIG_ADC=y. - zephyr2cp.py: the pwm and adc/dac driver branches; the status NeoPixel resolution on top of the new pad-based pin naming. - iobroker_internal.h: main's declarations. adafruit#11481 also renumbered soc_pad from the GPIO number to the package's pin id; the next commit adapts this branch's code to that. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…11481 adafruit#11481 changed iobroker's soc_pad from the global GPIO number to the package's pin id, with the GPIO number now in gpio_pad, and removed nrf_pad_ok(). PSEL, the bit-bang GPIO writes and the nRF54L domain check all need the GPIO number: - Replace the public iobroker_package_pin_soc_pad() with iobroker_package_pin_gpio_pad(), which resolves a package pin to its GPIO number. iobroker_package_pin_soc_pad() is internal again, as on main. - iobroker_pwm_allocate() and iobroker_pwm_allocate_unrouted() resolve through it, like the I2C, SPI and UART allocate functions now do. - neopixel_send() uses it for its pad. Verified on the nRF54LM20 DK: a 16-pixel RGBW ring animates on PORT1_10 and PORT3_00 (PWM) and PORT2_08 (bit-bang); PORT0_02 raises ValueError. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An allocation whose pins no instance can reach is a property of the request, like a pin that is not in the package map, so return -EINVAL for it, as adafruit#11481's analog allocate does for a pin with no analog function. busio now maps -EINVAL from allocate to "Invalid pins"; before, it fell through to "Use device tree to define ... devices". neopixel_send() also returns -EINVAL for a pad nothing can drive, and neopixel_write raises "Invalid pin" for it. Verified on the nRF54LM20 DK: busio.SPI on P0 pads and busio.UART on P1 pads raise ValueError: Invalid pins, the reverse cases construct; neopixel_write raises ValueError: Invalid pin on PORT0_02 and bit-bangs on PORT2_08. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Per review, neopixel_write no longer drives a PWM instance's registers under a pin that the caller still holds as a GPIO. Instead: - neopixel_write releases the DigitalInOut's GPIO claim, calls neopixel_send(), then claims the pin again and restores the DigitalInOut's configuration. - neopixel_send() allocates a PWM instance routed to the pin with the new single-pin iobroker_pwm_allocate(), initializes it (its pinctrl state connects OUT0), plays the sequence and releases it. For the bit-bang fallback it allocates the pin as a GPIO itself. - iobroker_pwm_allocate(pin, &dev) claims a whole instance and routes the pin to OUT0. It replaces the four-output version, which had no caller, and iobroker_pwm_allocate_unrouted(). Re-routing an instance after its first use needs a pwm_nrfx deinit hook, which Zephyr does not have yet; without it, every instance can be used once and later writes fall back to bit-banging. Verified with a local pwm_nrfx deinit hook, using steady images rewritten about 100 times a second: no glitches on the nRF54LM20 DK's PORT1_10 and PORT3_00 (PWM) with BLE on or off, nor on PORT2_08 (bit-bang) with BLE off; PORT0_02 still raises ValueError. A 16-pixel ring on the Feather nRF52840's A0 shows no glitches either. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@tannewt Your review and merge of adafruit/zephyr#4 is now a prerequisite. The When that merges I'll change the pinned zephyr commit, and then ask for a re-review here. The other changes have been pushed, but won't work perfectly until the zephyr change. |
tannewt
left a comment
There was a problem hiding this comment.
Getting close! A couple more questions around the new IOBroker function.
| package_pin_t *package_pin_out); | ||
|
|
||
| // Resolve a package pin to the global GPIO number of the pad it is bonded to | ||
| // (GPIO controller port index * 32 + pin within the port), for a caller that |
|
|
||
| int neopixel_send(package_pin_t pin, const uint8_t *pixels, size_t num_bytes, | ||
| void *pattern_buffer, size_t pattern_buffer_size) { | ||
| // iobroker's global GPIO number (port * 32 + pin) is also the nrfx pin |
There was a problem hiding this comment.
This feels too convenient and therefore not general enough. Why not just a gpio allocate again for bitbang? How will that change with the "fast gpio" PR I linked to.
Claude wrote most of the code, with @dhalbert steering and testing. @dhalbert revised this post extensively for clarity.
neopixel_writefor the zephyr-cp port on Nordic.zephyr,deferred-init. Zephyr's PWM driver is compiled but not currently initialized on them.pwmiocan share the same instances later.internal-modules/neopixel, with a CircuitPython-free API, so other Zephyr projects can share it.common-hal/neopixel_writekeeps only the latch-period guard and the stack-or-heap pattern buffer choice.iobroker_pwm_allocate()and apwmpool in the generated board tables.iobroker_pwm_allocate_unrouted(), for a caller that owns the pin and drives the instance's registers itself. It names the pin but claims only the instance.iobroker_package_pin_soc_pad()public so the neopixel module can use it.-ENXIO.busiothen raisesValueError: Invalid pins, instead of silently producing no output.STATUS_NEOPIXELandSTATUS_NEOPIXEL_POWERkeys incircuitpython.toml. The keys name entries in the board's existing[pins]table.MICROPY_HW_NEOPIXELandCIRCUITPY_STATUS_LED_POWERinto the generatedmpconfigboard.h.CONFIG_PWM_NRFX=y, so other nRF boards are unchanged until their overlays add routable PWM instances.Implementation notes
src/<vendor>/<soc>/, like iobroker's.src/nordic/nrf/neopixel_nrf.cis the only one so far.ports/nordic, the implementation will use a buffer on the stack for pixel strips up to 24 RGB pixels (18 RGBW). Longer strips allocate a heap buffer, which is freed inreset_port().led_stripdrivers were not used. They fix the pin, chain length and byte order in devicetree and send zero for the W channel.Bit-bang fallback
ports/nordicdoes. But doing this carelessly messes up BLE. So set two port Kconfig defaults for nRF. They also protect the port's other critical sections (bitbangio,onewireio,microcontroller.disable_interrupts()).ZERO_LATENCY_IRQSso the controller's radio ISR bypassesirq_lock().BT_CTLR_ASSERT_OVERHEAD_START=nso a radio event delayed past the controller's 733 us prepare time budget is skipped. Without the latter, a 16-pixel bit-banged frame during advertising triggered a fatal assertion.ValueError: Invalid pin.ports/nordic.Testing
PORT1_10: P1, PWM, and forced bit-bang with BLE advertising happening simultaneously: 200 writes, 10 preempted frames, all resentPORT3_00: P3, PWM, and forced bit-bangPORT2_08: P2, bit-bang.PORT0_01andPORT0_02raiseValueError: Invalid pinbusio.SPI/busio.UARTon pads that can't be used now raiseValueError: Invalid pins. The DK's only UART instance thatbusiocan allocate,uart30, is in the P0 domain, so for UART it is the P1 request that is rejected.P1_11(P1, PWM) andP2_08(P2, bit-bang).P0_02raisesValueError: Invalid pin.cptools/testspass, including four new tests for the toml status-pin resolver.Not in this PR