Skip to content

Conversation

@jessebraham
Copy link
Member

@jessebraham jessebraham commented Sep 25, 2023

I have been meaning to do this for some time now, finally got around to it.

Rather than exposing the PeripheralClockControl struck via the SystemParts struct, the drivers just handle this all internally. Since PeripheralClockControl was already a ZST, I've just removed the &mut self argument of the enable function and instead converted it to an associated function. This type's scope was also limited to the crate-level.

This is obviously a pretty breaking change, but it's more than worth it IMO.

These changes were pretty straight forward, but also fairly invasive, so hopefully I haven't broken anything! If anybody has any questions/concerns/suggestions please let me know!

Closes #776

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.

one nit pick, but LGTM otherwise!

@bjoernQ
Copy link
Contributor

bjoernQ commented Sep 25, 2023

Probably enable for targets other than C6/H2 should be done in a critical section since at least in theory it could get interrupted between reading a register and writing it. If in an interrupt another peripheral gets enabled we might write back a bad value

Not very likely t.b.h. but at least we don't prevent it anymore

@jessebraham
Copy link
Member Author

Sorry I'm not quite sure I follow, what do you mean by "we don't prevent it anymore"? What were we doing before exactly that we're not doing now?

@bjoernQ
Copy link
Contributor

bjoernQ commented Sep 25, 2023

Before the user had to pass a mutable reference to SystemClockControl - there can be always only one at a time so access had to be serialized before

Maybe there is already something to prevent it in this PR ... using a smartphone right now - not ideal to look at this amount of changes

@jessebraham
Copy link
Member Author

Oh I see what you mean now, sorry about that.

@MabezDev
Copy link
Member

Before the user had to pass a mutable reference to SystemClockControl - there can be always only one at a time so access had to be serialized before

Maybe there is already something to prevent it in this PR ... using a smartphone right now - not ideal to look at this amount of changes

The desktop page doesn't work much better either :D, so laggy.

@AnthonyGrondin
Copy link
Contributor

Thank you.

This should fix #776

@bugadani
Copy link
Contributor

I always appreciate changes where the update process is one step:

  • delete code

fairly invasive, so hopefully I haven't broken anything!

I'd love to try this in my project, but unfortunately, I am unable to because of two separate build errors introduced recently into the HAL. (#818)

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 - I still think there should be a critical-section for non C6/H2 however

@jessebraham jessebraham merged commit 0064766 into esp-rs:main Sep 26, 2023
@jessebraham jessebraham deleted the feature/pcc branch September 26, 2023 16:13
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.

Provide a way to enable Peripherals without direct access to PeripheralClockControl

5 participants