Skip to content
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

Add an RP2350 HAL. #834

Merged
merged 19 commits into from
Aug 23, 2024
Merged

Add an RP2350 HAL. #834

merged 19 commits into from
Aug 23, 2024

Conversation

thejpster
Copy link
Member

Taken from https://github.com/thejpster/rp-hal-rp2350-public, with minor tweaks.

@thejpster
Copy link
Member Author

Hey, it finally completed all 98 jobs!

.github/workflows/rp235x_hal_examples_riscv.yml Outdated Show resolved Hide resolved
rp-hal-common/src/lib.rs Outdated Show resolved Hide resolved
rp235x-hal-examples/.cargo/config.toml Outdated Show resolved Hide resolved
rp235x-hal-examples/.cargo/config.toml Outdated Show resolved Hide resolved
rp235x-hal-examples/.cargo/config.toml Outdated Show resolved Hide resolved
rp235x-hal/src/spi.rs Outdated Show resolved Hide resolved
rp235x-hal/src/spi.rs Outdated Show resolved Hide resolved
rp235x-hal/src/spi.rs Outdated Show resolved Hide resolved
rp235x-hal/src/timer.rs Outdated Show resolved Hide resolved
rp235x-hal/src/watchdog.rs Outdated Show resolved Hide resolved
thejpster and others added 11 commits August 22, 2024 19:28
Also remove references to RP2040 that I missed.
elf2uf2-rs doesn't work on RP2350. See JoNil/elf2uf2-rs#33
Of course changing the board name caused every comment to re-wrap.
Both examples had a copy-pasta description. That's now fixed.
Both examples had a typo in the comments.
Both examples had a typo in the description. That's now fixed.
@thejpster
Copy link
Member Author

OK I think I hit them all. RIP all those GHA free minutes.

@jannic
Copy link
Member

jannic commented Aug 22, 2024

Some files got updates for rp2040 after you did the copy for rp235x.
Two examples I found were #831 for SPI and #798 for UART.
We could either update the pull request, or merge it first and then check for unintended divergences. What would you prefer?
(Maybe we can also do some more code deduplication?)

@9names
Copy link
Member

9names commented Aug 23, 2024

We could either update the pull request, or merge it first and then check for unintended divergences.

Lets do it the latter way, it would be better as smaller update PRs.
Github really struggled when I was trying to review this mega PR, the sooner it's closed the better.

Copy link
Member

@9names 9names left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, couple of copy-paste errors on datasheet URL but otherwise good to go

rp235x-hal/src/atomic_register_access.rs Show resolved Hide resolved
rp235x-hal/src/lposc.rs Outdated Show resolved Hide resolved
rp235x-hal/src/timer.rs Outdated Show resolved Hide resolved
@thejpster
Copy link
Member Author

Maybe we can also do some more code deduplication?

Yes, once this is in there's a lot of scope for de-duplication. I wasn't able to track the rp2040-hal whilst working in my private repo, so it sort of diverged. Then to open this PR I had to rebase a bunch of stuff and try and get closer to where you were (but you kept moving). I propose that after this goes in we do a diff -r rp2040-hal rp235x-hal and try and remove as many diffs as possible - either with changes to one of the HALs, or by moving things into rp-common-hal.

@thejpster thejpster merged commit 84e1bf0 into main Aug 23, 2024
98 checks passed
@thejpster thejpster deleted the add-rp235x branch September 8, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants