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

Turn some not-too-useful TODOs into useful documentation #1451

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

bjoernQ
Copy link
Contributor

@bjoernQ bjoernQ commented Apr 16, 2024

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • My changes were added to the CHANGELOG.md in the proper section.

Extra:

Pull Request Details 📖

Description

Some TODOs turned out to be just noise

Testing

Nothing should have changed

@bjoernQ bjoernQ added the skip-changelog No changelog modification needed label Apr 16, 2024
@@ -274,7 +274,6 @@ pub struct Clocks<'d> {
pub pll_48m_clock: HertzU32,
#[cfg(esp32h2)]
pub pll_96m_clock: HertzU32,
// TODO chip specific additional ones as needed
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now a normal process during chip-bring-up

@@ -52,8 +52,6 @@ impl EmbassyTimer {

pub fn init(clocks: &Clocks, mut timer: TimerType) {
// set divider to get a 1mhz clock. APB (80mhz) / 80 = 1mhz...
// TODO: assert APB clock is the source and its at the correct speed for the
Copy link
Contributor Author

@bjoernQ bjoernQ Apr 16, 2024

Choose a reason for hiding this comment

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

TIMG driver doesn't currently support anything else. When that changes we need to revisit all uses anyways

@@ -22,7 +22,12 @@
//! registers with flexible updating methods.
//! * Fault Detection Module (Not yet implemented)
//! * Capture Module (Not yet implemented)
//!
#![doc = ""]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

alternative would be to document it for all chips - I guess having just the right information for the currently selected chip is better?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I've been wondering what we should do about this as well. I guess in this case documenting it on a per-chip basis is simple enough, so this should be fine.

Copy link
Member

Choose a reason for hiding this comment

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

This is the nicest way to do it I've seen so far, so I'm going to steal this method :D. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

FYI it doesn't add any newlines, so you can just inline the bit you want to change, for example:

//! The System Timer is a 
#![cfg_attr(esp32s2, doc = "`64-bit`")]
#![cfg_attr(not(esp32s2), doc = "`54-bit`")]
//! timer with three comparators capable of signalling an alarm interupt on each.

Creates

image

Which is nice.

@@ -647,7 +647,6 @@ impl RtcClock {
(100_000_000 * 1000 / period) as u16
}

// TODO: implement for ESP32-C6, and H2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's already done

Copy link
Member

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

Copy link
Member

@SergioGasquez SergioGasquez 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

@jessebraham jessebraham added this pull request to the merge queue Apr 16, 2024
Merged via the queue into esp-rs:main with commit e368be0 Apr 16, 2024
23 checks passed
@bjoernQ bjoernQ deleted the remove-useless-todo-comments branch November 26, 2024 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No changelog modification needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants