-
Notifications
You must be signed in to change notification settings - Fork 190
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
Autodetect xtal-freq #1165
Autodetect xtal-freq #1165
Conversation
197f882
to
88349d9
Compare
aa18174
to
f325ee5
Compare
I think the current PR already solves #1164, but it does not resolve the TODO of implementing We could also implement the |
Probably fine to implement those in a separate PR rather than hold things up here, but your call obviously. I think removing the crystal frequency features is a huge win already! |
Sorry I updated the CI workflow without thinking, so this will need a quick rebase |
f325ee5
to
df8cbbd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!!! Thanks for looking into this, I'm stoked to remove some features :D.
I have a few comments, but nothing major. I don't actually have any 26/24mhz esp's so I can't test it myself, sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides the other comments LGTM - tested with ESP32-C2 26MHz
9a1a737
to
8b8dc03
Compare
151b6c7
to
c278c41
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! LGTM!
xtal-26mhz
andxtal-40mhz
featuresRtcClock::estimate_xtal_frequency()
to select the crystal frequency as suggested in Can we auto detect xtal reliably at runtime? #116433
to be the limit of the two freqs as that is whatesptool
uses (in different scenario, but I think it makes sense to reuse it)Draft until properly tested:
Tests:
RtcClock::estimate_xtal_frequency()
in https://github.com/esp-rs/esp-hal/pull/1165/files#diff-75a5e847ec368b58a227b16ea788a0007a569040ed5793bf8cdda6f37676f0f5R384 and usually is 25 for 26MHz and 38 for 40MHz