Update NVIC #62
Update NVIC #62
Conversation
|
Thanks for the PR, @hannobraun. I would prefer to keep the existing API and implementation for ARMv7 because (a) changing the fields is a breaking change, (b) the word access limitation doesn't exist on that architecture so no need to "cripple" the register, (c) the change on re: (c). The documentation should note that Changing the fields is a breaking change on ARMv6 so I'm wondering if we should fix |
Oh, good catch! I didn't think about that.
Does the concept of "breaking change" apply, if the existing behavior is already broken? Personally I'd prefer to be notified of my broken code (via a compiler error) as soon as possible. Better than it staying silently broken until 0.4. Not sure how others might see this. Regarding the rest of your comment: I'm in full agreement. I'll update the pull request next week. |
src/peripheral/mod.rs
Outdated
| // able to verify that, due to a Cargo bug[1]. As it stands, the | ||
| // documentation always shows the ARMv7-M variant of the field. | ||
| // [1] https://github.com/rust-lang/cargo/issues/2539 | ||
| #[allow(missing_docs)] |
There was a problem hiding this comment.
Does anyone know if there's a rust issue for the problem I've described here? I couldn't find one.
|
@japaric I've updated the pull request. Please let me know if there's anything else I can do. Please note that I haven't tested this on an ARMv7-M, as I only have ARM Cortex-M0+ microcontrollers at hand right now. Presumably nothing should have changed, as I only moved the existing code into a |
Changing the fields of a struct is a breaking change, API wise, as per semver. So if I want to release a new version after merging this PR, as it is, it would have to be v0.4.0 to comply with semver. If the existing behavior is already broken then that's a bug in the library. Depending on the gravity of the bug you may want to yank the buggy release. I have only yanked buggy versions when the bug was a memory safety hole. Check versions v0.2.x in the CHANGELOG; there I made "breaking change" across patch releases because I yanked the previous release (all the way to v0.2.0). I don't think the yanking approach can be applied here because the field API has been like that, i.e. broken on ARMv6, since a long time ago; we would have to yank pretty much all the previous releases. I would prefer to hold off on making a new minor release until rust-embedded/svd2rust#151 is implemented, if implemented at all. Mainly because implementing that may involve breaking changes here and in other places and I'd like to make all those at once (i.e. upgrade the ecosystem in lockstep). But if this is a pressing issue then I'm fine with releasing a v0.4.0 and then move to v0.5.0 when rust-embedded/svd2rust#151 is implemented.
We can fix |
I don't mean to argue that this isn't technically a breaking change. It definitely is. I was just wondering if that matters, if the feature was broken in the first place. Either way, I don't feel strongly about this, and I don't think it makes sense to yank any versions over what appears to be a niche issue (presumably, I assume someone would have noticed earlier if it wasn't).
I don't mind depending on my fork for the time being, so it's not pressing to me. So, how to proceed? I see three options:
I'd prefer options 1 or 2, as that wouldn't require any extra work from me, at the moment. And option 3 would mean we have to remember to make the breaking change before 0.4. But if you think option 3 is the way to go, I'll happily make some time to update the pull request. |
|
FYI, I opened a Rust issue for the documentation issue I worked around in this pull request: |
|
I've pushed an updated commit to remove my workaround for the issue caused by the interaction of This duplicates the documentation for both variants of the field. In the linked Rust issue, a workaround was suggested to prevent that, defining a
|
|
☔ The latest upstream changes (presumably #65) made this pull request unmergeable. Please resolve the merge conflicts. |
According to the ARMv7-M Technical Reference Manual[1], there are 124 IPR registers available on ARMv7-M, and 16 of all others. I don't know where the original numbers came from, since on ARMv6-M, there are only 8 IPR registers available, and 1 of each of the others.[2] This commit removes some test cases that were checking the address of the last register. Since the last register has changed, those are no longer applicable. I decided to remove instead of update them, since they only really test the length of each register type, which is obvious enough from the code. [1]: https://silver.arm.com/download/ARM_and_AMBA_Architecture/AR580-DA-70000-r0p0-05rel0/DDI0403E_B_armv7m_arm.pdf [2]: https://silver.arm.com/download/ARM_and_AMBA_Architecture/AR585-DA-70000-r0p0-00rel0/DDI0419C_arm_architecture_v6m_reference_manual.pdf
On ARMv6-M, anything but world-aligned access to the IPR registers will lead to unpredictable results. Fixes #61.
|
I've rebased the branch, but wasn't able to re-test the changes. The application I've previously used to test pull request this hasn't been ported to the singleton side of things yet, and I don't think I'll get to that until next year. That said, the rebase was trivial and the diff looks basically the same as before. I don't think I've introduced any new problems. |
There was a problem hiding this comment.
Thanks for updating the PR, @hannobraun.
I think some of these methods, their signatures, will have to be updated to better match the concept of scoped singletons (e.g. some methods can become static methods (no self argument)) but that can be done in another PR)
@homunkulus r+
| @@ -118,12 +153,40 @@ impl RegisterBlock { | |||
| /// | |||
| /// NOTE See `get_priority` method for an explanation of how NVIC priorities | |||
| /// work. | |||
| /// | |||
| /// On ARMv6-M, updating an interrupt priority requires a read-modify-write | |||
There was a problem hiding this comment.
I think this is not an issue with scoped singletons, at least if we change the signature to &mut self. With scoped singletons if the singleton (peripheral) in owned by an execution context then that context has &mut - access to the peripheral and this is not an issue. If the peripheral is shared between two, or more, execution contexts then some mechanism must be in place to (safely and temporarily) yield a &mut- reference to the register block to one of the contexts -- again, no issue there.
I think it's still a good idea to mention that the operation is an atomic write on ARMv7 and a RMW operation on ARMv6.
|
@homunkulus r+ |
|
📌 Commit d952f0c has been approved by |
Update NVIC This pull request updated the NVIC definition and brings it in line with the ARMv7-M technical reference manual. Since ARMv6-M's NVIC is a subset of ARMv7-M's one, this should work on both platforms. I tried adding some `#[cfg(armv7m]`, to make only the registers available on ARMv6-M visible on that platform, but aborted that plan, as it seemed to add a lot of complexity. What do you think about this, @japaric? I also checked the [ARMv8-M Technical Reference Manual](https://static.docs.arm.com/ddi0553/a/DDI0553A_e_armv8m_arm.pdf). The NVIC is largely identical to ARMv7-M's one and only adds an additional block of registers between IABR and IPR, so it will be straight-forward to add support once that becomes relevant.
|
☀️ Test successful - status-travis |
|
Thanks for merging, @japaric! I agree with you comments regarding the method signatures are spot on. I'm going to open an issue to track that. |
This pull request updated the NVIC definition and brings it in line with the ARMv7-M technical reference manual. Since ARMv6-M's NVIC is a subset of ARMv7-M's one, this should work on both platforms.
I tried adding some
#[cfg(armv7m], to make only the registers available on ARMv6-M visible on that platform, but aborted that plan, as it seemed to add a lot of complexity. What do you think about this, @japaric?I also checked the ARMv8-M Technical Reference Manual. The NVIC is largely identical to ARMv7-M's one and only adds an additional block of registers between IABR and IPR, so it will be straight-forward to add support once that becomes relevant.