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 ETM support for general purpose timers #1287

Merged
merged 18 commits into from
Mar 15, 2024
Merged

Add ETM support for general purpose timers #1287

merged 18 commits into from
Mar 15, 2024

Conversation

sisoteuz
Copy link
Contributor

@sisoteuz sisoteuz commented Mar 14, 2024

Implement Event Task Matrix functions for general purpose timers. ETM is only available for esp32-c6 and esp32-h2 chips.

Closes #1274

Must

  • The code compiles without errors or warnings.
  • All examples work.
  • cargo fmt was run.
  • Your changes were added to the CHANGELOG.md in the proper section.
  • You updated existing examples or added examples (if applicable).
  • Added examples are checked in CI

Nice to have

  • You add a description of your work to this PR.
  • You added proper docs for your newly added features and code.

@sisoteuz
Copy link
Contributor Author

I will explore using macros to generate code for each timer instead of introducing this const G: u8 parameter on the different traits like I did in this draft.

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 14, 2024

Thanks for working on this!

I'd say introducing the const generic is something we'd like to avoid.

One solution might be to add a function like fn id() -> u8; to the TimerGroupInstance trait and implement the function for TIMG0/TIMG1 to return 0 or 1. The compiler should optimize all that away so it won't introduce runtime cost

Not sure if that's expected (since it's a draft PR) but the example doesn't work for me, yet.

@sisoteuz
Copy link
Contributor Author

sisoteuz commented Mar 14, 2024

One solution might be to add a function like fn id() -> u8; to the TimerGroupInstance trait and implement the function for TIMG0/TIMG1 to return 0 or 1. The compiler should optimize all that away so it won't introduce runtime cost

I actually started with doing that and ditching it because I was aiming to introduce no extra runtime cost. But seems it’s fine so I’ll definitely give it a go. Thanks for the suggestion !

Not sure if that's expected (since it's a draft PR) but the example doesn't work for me, yet.

I will double check as I tested late last night on my dev board, might very well be that I missed to push something.

@sisoteuz sisoteuz marked this pull request as ready for review March 15, 2024 01:50
@sisoteuz sisoteuz marked this pull request as draft March 15, 2024 01:51
@sisoteuz
Copy link
Contributor Author

sisoteuz commented Mar 15, 2024

I can confirm that the implementation is working on my esp32-c6 board with the example etm_timer.rs. The example sets up ETM channel 0 to stop Timer0 on alarm that we configure to trigger after 100ms the counter starts.

The output is as below :

counter in interrupt: 10    # Alarm triggered the interrupt after 100ms and reset the counter (default behaviour)
counter in main: 10         # Counter got stopped by ETM (new feature)
counter in main: 10
counter in main: 10
counter in main: 10
counter in main: 10
counter in main: 10

I couldn't test on esp32-h2 as I don't own one. However looking at the technical reference document the ETM tasks and events IDs are the same so in theory it should work.

Now 2 checks are failing apparently because of the use of delay.delay_ms(500u32) in my example and not importing the DelayMs trait (I don't see why that would be necessary).

I can successfully build all the examples locally and I'm using Delay like in all the other examples. Really not sure why it is failing, I'd really appreciate any help as this failure doesn't make a lot of sense to me.

Thanks.

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 15, 2024

I tested on ESP32-H2 and it works

For the delay: there were some changes lately - probably easiest would be to use delay_millis instead of delay_ms

Otherwise looks good to me

@sisoteuz sisoteuz marked this pull request as ready for review March 15, 2024 10:28
@sisoteuz
Copy link
Contributor Author

For the delay: there were some changes lately - probably easiest would be to use delay_millis instead of delay_ms

Thanks that fixed the issue. I moved the PR to ready for review.

Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, this LGTM!

@bjoernQ is more familiar with this particular peripheral, so I will wait for him to give this his stamp of approval as well.

@sisoteuz
Copy link
Contributor Author

Thanks ! I had a blast making this contribution.

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks for your contribution!

@bjoernQ bjoernQ added this pull request to the merge queue Mar 15, 2024
Merged via the queue into esp-rs:main with commit 1846543 Mar 15, 2024
18 checks passed
yanshay pushed a commit to yanshay/esp-hal that referenced this pull request Mar 16, 2024
* Implement ETM for general purpose timers

* Revert changes to timer_interrupt example

* Add an example for ETM for general purpose timers

* Add newly introduced generic const param for Timer0

* Use id function on TimerGroupInstance instead of const parameter

* Revert const parameter on Timer1

* Specify supported chips

* Update CHANGELOG.md

* Update CHANGELOG.md

* Fix example

* Assign configured ETM channel to a variable to prevent drop

* Fix comments

* Move declaration of delay after critical section

* Use delay_millis instead of delay_ms

* Remove mut from delay

* Add documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ETM tasks and events for timers
3 participants