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

Unify the low-power peripheral names and rename the RtcCntl driver #872

Closed
jessebraham opened this issue Oct 25, 2023 · 5 comments · Fixed by #1064
Closed

Unify the low-power peripheral names and rename the RtcCntl driver #872

jessebraham opened this issue Oct 25, 2023 · 5 comments · Fixed by #1064
Assignees
Labels
RFC Request for comment

Comments

@jessebraham
Copy link
Member

At present, depending on which chip you're targeting, this driver is initialized in one of two ways:

// esp32c6 & esp32h2:
let rtc_cntl = RtcCntl::new(peripherals.LP_CLKRST);

// Everything else:
let rtc_cntl = RtcCntl::new(peripherals.RTC_CNTL);

There are two major issues with our current implementation:

  1. The peripheral singleton structs should have their names unified; it's confusing passing two different peripherals to the same constructor, depending on which chip you're using. It's also confusing that for the C6 and H2 the peripheral name is seemingly unrelated to the driver's name.
  2. RtcCntl is just not a good name. This was chosen initially as it was used in the TRMs, however with the new naming schemes this is less relevant now. This name is also, IMO, incredibly misleading, as may people associate it with Real-Time Clocks instead.

So the question remains, what should we call these things?

@jessebraham jessebraham added the RFC Request for comment label Oct 25, 2023
@bugadani
Copy link
Contributor

For me, anything goes as long as it's not RTC. Let me throw LP_CNTL in the pool of possibilities :)

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 25, 2023

I double that - anything not containing RTC is fine to me

@jessebraham
Copy link
Member Author

Is LP_CLKRST or even just CLKRST maybe not a bad name to use? It's at least consistent with some chips this way. Just a thought.

@bugadani
Copy link
Contributor

I think LP_CNTL is broad enough so that it covers the power domain stuff, too, that I think belongs to RTC in the S3. CLKRST is somewhat more ... concrete and may not be precise enough because of that.

@MabezDev
Copy link
Member

Throwing my suggestion into the ring:

LPWR and LowPowerDomain

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request for comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants