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

Create virtual peripherals for CPU control and radio clocks #1428

Merged
merged 4 commits into from
Apr 22, 2024

Conversation

jessebraham
Copy link
Member

A first step towards #1401, the changes get a little more involved from here on out so I figured I'd just split things up to keep it easy to review.

In short, I've created virtual CPU_CTRL and RADIO_CLK peripherals, rather than manually defining the structs like we used to do when splitting.

Happy to hear any feedback regarding peripheral names, I think these are reasonable though.

@jessebraham jessebraham requested review from bjoernQ and MabezDev April 12, 2024 14:30
@jessebraham jessebraham changed the title Created virtual peripherals for CPU control and radio clocks Create virtual peripherals for CPU control and radio clocks Apr 12, 2024
@MabezDev
Copy link
Member

This LGTM, just need to use peripheral ref on the constructors like @bjoernQ said.

@jessebraham
Copy link
Member Author

Sorry forgot on Friday, will get that updated shortly!

@jessebraham jessebraham requested a review from bjoernQ April 15, 2024 18:23
pub struct CpuControl {
_cpu_control: crate::system::CpuControl,
pub struct CpuControl<'d> {
_cpu_control: PeripheralRef<'d, CPU_CTRL>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't really need to store it if we don't have any use for it

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 - the virtual peripherals are really nice

@jessebraham jessebraham marked this pull request as draft April 16, 2024 17:06
@jessebraham
Copy link
Member Author

I'm having second thoughts on merging this before the release, maybe we should wait so that we can tackle the remaining bits of the SYSTEM.split() removal? Not against merging it before the release if anybody else things it's worth doing though.

@jessebraham jessebraham marked this pull request as ready for review April 22, 2024 14:34
@jessebraham
Copy link
Member Author

This should be fine to merge now, I think.

Copy link
Member

@MabezDev MabezDev 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!

@MabezDev MabezDev enabled auto-merge April 22, 2024 14:47
@MabezDev MabezDev added this pull request to the merge queue Apr 22, 2024
@MabezDev MabezDev removed this pull request from the merge queue due to a manual request Apr 22, 2024
@MabezDev MabezDev added this pull request to the merge queue Apr 22, 2024
Merged via the queue into esp-rs:main with commit 4d2ab5b Apr 22, 2024
21 checks passed
@jessebraham jessebraham deleted the refactor/system branch April 22, 2024 15:32
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.

4 participants