search

rp-rs / rp2040-pac

A Rust PAC for the RP2040 Microcontroller
145 stars 28 forks source link

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

Closed AkiyukiOkayasu closed 8 months ago

AkiyukiOkayasu commented 9 months ago

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 b373a5516f1193029e7c7829d74d308c19516814 for details.

jannic commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

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

Anyway, I am sending a PR https://github.com/rp-rs/rp-hal/pull/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 commented 9 months ago

@AkiyukiOkayasu: Did you see https://github.com/AkiyukiOkayasu/rp2040-pac/pull/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.

AkiyukiOkayasu commented 9 months ago

@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 commented 9 months ago

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

jannic commented 9 months ago

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 commented 9 months ago

@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 commented 8 months ago

@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 commented 8 months ago

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.