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

Fix delay on esp32h2. #1535

Merged
merged 3 commits into from
May 3, 2024
Merged

Fix delay on esp32h2. #1535

merged 3 commits into from
May 3, 2024

Conversation

playfulFence
Copy link
Contributor

closes #1509

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • My changes were added to the CHANGELOG.md in the proper section.

Pull Request Details 📖

Description

Сoefficient for getting 16MHz in Delay from xtal_clock (2.5) wrong because it's related for 40MHz xtal, when esp32h2's is 32MHz

image

Testing

Tested delay time with DSLab LA, it's 500-501ms now.

@bjoernQ
Copy link
Contributor

bjoernQ commented May 3, 2024

Thanks! Code looks good and fixes the issue

Now thinking about it I wonder why we are not using Systimer::TICKS_PER_SECOND to know the frequency?
Shouldn't we also have an issue with the 26 MHz C2?

I thought we should also have issues on S2 but apparently, we use the CPU's timer on Xtensa (i.e. no issue there) - which makes we wonder if we should use systimer everywhere it's available (i.e. not on ESP32)

Copy link
Member

@SergioGasquez SergioGasquez 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 fixing this! Do you mind enabling the get_time test on H2 so we test it moving forward?

@SergioGasquez
Copy link
Member

Shouldn't we also have an issue with the 26 MHz C2?

Maybe we do have them, this morning I tried locally the test suite on a 26 MHz C2 and the get_time test was failing IIRC, I didnt investigate anything as I just wanted to confirm that pins of other tests were working fine 🤔

@playfulFence
Copy link
Contributor Author

Thanks for fixing this! Do you mind enabling the get_time test on H2 so we test it moving forward?

Done!

@jessebraham
Copy link
Member

Now thinking about it I wonder why we are not using Systimer::TICKS_PER_SECOND to know the frequency? Shouldn't we also have an issue with the 26 MHz C2?

I thought we should also have issues on S2 but apparently, we use the CPU's timer on Xtensa (i.e. no issue there) - which makes we wonder if we should use systimer everywhere it's available (i.e. not on ESP32)

Probably worth discussing, but this PR can be merged regardless I think so let's either open an issue or make a note to discuss it in our next meeting.

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.

Good catch, thanks for the fix!

@jessebraham jessebraham enabled auto-merge May 3, 2024 16:51
Copy link
Member

@SergioGasquez SergioGasquez 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

@jessebraham jessebraham added this pull request to the merge queue May 3, 2024
@SergioGasquez
Copy link
Member

SergioGasquez commented May 3, 2024

Not sure what happened, but HIL on S3 failed to build the tests: https://github.com/esp-rs/esp-hal/actions/runs/8942444773/job/24565046328

Edit: CI succeeded doing the same: https://github.com/esp-rs/esp-hal/actions/runs/8942444770/job/24565051849 werid

@jessebraham
Copy link
Member

I've seen that happen before I think, not sure what causes it. Really weird.

Merged via the queue into esp-rs:main with commit 209a82b May 3, 2024
22 checks passed
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.

ESP32-H2: Delay::delay_millis delays too short
4 participants