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

Bump svd2rust v0.31.5 and svdtool v0.3.9 #91

Merged
merged 10 commits into from
Feb 19, 2024

Conversation

AkiyukiOkayasu
Copy link
Contributor

svdtool and svd2rust have been updated to the latest.

svd2rust's update only re-generated the file.
The svdtool update seems to have made it necessary to explicitly specify the description to collect the register.
See b373a55 for details.

@jannic
Copy link
Member

jannic commented Jan 24, 2024

This contains some breaking changes, but I think that doesn't matter: According to cargo semver-checks, we already have some breaking changes since v0.5.0, so the next release will need a bump to v0.6.0 anyway.

@AkiyukiOkayasu
Copy link
Contributor Author

I agree with that opinion.
rp-hal needs to make some changes as well, but I am willing to work on it.
I think most of them don't require significant changes.

@jannic
Copy link
Member

jannic commented Jan 26, 2024

I think most of them don't require significant changes.

I played a bit with it. Most of the errors are of this form:

error[E0616]: field `en` of struct `rp2040_pac::pwm::RegisterBlock` is private
   --> rp2040-hal/src/pwm/mod.rs:567:33
    |
567 |             let reg = self._pwm.en.as_ptr();
    |                                 ^^ private field
    |
help: a method `en` also exists, call it with parentheses
    |
567 |             let reg = self._pwm.en().as_ptr();
    |                                   ++

The compiler hint is spot-on. So fixing is trivial, but very tedious. There are a lot of register accesses in the hal.

Some are inside macros which makes the error messages less obvious, but the fix is always the same.

@AkiyukiOkayasu
Copy link
Contributor Author

Thanks for the specific examples.
Easy to fix, but it's a huge amount of work.

This breaking change was caused by rust-embedded/svd2rust#692 in svd2rust v0.31.0.
The fix is tedious, but I think svd2rust should be updated.

Anyway, I am sending a PR rp-rs/rp-hal#757 to rp-hal.
I will do this fix at the same time, as that PR will need to be fixed after rp2040-pac v0.6.0 is released.

@jannic
Copy link
Member

jannic commented Jan 27, 2024

@AkiyukiOkayasu: Did you see AkiyukiOkayasu#1 ?

And I pushed the changes I already made to rp-hal while experimenting with the updated PAC to https://github.com/jannic/rp-hal/tree/pac-update . It's not complete (doesn't build yet), but I wanted to publish it somewhere to avoid duplicated work. Feel free to copy whatever you want.

Add --reexport-core-peripherals flag to svd2rust
@AkiyukiOkayasu
Copy link
Contributor Author

@jannic Sorry, I didn't see that. Thank you for PR!

Thanks also for the work on rp-hal. I appreciate your thoughtfulness.
I'm busy today and tomorrow, so I'll check back the day after tomorrow.

@AkiyukiOkayasu
Copy link
Contributor Author

@jannic The pac-update branch seems to be 32 commits behind the rp-hal/rp-hal main branch, is that ok?

@jannic
Copy link
Member

jannic commented Jan 29, 2024

I just forgot to git pull first. The branch wasn't prepared carefully because it was only meant as a proof of concept to make sure that the pac still works. And then I didn't have enough time to finish it.

@AkiyukiOkayasu
Copy link
Contributor Author

@jannic The HAL update is almost finished. A few issues remain, but I hope to have them resolved in the next few days.
I contacted you just to make sure that the work is not duplicated.

@AkiyukiOkayasu
Copy link
Contributor Author

@jannic I have confirmed that the pac modified in this PR works with rp-hal. A large number of small changes were needed.

I think it is exactly what you imagined, please check rp-hal's PR for the content.

@AkiyukiOkayasu
Copy link
Contributor Author

Sorry, I noticed that some of HAL's on-target-tests were broken. Still needs additional work.
Perhaps no further modifications to the PAC are needed, but in any case, the problem is happening at HAL, so development will proceed at HAL's PR.

Copy link
Member

@jannic jannic left a comment

Choose a reason for hiding this comment

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

After fixing the on-target tests of rp2040-hal, I'm convinced that the updated PAC will work as expected.

@AkiyukiOkayasu, thanks for all the work updating the PAC and preparing the proof-of-concept port of rp2040-hal. Looking at the diff of rp2040-hal, it must have been a lot of tedious editing.

@ithinuel ithinuel merged commit 615f600 into rp-rs:main Feb 19, 2024
4 checks passed
@jannic jannic mentioned this pull request Feb 20, 2024
@AkiyukiOkayasu AkiyukiOkayasu deleted the bump-svd2rust-0.31.5 branch February 26, 2024 13:35
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