Skip to content
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

Add support for additional DWT counters #349

Merged
merged 3 commits into from
Aug 14, 2021
Merged

Conversation

newAM
Copy link
Member

@newAM newAM commented Aug 8, 2021

This adds support for these DWT counters:

  • CPI counter
  • Exception overhead counter
  • LSU counter
  • Folded-instruction counter

The getters do not have the get_ prefix per the C-GETTER API guidelines. This does not match the existing get_cycle_count method. I was not sure if I should maintain consistency or not, let me know if you want this changed.

Here is the documentation to validate each of the added methods:

This adds support for these counters:
* CPI counter
* Exception overhead counter
* LSU counter
* Folded-instruction counter
@newAM newAM requested a review from a team as a code owner August 8, 2021 21:20
@rust-highfive
Copy link

r? @therealprof

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

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m labels Aug 8, 2021
@@ -62,7 +62,56 @@ pub struct Comparator {
reserved: u32,
}

// DWT CTRL register fields
Copy link
Contributor

@thejpster thejpster Aug 9, 2021

Choose a reason for hiding this comment

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

It might be worth trying to add a link to the Arm Armv7-M ARM here, to explain where the constants came from.

https://developer.arm.com/documentation/ddi0403/d/Debug-Architecture/ARMv7-M-Debug/The-Data-Watchpoint-and-Trace-unit/Control-register--DWT-CTRL might work.

@thejpster
Copy link
Contributor

thejpster commented Aug 9, 2021

I think I'd like to see a cycle_count() method for consistency. I'm relaxed as to whether get_cycle_count() is marked deprecated, or on what timescale (but it should at least note in the docs that cycle_count() is canonical).

@newAM
Copy link
Member Author

newAM commented Aug 11, 2021

I think I'd like to see a cycle_count() method for consistency. I'm relaxed as to whether get_cycle_count() is marked deprecated, or on what timescale (but it should at least note in the docs that cycle_count() is canonical).

I am happy to do that too. This is a debug feature, the crate is still pre-1.0, and it is a minor change, so I don't think the impact will be significant. Is it ok to make that change within this pull-request? That is marking get_cycle_count as deprecated and adding a cycle_count method that does the same thing. Not sure if there is any process for making a breaking change with this crate.

@thejpster
Copy link
Contributor

I'll defer to someone like @therealprof or @adamgreig on that one.

@adamgreig
Copy link
Member

Our policy is to make a minor release for breaking changes while <1.0 (per standard Rust semver), so I'm happy to deprecate get_cycle_count now, release that in the next cortex-m 0.7.x release, and remove it in the eventual 0.8.x (or 1.x) release.

This follows the rust C-GETTER API guidelines.
Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

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

Thanks! This all LGTM, but could you update the changelog to reflect the deprecation of get_cycle_count()? Happy to merge after that.

@newAM
Copy link
Member Author

newAM commented Aug 14, 2021

Thanks! This all LGTM, but could you update the changelog to reflect the deprecation of get_cycle_count()? Happy to merge after that.

Whoops, good catch, thanks! I updated the changelog.

Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

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

Thanks!

bors merge

@bors
Copy link
Contributor

bors bot commented Aug 14, 2021

Build succeeded:

@bors bors bot merged commit bb41983 into rust-embedded:master Aug 14, 2021
@adamgreig adamgreig mentioned this pull request Dec 31, 2021
bors bot added a commit that referenced this pull request Jan 2, 2022
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants