-
Notifications
You must be signed in to change notification settings - Fork 154
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
Differentiate comparator 0 as the only one capable of cycle compare #377
base: master
Are you sure you want to change the base?
Differentiate comparator 0 as the only one capable of cycle compare #377
Conversation
r? @adamgreig (rust-highfive has picked a reviewer for you, use r? to override) |
🏷️ @tmplt |
I tested the cycle count comparison functionality on HW to make sure this all worked as expected. I'm not sure how to test the address compare functionality on HW so didn't check that. |
I don't see any outstanding issues reviewing from a phone at the moment. I'll go over it in detail when I'm back in my office next week. |
Thoughts @tmplt ? |
I'll go over it tomorrow. |
/// Comparator | ||
pub comp: RW<u32>, | ||
/// Comparator Mask | ||
pub mask: RW<u32>, | ||
/// Comparator Function | ||
pub function: RW<Function>, | ||
reserved: u32, | ||
_supported_functions: core::marker::PhantomData<SupportedFunctions>, |
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.
What effect does this have?
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'm not 100% sure what you mean so I'll take a stab at what I think you're asking. We need to "cary" around the generic argument SupportedFunctions
, but it's zero sized so we use PhantomData
. As the tests show, it does not increase the size of the RegisterBlock
(addresses for comp0
and more importantly comp[0]
remained the same)
assert_eq!(address(&dwt.comp[0].comp), 0xE000_1020); | ||
assert_eq!(address(&dwt.comp[0].mask), 0xE000_1024); | ||
assert_eq!(address(&dwt.comp[0].function), 0xE000_1028); | ||
} |
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.
Can we test configure
here?
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 don't think so unfortunately. These tests run on the native host so the best we can do is make sure these things are pointing to the right address. If we dereferenced them and tried to check behavior, we'd crash and burn
c8a4fdb
to
0dc7047
Compare
I made all of the edit: Looks like this was originally how it was implemented, but then you changed it in #342. Let me know if there was a good reason, but I think it makes sense to go back to associated functions. |
I also tried to be fancy by wrapping these two slightly different comparators in a unified struct which then vended the right one by implementing the |
Actually.. I suppose I could still implement enum ComparatorTypes {
NoCycleCount(Comparator<NoCycleCompare>),
WithCycleCount(Comparator<HasCycleCompare>), thoughts? |
At the time I thought it was relics from an older implementation which I refactored just to remove the As for |
In that case this should be ready to review and merge |
CHANGELOG.md
Outdated
@@ -20,6 +25,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/). | |||
|
|||
### Fixed | |||
- Fixed `singleton!()` statics sometimes ending up in `.data` instead of `.bss` (#364, #380). | |||
- Corrected `SCB.ICSR.VECTACTIVE`/`SCB::vect_active()` to be 9 bits instead of 8. | |||
Also fixes `VectActive::from` to take a `u16` and subtract `16` for | |||
`VectActive::Interrupt`s to match `SBC::vect_active()` (#373). |
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.
Transient changes from another PR?
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 failed to update the changelog in my original PR so sneaking it in here
I want to try this out on hardware before I sign off on it. Hopefully I'll have it done by next week. |
Sounds good, thanks! |
I've been testing this PR with a in
With the patch I get a very confusing trace. I doubt this PR is at fault, but to make sure I need to handle #392 and #383 first. |
Statically enforces that only comparator 0 on `armv7m` can be configured for cycle comparison by introducing a marker trait differentiating comparator 0 and the rest of them, and only implementing the ability for this configuration on comparator 0. Closes rust-embedded#376
…unction & some cleanup
b666289
to
457864d
Compare
rebased onto latest master |
a41ceeb
to
ef728e8
Compare
Statically enforces that only comparator 0 on
armv7m
can be configured for cycle comparison by introducing a marker trait differentiating comparator 0 and the rest of them, and only implementing the ability for this configuration on comparator 0.Closes #376