-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
Support for RM2 break out boards #16057
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
base: master
Are you sure you want to change the base?
Conversation
I see one test failure for rp2040 Pico in rp2_dma which implies DMA has go slower. It does appear to be caused by the sleep change which doesn't make a lot of sense. I tried taking an average over a number of runs and get much better results - even if it's a loop of 1?! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #16057 +/- ##
=======================================
Coverage 98.57% 98.57%
=======================================
Files 164 164
Lines 21349 21349
=======================================
Hits 21045 21045
Misses 304 304 ☔ View full report in Codecov by Sentry. |
Code size report:
|
That could be due to flash caching effects, where the first run through code is slower but after that it's faster. |
Maybe. Resetting src before the copy makes it twice as fast. Maybe that puts it in the cache? I don't quite understand why this is faster however...
|
9a47835
to
57472a3
Compare
I've taken the liberty to add the ability to use cyw43 with normal Pico or Pico 2 devices as we're starting to see cyw43 breakout boards appearing https://shop.pimoroni.com/products/rm2-breakout?variant=53492995719547 If you wanted to do this you have to enable it and define the default pins to use...
You can use different pins by configuring them at run time.
|
cc00c33
to
038f261
Compare
Awesome stuff. Will switch our wireless builds over to this and see how it fares! |
This possibly breaks PPP support, though I can't seem to figure out why. Might be a configuration quirk. (Doing some poking and rebasing in some commits from master to see if I can either fix it, or figure out what's awry... have some variations on Update: Went with my gut instinct and reverted 50cfd54, seems to have fixed PPP |
Also seem to be having some trouble with WiFi not working on soft reset. If I run the same code twice I need to press reset on a board or I'll get connection aborted or I'm also having a lot of trouble establishing a connection at all which might be related to my router, or my early-stage development boards, or something else, but I know I'm not the only one encountering strangeness. I can't seem to quantify it any better than that- though- and without a Pico 2 W as a reference it's tricky to eliminate any external factors. |
I've tested this with a Pico 2 and your RM2 breakout. I sometimes have to slow the clock div to 3. I believe this was the command line... import network I'll check what happens after a soft reset. |
Weirdly no amount of messing with the clock divider seems to help me. It will either stubbornly refuse to connect, or - seemingly out of the blue - connect and work fine all night until I run the code again at which point GOTO STUBBORN. I can only conclude it's something up with my router (A Mikrotik L009 I use for 2.4GHz devices) but it feels like it's only got particularly bad recently. I guess I should build this code for the Pico W and see what happens! |
Can't replicate on a Pico W, even if I enable dynamic pins. Either specifically a Pico 2 / RP2350 problem, or something up with our batteries-included builds. |
The only problem I have "sometimes" is when I pull the plug on a connected device. It will often take a couple of attempts to reconnect "next time". But it sounds like you have a bigger issue. I can send you a Pico 2 W if you like. Otherwise I'll just spend some time playing around with your RM2 breakout. |
I think our builds were not configuring the reserved pin behaviour, which could explain an awful lot. I've gone back to the proverbial drawing board and rebased my Plasma 2350 W config upon your Pico 2 W config and it seems to work. Will try the same with Pico Plus 2 (W). With your changes there's no reason I should be rolling a W vs non-W build at all so I'm beavering away combining everything (which dramatically simplifies things too, handy) (I'd still love a Pico 2 W for testing, but it's not urgent 😆) I think the issue I mentioned above regarding 50cfd54 breaking PPP is still a problem, but I will re-test once I've finished all my mucking about. Thanks! |
We're still seeing an issue establishing a connection on the first try, which I'm working on narrowing down. Otherwise, things are working much, much better. It was definitely related to MicroPython's soft reset pin cleanup trouncing the comms pin. (Might be fun with a wireless module for Yukon 😆) |
Since it would probably be difficult to support multiple wireless interfaces (though it would be really cool), should the |
I wondered when someone was going to ask for that feature :) I guess it's more of a problem now you have the ability to change the pins. |
I've been trying very hard not to ask! (at least as much because I can't think of a sensible use-case other than intellectual curiosity, but that doesn't mean there isn't one) It's possible, I think?, but potentially a bit of a nightmare refactor, and actually making use of multiple network interfaces is non-trivial. |
Yeah it should work (I've had a wifi and lan connections working at the same time with a wiznet evb). The wifi driver relies on cyw43_state global all over the place - need to fix that somehow. |
Some use cases for multiple network interfaces, fwiw: |
@anecdata try raising an issue in here https://github.com/georgerobotics/cyw43-driver (note: you can already connect Wifi in STA and AP at the same time). |
@peterharperuk can you please rebase this now that #16313 is merged? |
When building allow the port to be configured on the command line by passing a value for CMAKE_ARGS. Signed-off-by: Peter Harper <peter.harper@raspberrypi.com>
Signed-off-by: Peter Harper <peter.harper@raspberrypi.com>
Signed-off-by: Peter Harper <peter.harper@raspberrypi.com>
Signed-off-by: Peter Harper <peter.harper@raspberrypi.com>
Allow cyw43 pins and clock to be configured at runtime. Move cyw43 init code from main. e.g. wlan = network.WLAN(network.STA_IF, pin_on=2, pin_cs=3, pin_dat=4, pin_clock=5, div_int=3) ble = bluetooth.BLE(pin_on=2, pin_cs=3, pin_dat=4, pin_clock=5, div_int=3) ext_led = Pin('WL_GPIO0', Pin.OUT, pin_on=2, pin_cs=3, pin_dat=4, pin_clock=5, div_int=3) Signed-off-by: Peter Harper <peter.harper@raspberrypi.com>
2172c8f
to
c3570b2
Compare
I re-ran the tests on all devices. extmod/machine_i2s_rate.py failed once on pico2/rm2 but I couldn't repeat this. rp2_dma is failing on pico2/rm2 as noted before (I guess I should look info fixing this) but not on pico2_w (bit weird)? I see the thread_stdin issue on riscv as well but failures of select_poll_eintr, stress_schedule and thread_sleep2 when cyw43 is enabled. These don't look like regressions. I think I missed these failures previously. |
I've put the sleep changes over here #16454 |
@@ -34,26 +34,26 @@ ifeq ($(BUILD_VERBOSE),1) | |||
MAKE_ARGS += VERBOSE=1 # Picked up in Makefile generated by CMake | |||
endif | |||
|
|||
CMAKE_ARGS += -DMICROPY_BOARD=$(BOARD) -DMICROPY_BOARD_DIR="$(abspath $(BOARD_DIR))" | |||
override CMAKE_ARGS += -DMICROPY_BOARD=$(BOARD) -DMICROPY_BOARD_DIR="$(abspath $(BOARD_DIR))" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't think it's a good idea to complicate the Makefile by adding override
. Combining make and cmake is already tricky enough.
Why exactly is this needed, what were you trying to achieve with it? Can we do it a different way, eg just call cmake directly in such cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the intent is to pass compiler definitions through to a port, effectively skipping the need to change config files? Possibly to optionally build Pico with RM2 support.
We have variants for this, though there are few examples of that in practise. Here's one for switching to a RISCV build: https://github.com/micropython/micropython/tree/master/ports/rp2/boards/SEEED_XIAO_RP2350
Now that the call to |
I'm glad that the Pico 2 W changes were pulled out to a separate commit (and merged!) because now this PR is a lot cleaner, and it's clearer what it's trying to do (allow dynamic cyw43 config). And I now realize the complexity that's added here to the constructors of WLAN, BLE and Pin. It wasn't clear to me before how deep these changes were, I thought they were only related to the WLAN part. I really don't think the Taking a step back, what's needed here is a better abstraction/model of the hardware. The cyw43 peripheral is not just WLAN, it's also BLE and GPIO. So maybe it needs to be its own entity, and then you obtain the sub-entities from that. Eg: import cyw43
# construct a cyw43 instance (this will persist over soft reset)
cyw = cyw43.CYW43(regon=2, cs=3, data=4, clock=5, div_int=3)
# get the sub-systems from the cyw43 instance
wlan = cyw.WLAN()
ble = cyw.BLE()
led = cyw.Pin(0) The downside with this approach is that you can no longer use Alternatively, when the import cyw43
# construct a cyw43 instance (this will persist over soft reset)
cyw = cyw43.CYW43(regon=2, cs=3, data=4, clock=5, div_int=3)
# access sub-systems as normal
wlan = network.WLAN()
ble = bluetooth.BLE()
led = machine.Pin("LED") That's very similar to the "set cyw43 pins" approach, except that it's a cyw43 object that you create, rather than a function you call (to set the pins). The benefit of this is that you can just put a (Looking at how |
Over in CircuitPython we've run into an issue (adafruit#9777) with the RM2 breakout that may need to be addressed in this issue. The RM2 (and probably the bare CYW43439) appears to automatically select phasing for RM2 to host transfers depending on the gSPI clock speed. Reducing the gSPI clock speed below a certain (unknown) threshold causes "normal mode" to be forced by the RM2. Writing a 1 to gSPI register 0x0000 bit 4 has no effect in this case. |
@eightycc I've had to increase the pio divisor to 3 for reliable operation for reasons unknown. I don't know if that's the same issue. |
@peterharperuk I'd suspect signal issues for your case. The problem I'm reporting kicks in around divide by 10 and up. I hit it while increasing the divisor to compensate for a nasty fly wire bodge I've been using for tracing. |
I was led to believe (maybe wrongly) that the firmware was built to only work for a specific SPI clock speed. And that's why we have to mess about with the divisor if we change the core clock speed. |
Once I changed from |
@peterharperuk I've done some more testing with both the CYW43439 on the Pico W boards and the RM2 module. The older CYW43439 operates as described in the Cypress Semi datasheet. It does not change gSPI data phasing at lower clock rates. The newer RM2 modules change gSPI data phasing somewhere between 21.429MHz and 23.077MHz on the two RM2 modules I tested. Assuming a 150MHz processor clock, a divisor of 3 results in a 25MHz gSPI clock. Depending on the accuracy of the RM2's gSPI clock measurement, a divisor of 3 could conceivably fail intermittently. At 23.077MHz (divisor 3, fraction 64) with a debug build I'm seeing instability.which leads me to think that there's a range of gSPI clock frequencies centered around 22MHz that are unusable. Another factor that may affect the RM2's gSPI clock measurement leading to unpredictability is the PIO clock divider's insertion of "leap" cycles to maintain the specified clock rate. The resulting jitter may cause measurement of small clock samples by the RM2 to be inaccurate causing it to change phasing erratically. Back to stable operation at 18.75MHz (divisor 4, fraction 0), but need to specify |
No longer depends on micropython#16057
This makes sense. It feels like there should be a better approach to the pins.csv wrangling. It's a lot of duplication to accomplish not very much. My gut feeling would be to omit them, but perhaps something could be hung off the Note: I've raised a PR for the pins.csv functionality, since I think it's a necessary and sensible addition to the variant support anyway, that should at least remove a commit from this pile if it's accepted: #17028 |
Adds changes needed to support Pico 2 W. This requires the develop version of the sdk so I've kept the PR in draft.