add methods needed for cycle count comparisons #367
Conversation
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
S-waiting-on-review
|
Also tag @tmplt |
There was a problem hiding this comment.
Other than the below, DATAV{ADDR{1,0},SIZE} are SBZ on cycle count match, but these are not declared in the Function bitfield (c.f. Page 806) and never written to. These fields are also SBZ on address comparison match. I wonder if it isn't a good idea to write zero to these just in case.
It's for the better now that I think about it. If they are non-zero target behavior will be unpredictable on comparator match. Had they been marked SBZP I would have disregarded it, but that's not the case. |
|
Agreed, updated |
|
Yeah fair argument. I’ll remove the check and add to the documentation a
recommendation for the end user to check this function if they’re going to
use cycle compare
…On Thu, Dec 16, 2021 at 8:16 AM Viktor Vilhelm Sonesten < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/peripheral/dwt.rs
<#367 (comment)>
:
> }
impl Comparator {
/// Configure the function of the comparator
#[allow(clippy::missing_inline_in_public_items)]
- pub fn configure(&self, settings: ComparatorFunction) -> Result<(), DwtError> {
+ pub fn configure(&self, dwt: &DWT, settings: ComparatorFunction) -> Result<(), DwtError> {
The root issue in my eyes is requiring the user to pass the DWT to
configure for an effectivly non-exhaustive check: while the current check
ensures that dwt[0] supports cycle counters, configure still allows us to
configure dwt[1] for it (effectively an invalid configure invocation,
hardware-wise), as long as dwt[0] supports it. An exhaustive check would
require two different types for dwt[0] and dwt[n > 0] and only allow
cycle count comparisons for the dwt[0] type (as previously discussed),
but that's an issue for another day.
—
Reply to this email directly, view it on GitHub
<#367 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC5YNWQ6CJRERYE4LVMQ7NDURIGF3ANCNFSM5KBIQS7Q>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
CI is failing due to #371 |
|
@tmplt thanks for the groundwork and thorough review! Looking forward to reading your thesis 😄 |
|
Are those nightly CI failures expected due to asm! changes? |
|
Yes. That needs to be addressed for all other PRs |
TDHolmes
force-pushed
the
DCB-enhancements
branch
from
7935c89 to
4d2c198
Compare
|
rebased on top of the asm PR to fix CI |
|
Any update on this PR? It should be a non breaking change. Even though |
|
Looks ok to me, but I'm not an expert in this area |
|
@tmplt Seems to be the expert and approved but I dunno if he has privileges to merge |
There was a problem hiding this comment.
This is definitely a breaking change as it adds variants to two public enums that aren't already non_exhaustive, but all the code it changes is already breaking changes from #342, so it will have to go into a 0.8 either way.
Other than that LGTM.
bors merge
|
Build succeeded: |
375: Prepare v0.7.4 r=thejpster a=adamgreig I've created a new branch, `v0.7.x`, which is currently at the latest non-breaking commit (so includes #346 #349 #347 #351 #339 #352 #348 #363 #362 #361 but does not include #342), to track the 0.7 series since master now contains breaking changes for v0.8. This PR (which targets the new branch) cherry-picks #372 #369 #374 and bumps the version to v0.7.4 (and updates CHANGELOG) ready for a new v0.7.4 release. Once complete I'll also backport the changelog entries and bump the version in master to 0.7.4. I think this is everything that should be in 0.7 -- the only excluded PRs from master are #342 and #367 I believe, and I don't think we have any open PRs targeting 0.7 either. Any other thoughts on items for inclusion in 0.7.4 (or other changelog entries I missed)? Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com> Co-authored-by: Adam Greig <[email protected]>
Extends the existing DWT comparator code to include cycle count comparisons which fire a
DebugMonitorexception, if enabled withenable_debug_monitor. See working example here