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

update HC32 timer rates #27086

Merged
merged 5 commits into from
May 15, 2024

Conversation

shadow578
Copy link
Contributor

Description

As per the comment by @sjasonsmith in #26845 (comment), i've taken a closer look at the timer rates set in the HC32 HAL and updated them according to the comment.

  1. F_CPU is now constexpr and matches HCLK, the rate of the main CPU core. The comment has been updated accordingly.
  2. CYCLES_PER_MICROSECOND was removed to fallback to the default.
  3. HAL_TIMER_RATE remains unchanged since it's used in the calculation for STEPPER_TIMER_RATE. The comment has been updated accordingly.

Requirements

HC32-based printer

Benefits

Timing on HC32 should now function closer to what Marlin actually expects.
While i haven't observed a difference in e.g. print quality, this should still be a good thing.

Configurations

N/A

Related Issues

N/A

@thinkyhead thinkyhead added PR: Bug Fix T: HAL & APIs Topic related to the HAL and internal APIs. labels May 15, 2024
@thinkyhead
Copy link
Member

I added a compile-time warning about the hard-coded CPU clock, in prep for some future HC32 at a different clock rate.

Would it make sense to define sysclock this way?

constexpr uint32_t sysclock = F_CPU;

@thinkyhead thinkyhead merged commit f320c2a into MarlinFirmware:bugfix-2.1.x May 15, 2024
62 checks passed
@shadow578
Copy link
Contributor Author

Hi @thinkyhead ,

sadly some of the changes you've made may be incorrect:

  1. constexpr uint32_t sysclock = F_CPU; is not correct: sysclock is the output of the PLL that HCLK (the actual F_CPU) is derived from. While both are set to 200 MHz, it may be confusing to set sysclk to F_CPU, since it's what defined sysclk.
  2. same for HAL_TIMER_RATE, it too is derived from sysclock, not HCLK / F_CPU.
  3. the warning "HC32 clock is assumed to be 200MHz. If this isn't the case for your board please submit a report so we can add support." doesn't really make sense in the context of HC32. On HC32, all clocks are derived from a PLL, with a fallback to a internal RC oscillator. Thus, it will never exist a board where the clock cannot be made to be 200 MHz.

Should another PR be created to update this ?


As an aside, i've attached a marked up block diagram of the HC32s clock circuitry (Section 4.2.1 of The Reference Manual).
Marked in yellow is the (happy-) path using a external 8 / 16 MHz crystal oscillator as PLL input. Magenta is the fallback PLL input using the HRC (16 MHz internal RC oscillator).

clock block diagram

@thisiskeithb
Copy link
Member

Should another PR be created to update this ?

Since this PR is already merged, another will need to be submitted with your corrections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Bug Fix T: HAL & APIs Topic related to the HAL and internal APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants