Differentiate comparator 0 as the only one capable of cycle compare #377

TDHolmes · 2022-01-02T00:27:16Z

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

rust-highfive · 2022-01-02T00:27:19Z

r? @adamgreig

(rust-highfive has picked a reviewer for you, use r? to override)

TDHolmes · 2022-01-02T00:27:43Z

🏷️ @tmplt

TDHolmes · 2022-01-02T00:35:17Z

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.

tmplt · 2022-01-02T06:31:54Z

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.

TDHolmes · 2022-01-05T19:04:31Z

Thoughts @tmplt ?

tmplt · 2022-01-05T19:38:56Z

I'll go over it tomorrow.

CHANGELOG.md

src/peripheral/dwt.rs

tmplt · 2022-01-06T11:57:31Z

src/peripheral/dwt.rs

    /// 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>,


What effect does this have?

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)

tmplt · 2022-01-06T12:00:16Z

src/peripheral/test.rs

+        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);
+    }


Can we test configure here?

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

TDHolmes · 2022-01-06T22:00:07Z

I made all of the DWT::has_* implementation feature checks associated functions instead of methods (they no longer take &self) per this comment as it allows me to check has_cycle_counter in configure.

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.

TDHolmes · 2022-01-06T22:02:42Z

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 Index trait, however it requires GATs to work :( it would've been cool because then we could've also checked DWT::num_comp to see if the comparator you're trying to access is even implemented. Oh well...

TDHolmes · 2022-01-06T23:11:15Z

Actually.. I suppose I could still implement Index where I return the type Result<ComparatorTypes, DwtError> where ComparatorTypes is an enum:

enum ComparatorTypes {
    NoCycleCount(Comparator<NoCycleCompare>),
    WithCycleCount(Comparator<HasCycleCompare>),

thoughts?

tmplt · 2022-01-07T10:39:36Z

Let me know if there was a good reason

At the time I thought it was relics from an older implementation which I refactored just to remove the unsafe usage.

As for Index I suggest limiting this PR to its namesake and create another one for the comparator implementation check.

TDHolmes · 2022-01-07T16:35:39Z

In that case this should be ready to review and merge

src/peripheral/dwt.rs

tmplt · 2022-01-07T16:41:33Z

CHANGELOG.md

@@ -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).


Transient changes from another PR?

I failed to update the changelog in my original PR so sneaking it in here

src/peripheral/dwt.rs

tmplt · 2022-01-08T01:19:38Z

I want to try this out on hardware before I sign off on it. Hopefully I'll have it done by next week.

TDHolmes · 2022-01-08T02:20:09Z

Sounds good, thanks!

tmplt · 2022-01-13T15:49:34Z

I've been testing this PR with a in cortex-m-rtic-trace branch, based on an rtic-scope-dw0 branch of cortex-m that contains this PR, for use with cargo-rtic-scope. Without the patch I see

hardware task - enter
software task - enter
software task - exit
hardware task - exit / overflow

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

TDHolmes · 2022-10-08T18:59:31Z

rebased onto latest master

TDHolmes requested a review from a team as a code owner January 2, 2022 00:27

rust-highfive assigned adamgreig Jan 2, 2022

rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m labels Jan 2, 2022

tmplt reviewed Jan 6, 2022

View reviewed changes

TDHolmes force-pushed the differentiate-comparator-0 branch from c8a4fdb to 0dc7047 Compare January 6, 2022 21:57

tmplt suggested changes Jan 7, 2022

View reviewed changes

TDHolmes added 8 commits October 8, 2022 11:55

changelog

22efd36

move the has_* DWT methods to associated functions

f24999f

register field rename as suggested

916e494

check DWT::has_cycle_counter when configuring a comparator for that f…

ffa638c

…unction & some cleanup

call out the has_ changes in changelog

82b1975

fix tests

79111ff

review comments

457864d

TDHolmes force-pushed the differentiate-comparator-0 branch from b666289 to 457864d Compare October 8, 2022 18:58

has_cycle_counter is now an associated function

49a8438

TDHolmes mentioned this pull request Oct 8, 2022

fix cyccnt extension based on cortex-m change TDHolmes/embedded-profiling#8

Open

fix tests

ef728e8

TDHolmes force-pushed the differentiate-comparator-0 branch from a41ceeb to ef728e8 Compare October 8, 2022 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Differentiate comparator 0 as the only one capable of cycle compare #377

Differentiate comparator 0 as the only one capable of cycle compare #377

TDHolmes commented Jan 2, 2022

rust-highfive commented Jan 2, 2022

TDHolmes commented Jan 2, 2022

TDHolmes commented Jan 2, 2022

tmplt commented Jan 2, 2022

TDHolmes commented Jan 5, 2022

tmplt commented Jan 5, 2022

tmplt Jan 6, 2022

TDHolmes Jan 6, 2022 •

edited

Loading

tmplt Jan 6, 2022

TDHolmes Jan 6, 2022

TDHolmes commented Jan 6, 2022 •

edited

Loading

TDHolmes commented Jan 6, 2022 •

edited

Loading

TDHolmes commented Jan 6, 2022 •

edited

Loading

tmplt commented Jan 7, 2022

TDHolmes commented Jan 7, 2022

tmplt Jan 7, 2022

TDHolmes Jan 7, 2022

tmplt commented Jan 8, 2022

TDHolmes commented Jan 8, 2022

tmplt commented Jan 13, 2022

TDHolmes commented Oct 8, 2022

Differentiate comparator 0 as the only one capable of cycle compare #377

Are you sure you want to change the base?

Differentiate comparator 0 as the only one capable of cycle compare #377

Conversation

TDHolmes commented Jan 2, 2022

rust-highfive commented Jan 2, 2022

TDHolmes commented Jan 2, 2022

TDHolmes commented Jan 2, 2022

tmplt commented Jan 2, 2022

TDHolmes commented Jan 5, 2022

tmplt commented Jan 5, 2022

tmplt Jan 6, 2022

Choose a reason for hiding this comment

TDHolmes Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

tmplt Jan 6, 2022

Choose a reason for hiding this comment

TDHolmes Jan 6, 2022

Choose a reason for hiding this comment

TDHolmes commented Jan 6, 2022 • edited Loading

TDHolmes commented Jan 6, 2022 • edited Loading

TDHolmes commented Jan 6, 2022 • edited Loading

tmplt commented Jan 7, 2022

TDHolmes commented Jan 7, 2022

tmplt Jan 7, 2022

Choose a reason for hiding this comment

TDHolmes Jan 7, 2022

Choose a reason for hiding this comment

tmplt commented Jan 8, 2022

TDHolmes commented Jan 8, 2022

tmplt commented Jan 13, 2022

TDHolmes commented Oct 8, 2022

TDHolmes Jan 6, 2022 •

edited

Loading

TDHolmes commented Jan 6, 2022 •

edited

Loading

TDHolmes commented Jan 6, 2022 •

edited

Loading

TDHolmes commented Jan 6, 2022 •

edited

Loading