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 RealTimeClock & UsbBus ownership #725

Merged
merged 2 commits into from
Dec 14, 2023
Merged

Conversation

jnthbdn
Copy link
Contributor

@jnthbdn jnthbdn commented Dec 3, 2023

The RealTimeClock works great, but it takes ownership of the rtc_clock field in the ClockManager, which makes it invalid (partial move).
Moreover, the RealTimeClock struct doesn't allow freeing rtc_clock, the new function doesn't do anything about it, so the instance is destroyed.

Following the model of other functions present in the HAL for clock configuration (such as setup_pll_blocking or configure_clock from ClockManger), I modified the parameter so that it was a reference.

#724

Thanks for this brilliant HAL!

@ithinuel ithinuel self-requested a review December 3, 2023 12:51
Copy link
Member

@ithinuel ithinuel left a comment

Choose a reason for hiding this comment

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

Would it make sense to make RealTimeClock only take a frequency here instead?

A benefit of expecting the RtcClock is that at least it implicitly checks one's using the right clock source; but it doesn't tie them together either.
Nothing prevent that clock source to be reconfigured which would render anything the RealTimeClock inconsistent.
So since the benefit provided by using &RtcClock is pretty thin, is it worth keeping at all?

cc @thejpster @9names @jannic

@jnthbdn
Copy link
Contributor Author

jnthbdn commented Dec 3, 2023

Thanks for your review, I quite agree with your comment, it would make the function's intentions clearer.

But the more I look at the code, the more I wonder if my PR is really relevant...

Let me explain, as you said in issue #724, it's interesting that ownership is taken in order to ensure that clocks are not subsequently modified. But there's still the problem of the ClockManager being partially moved. So in the end, even though it's a big change, wouldn't it be better to make the ClockManager fields Option<T>?
Changing this line

pub [<$name:snake>]: $name,

with

pub [<$name:snake>]: Option<$name>

Because in this PR we've solved the RealTimeClock problem, but the new function in the UsbBus does the same thing.

_pll: UsbClock,

@ithinuel
Copy link
Member

ithinuel commented Dec 3, 2023

Would it make sense to rather than having fields public, adding a split method and include as part of its output a extra entry offering a subset of what the clock manager does but that does not collide with the individual clocks ?

@jnthbdn
Copy link
Contributor Author

jnthbdn commented Dec 11, 2023

I modified the PR by adding free functions to the RealTimeClock and UsbBus structs.

@jnthbdn jnthbdn changed the title Fix RealTimeClock ownership Fix RealTimeClock & UsbBus ownership Dec 11, 2023
Copy link
Member

@ithinuel ithinuel left a comment

Choose a reason for hiding this comment

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

LGTM

@jannic jannic merged commit 85474db into rp-rs:main Dec 14, 2023
8 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.

None yet

3 participants