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

Working HAL for the XOSC #25

Merged
merged 7 commits into from
Apr 28, 2021
Merged

Working HAL for the XOSC #25

merged 7 commits into from
Apr 28, 2021

Conversation

Nic0w
Copy link
Contributor

@Nic0w Nic0w commented Apr 24, 2021

Hi,

This is a full, working implementation of a CrystalOscillator that allows use of the XOSC.
Typestate programming is used to transition between the different stages : Disabled > Initialized > Stable > Disabling, Dormant.

I've tested the code on my Pi Pico up to the first 3 stages (ie up to Stable) and confirmed with the Frequency Counter that the XOSC's frequency is indeed 12MHz at that point.

To be noted that the code is based on hardware_xosc/xosc.c from the official pico-sdk.

I also want to point out that I'm a novice in Rust and that this is my first ever contribution to a Rust crate, so don't hesitate to point out things that I could have done better.

Regards,

Copy link
Contributor

@tdittr tdittr left a comment

Choose a reason for hiding this comment

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

Hi,

thanks for taking up the work on the Oscillator. I started work on this previously, but I think you picked up from there anyway?

Overall this PR looks really good. I would just change a few minor details and maybe not yet expose the Dormant mode.

rp2040-hal/src/lib.rs Outdated Show resolved Hide resolved
rp2040-hal/src/xosc.rs Outdated Show resolved Hide resolved
rp2040-hal/src/xosc.rs Outdated Show resolved Hide resolved
rp2040-hal/src/xosc.rs Outdated Show resolved Hide resolved
rp2040-hal/src/xosc.rs Show resolved Hide resolved
rp2040-hal/src/xosc.rs Show resolved Hide resolved
rp2040-hal/src/xosc.rs Outdated Show resolved Hide resolved
rp2040-hal/src/xosc.rs Outdated Show resolved Hide resolved
rp2040-hal/src/xosc.rs Outdated Show resolved Hide resolved
rp2040-hal/src/xosc.rs Outdated Show resolved Hide resolved
@Nic0w
Copy link
Contributor Author

Nic0w commented Apr 24, 2021

Hi,

thanks for taking up the work on the Oscillator. I started work on this previously, but I think you picked up from there anyway?

Overall this PR looks really good. I would just change a few minor details and maybe not yet expose the Dormant mode.

Hi,

Thanks for your feedback and code review =) I read extensively your PR and I saw you tried to bind nicely together Clocks, PLLs, Oscillators, ... with extensive typestates and such. I then had a brain meltdown trying to think about how to pick up your work; not criticizing your code (from which I learned a lot) but the complexity of the clocks/PLL/oscillators subsystems and the complexity of "bridging" them together, with all the different abstractions that would be needed : macros, typestate, ...

So I decided on starting from scratch, taking inspiration from your PR as you saw (StableOscillatorToken for example) with the goal of trying to implement one subsystem at a time, starting at the bottom of the foodchain : XOSC.

Not sure if it's the right approach but I figured that it might be easier that way =)

@tdittr
Copy link
Contributor

tdittr commented Apr 24, 2021

Oh I can feel you, I had the same brain-meltdown. I think I bit off a bit to much of the problem. Writing it in smaller steps definitely is a good idea. 👍

Maybe at some point in the future when we have all the building blocks we can actually tie them together.

@tdittr tdittr mentioned this pull request Apr 24, 2021
@Nic0w
Copy link
Contributor Author

Nic0w commented Apr 25, 2021

This is ready for review again :)

Copy link
Contributor

@tdittr tdittr left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍

Copy link
Member

@9names 9names left a comment

Choose a reason for hiding this comment

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

Looks good to me too.
After merging, it would be nice to get an example showing how to use this correctly.

@9names 9names merged commit f728de5 into rp-rs:main Apr 28, 2021
ithinuel pushed a commit to ithinuel/rp-hal that referenced this pull request Aug 15, 2023
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.

3 participants