Skip to content

NeoPixel does not work on RP2350B with GPIO pin >=32 #16190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sfe-SparkFro opened this issue Nov 8, 2024 · 8 comments · Fixed by #16191
Closed

NeoPixel does not work on RP2350B with GPIO pin >=32 #16190

sfe-SparkFro opened this issue Nov 8, 2024 · 8 comments · Fixed by #16191
Labels

Comments

@sfe-SparkFro
Copy link
Contributor

Port, board and/or hardware

rp2 port with RP2350B

MicroPython version

v1.24.0-preview.461.gb8227a3f7.dirty

Reproduction

import neopixel
import machine

pin = 32

p = machine.Pin(pin)
n = neopixel.NeoPixel(p, 1)

n[0] = (127, 0, 0)
n.write()

Expected behaviour

A NeoPixel connected to pin should illuminate.

Observed behaviour

A NeoPixel connected to pin only illuminates if pin is <32. If pin is >=32, the NeoPixel does not illuminate.

Additional Information

The root problem appears to be here:

static inline void mp_hal_pin_low(mp_hal_pin_obj_t pin) {
gpio_clr_mask(1 << pin);
}
static inline void mp_hal_pin_high(mp_hal_pin_obj_t pin) {
gpio_set_mask(1 << pin);
}

gpio_set_mask() requires a 32-bit value, so the shifted bit gets truncated when using a pin >= 32. There is a gpio_set_mask64(), however for some reason it didn't work when I tried it (I didn't dig into it). However changing to gpio_put() works fine on my end.

Code of Conduct

Yes, I agree

@robert-hh
Copy link
Contributor

Looks like mp_hal_pin_low() and mp_hal_pin_high() in rp2/mphalport.h have to be fixed to use gpio_set_mask64() and gpio_clr_mask64() instead of the 32bit versions. Someone with a RP2350B could implement and test it. It may have an impact on Neopixel timing.

@sfe-SparkFro
Copy link
Contributor Author

Turns out the reason the 64-bit versions didn't work is because you need to explicitly typecast to a uint64_t. Will update my PR, since I think the mask functions are faster than gpio_put() (though I didn't see any timing issues).

@robert-hh
Copy link
Contributor

robert-hh commented Nov 8, 2024

though I didn't see any timing issues

I will compare it to the previous state tomorrow. I still have the old test data from rp2040. The difference should be small, like a few clock cycles per output phase.
Edit: Looking into the code, lacking an RP2350B the test make no sense.

@sfe-SparkFro
Copy link
Contributor Author

Actually, would it even matter if it takes longer? The bitstream implementation (which the NeoPixel driver uses) just waits until it's time to toggle the pin state:

for (size_t j = 0; j < 8; ++j) {
uint32_t *t = &timing_ns[b >> 6 & 2];
uint32_t start_ticks = systick_hw->cvr = SYSTICK_MAX;
mp_hal_pin_high(pin);
while ((start_ticks - systick_hw->cvr) < t[0]) {
}
b <<= 1;
mp_hal_pin_low(pin);
while ((start_ticks - systick_hw->cvr) < t[1]) {
}
}

So as long as setting the pin high/low doesn't take longer than the time between pulses (shortest is 400ns), I don't think it matters if this change results in a couple extra clock cycles.

@robert-hh
Copy link
Contributor

Probably it will not matter. Especially not for neopixel, which has quite relaxed timing requirements.

@robert-hh
Copy link
Contributor

robert-hh commented Nov 9, 2024

Testing gives sometimes unexpected results. I tested the bitstream timing with a unmodified RP2350A and the differ from the rp2040. So it needs a small adjustment. I made a change to the compensation value as of:

#if PICO_RP2350
#define MP_HAL_BITSTREAM_NS_OVERHEAD  (5)
#else
#define MP_HAL_BITSTREAM_NS_OVERHEAD  (9)
#endif

Maybe you could include this change into your PR, making it a single commit for about the same topic.

All timings made with a bit tuple of (1000, 1000, 1000, 1000) and \x55\x55 as pattern.
Timing with 9ns compensation at 125MHz clock:
bitstream_p2_125M

Timing with 5ns compensation at 125MHz clock:
bitstream_p2_125M_5ns

Timing with 5 ns compensation at 150MHz clock (default for RP2350):
bitstream_p2_150M_5ns

For comparison a figure for RP2040:
Timing with 9ms compensation at 125MHz:

bitstream_125M_9ns

@robert-hh
Copy link
Contributor

For comparison the timing for RP2350, RISCV. The 150Mhz figures are good, the ones for 125MHz and 250Mhz are worse. They would need a larger compensation value. The value for 250 Mhz is surprising. Usually the timing got better at higher frequencies.

150MHz, 5ns, compensation:
bitstream_RISCV_150M_5ns

125MHz, 5 ns compensation:
bitstream_RISCV_125M_5ns

@sfe-SparkFro
Copy link
Contributor Author

PR updated with your suggested timings!

Gadgetoid pushed a commit to pimoroni/micropython that referenced this issue Nov 26, 2024
Gadgetoid pushed a commit to pimoroni/micropython that referenced this issue Dec 5, 2024
Gadgetoid pushed a commit to pimoroni/micropython that referenced this issue Dec 5, 2024
Gadgetoid pushed a commit to pimoroni/micropython that referenced this issue Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants