Statically enforces that only comparator 0 onarmv7m 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

TDHolmes requested a review froma team as acode owner

January 2, 2022 00:27

rust-highfive assignedadamgreig

Jan 2, 2022

Copy link

rust-highfive commentedJan 2, 2022

r?@adamgreig

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

rust-highfive added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-cortex-m labels

Jan 2, 2022

Copy link

ContributorAuthor

TDHolmes commentedJan 2, 2022

🏷️@tmplt

Copy link

ContributorAuthor

TDHolmes commentedJan 2, 2022

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.

Copy link

Contributor

tmplt commentedJan 2, 2022

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.

Copy link

ContributorAuthor

TDHolmes commentedJan 5, 2022

Thoughts@tmplt ?

Copy link

Contributor

tmplt commentedJan 5, 2022

I'll go over it tomorrow.

tmplt reviewed

Jan 6, 2022

View reviewed changes

CHANGELOG.md OutdatedShow resolvedHide resolved

src/peripheral/dwt.rs OutdatedShow resolvedHide resolved

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

Copy link

Contributor

tmpltJan 6, 2022

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?

Copy link

ContributorAuthor

TDHolmesJan 6, 2022•
edited
Loading

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 argumentSupportedFunctions, but it's zero sized so we usePhantomData. As the tests show, it does not increase the size of theRegisterBlock (addresses forcomp0 and more importantlycomp[0] remained the same)

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

Copy link

Contributor

tmpltJan 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Can we testconfigure here?

Copy link

ContributorAuthor

TDHolmesJan 6, 2022

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

TDHolmes force-pushed thedifferentiate-comparator-0 branch fromc8a4fdb to0dc7047Compare

January 6, 2022 21:57

Copy link

ContributorAuthor

TDHolmes commentedJan 6, 2022•
edited
Loading

I made all of theDWT::has_* implementation feature checks associated functions instead of methods (they no longer take&self) perthis comment as it allows me to checkhas_cycle_counter inconfigure.

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.

Copy link

ContributorAuthor

TDHolmes commentedJan 6, 2022•
edited
Loading

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

Copy link

ContributorAuthor

TDHolmes commentedJan 6, 2022•
edited
Loading

Actually.. I suppose I could still implementIndex where I return the typeResult<ComparatorTypes, DwtError> whereComparatorTypes is an enum:

enumComparatorTypes{NoCycleCount(Comparator<NoCycleCompare>),WithCycleCount(Comparator<HasCycleCompare>),

thoughts?

Copy link

Contributor

tmplt commentedJan 7, 2022

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 theunsafe usage.

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

Copy link

ContributorAuthor

TDHolmes commentedJan 7, 2022

In that case this should be ready to review and merge

tmplt suggested changes

Jan 7, 2022

View reviewed changes

src/peripheral/dwt.rs OutdatedShow resolvedHide resolved

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

Copy link

Contributor

tmpltJan 7, 2022

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?

Copy link

ContributorAuthor

TDHolmesJan 7, 2022

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

src/peripheral/dwt.rs OutdatedShow resolvedHide resolved

Copy link

Contributor

tmplt commentedJan 8, 2022

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

Copy link

ContributorAuthor

TDHolmes commentedJan 8, 2022

Sounds good, thanks!

Copy link

Contributor

tmplt commentedJan 13, 2022

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

hardware task - entersoftware task - entersoftware task - exithardware 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.

TDHolmes added8 commits

October 8, 2022 11:55

Differentiate comparator 0 as the only one capable of cycle compare

9e263cc

Statically enforces that only comparator 0 on `armv7m` can be configuredfor cycle comparison by introducing a marker trait differentiatingcomparator 0 and the rest of them, and only implementing the ability forthis configuration on comparator 0.Closesrust-embedded#376

changelog

22efd36

move thehas_* 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 thedifferentiate-comparator-0 branch fromb666289 to457864dCompare

October 8, 2022 18:58

Copy link

ContributorAuthor

TDHolmes commentedOct 8, 2022

rebased onto latest master

has_cycle_counter is now an associated function

49a8438

TDHolmes mentioned this pull request

Oct 8, 2022

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

Open

fix tests

ef728e8

TDHolmes force-pushed thedifferentiate-comparator-0 branch froma41ceeb toef728e8Compare

October 8, 2022 21:28

Labels

S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-cortex-m

4 participants

Movatterモバイル変換

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

Uh oh!

Conversation

TDHolmes commentedJan 2, 2022

Uh oh!

rust-highfive commentedJan 2, 2022

Uh oh!

TDHolmes commentedJan 2, 2022

Uh oh!

TDHolmes commentedJan 2, 2022

Uh oh!

tmplt commentedJan 2, 2022

Uh oh!

TDHolmes commentedJan 5, 2022

Uh oh!

tmplt commentedJan 5, 2022

Uh oh!

Uh oh!

Uh oh!

tmpltJan 6, 2022

Choose a reason for hiding this comment

Uh oh!

TDHolmesJan 6, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmpltJan 6, 2022

Choose a reason for hiding this comment

Uh oh!

TDHolmesJan 6, 2022

Choose a reason for hiding this comment

Uh oh!

TDHolmes commentedJan 6, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

TDHolmes commentedJan 6, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

TDHolmes commentedJan 6, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

tmplt commentedJan 7, 2022

Uh oh!

TDHolmes commentedJan 7, 2022

Uh oh!

Uh oh!

tmpltJan 7, 2022

Choose a reason for hiding this comment

Uh oh!

TDHolmesJan 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tmplt commentedJan 8, 2022

Uh oh!

TDHolmes commentedJan 8, 2022

Uh oh!

tmplt commentedJan 13, 2022

Uh oh!

TDHolmes commentedOct 8, 2022

Uh oh!

Uh oh!

TDHolmesJan 6, 2022•
edited
Loading

TDHolmes commentedJan 6, 2022•
edited
Loading

TDHolmes commentedJan 6, 2022•
edited
Loading

TDHolmes commentedJan 6, 2022•
edited
Loading