Allow setting external 32khz quartz for slow rtc clock

[Szybet]

Hello

This is just a working draft for setting the external 32khz quartz for slow rtc clock. I would like to discuss some potential fixes for issues I encountered. The final goal is to get this merged some day, some how.

So to the issues:

• For this to work it is needed to enable CONFIG_RTC_CLK_SRC_EXT_CRYS for the bootloader and rebuild it, reflash it. Otherwise it's simply not turned on and the program tries to calibrate the quartz, so it freezes at that point. The possible fixes are: 2 set of bootloader binaries? A simple note that you need a custom bootloader binary?
• post_init at esp-hal/src/soc/esp32c6/mod.rs turns the RTC on, I don't know why it is the case, but this requires choosing the rtc clock as the default variable in the code, as there is no config system yet. The obvious solution is to not do that? I don't know the reason behind it, but it messes things up. Now, if the user wants the rtc variable, it will be initialized / calibrated 2 times for no reason
• To create the RTC now you need to specify the clock: Rtc::new(LPWR::steal(), RtcSlowClock::default()); this breaks all current examples and programs. In my experience, this will not be an issue 😃

[bugadani]

We might be able to place this into esp_hal::init once #1970 and its follow-up are merged.

[Szybet]

I'm gonna adjust this draft once the other pull request is merged then

[Szybet]

@bugadani I have questions on how to exactly do it

esp-hal/esp-hal/src/lib.rs

Lines 726 to 742 in 08d406e

	/// System configuration.
	#[non_exhaustive]
	#[derive(Default)]
	pub struct Config {
	/// The CPU clock configuration.
	pub cpu_clock: CpuClock,
	}
	/// Initialize the system.
	///
	/// This function sets up the CPU clock and returns the peripherals and clocks.
	pub fn init(config: Config) -> (Peripherals, Clocks<'static>) {
	let peripherals = Peripherals::take();
	let clocks = ClockControl::new(config.cpu_clock).freeze();
	(peripherals, clocks)
	}

Here, I should probably add the enum RtcSlowClock to Config and then configure it in init the question is, should init always return now the Rtc class, even when the user never asked for it (as he probably wanted the default). This will introduce another "Api" change and all examples would need to be adjusted. I don't know, a possible fix for it is a struct that returns those variables instead of returning them manually.

[bugadani]

I'm not very familiar with the rtc API, but I think you should add RtcSlowClock to Config, call rtc::configure_clock here and remove the clock source parameter from Rtc::new. There's no need to return anything more than what we currently do.

[Szybet]

Oh, this will not work at all because post_init still configures the RTC clock with the default RtcSlowClock, there is no way to pass to it the config

[bugadani]

@MabezDev can we get rid of post_init in favour of esp_hal::init?

[Szybet]

Why does it exist in the first place?

[Dominaezzz]

To run code before main but after initialisation. Useful for stuff like psram and until now, watchdog.

Rather than get rid of the post_init , maybe just move the watchdog disabling code to the init function and have that be configurable.
That would help fix #1698 as well.
(Arguably out of scope for this PR but it should solve your problem I think)

[Szybet]

Okay, I did it for the esp32c6 for now. Does it look fine? If yes, I can do it for the rest of the cpu's.

Also I tested it on esp32c6-devkit-m-1.0 with a soldered in xtal 32khz, tested if quartz turns on with an oscilloscope, it did. It required a special bootloader as I said, otherwise it won't output hello world.

[Szybet]

Silence means it's fine, right 😃 ?

[bugadani]

I think so. Please rebase :)

[bugadani]

I don't see the value in this test. For one, what's "a special bootloader" that is required for it? Next, what does this example show, that changing the clock source doesn't break the whole system?

[Szybet]

The bootloader needs to be build with CONFIG_RTC_CLK_SRC_EXT_CRYS=y

Well yes, also allows to measure the sine wave with an osciloscope

[bugadani]

Can we select the clock source here, instead of relying on the bootloader?

[Szybet]

Not without losing some optimization https://github.com/espressif/esp-idf/blob/59e18382702b6986be3d3f55e9ac7763c1397cf7/components/bootloader_support/src/bootloader_clock_init.c#L82-L90

And maybe something more is behind it, I don't know

[bugadani]

if (!rtc_clk_32k_enabled()) {

Unless that takes long to decide, this is safe to do. If the bootloader enabled the clock source, and the user selects in in esp-hap, we can skip initialization. Otherwise, in the future we might allow other bootloaders which may or may not set up the clock source, so I'd say let's enable the clock ourselves, if it's not done already.

[Szybet]

Please note it's inside the macro statement #if CONFIG_ESP_SYSTEM_RTC_EXT_XTAL

Well maybe it is safe but we lose some optimization, I don't think we should lose any performance because at default we don't control the bootloader fully. In the future we probably will by using a rust based bootloader so it will work the same just like now, but without the hassle of recompiling it

[bugadani]

I don't think we should lose any performance because at default we don't control the bootloader fully

Let's prefer the option that works by default. If the user has problems with the startup time, they can use a custom bootloader.

[Szybet]

Making it work only in the application would require changing the code for it to wait to start up, not only is it not efficient but also I don't know how to do it, I looked at a lot of esp-idf code and haven't really found anything that waits for it, I ques booting the bootloader is enough time for it to start after all. Using a fixed time is also bad, would require cpu dependent testing to optimize it.

This is simply not the way to do it...

[bugadani]

Fine. At least please print a warning or panic if the bootloader forgot to enable the clock source.

[Szybet]

Ok, I will try to find a way to detect it (probably just failing the calibration step)

[Szybet]

Does this look fine? It's able to return to the default clock and everything seems to work fine, but I can't guarantee it

Also how do I enable logs from esp-hal, the log feature is already enabled when running examples

[bjoernQ]

To enable logs via esp-println your need to initialize the logger - e.g. esp_println::logger::init_logger_from_env(); early in your code. And you need to specify the log-level via the env variable ESP_LOGLEVEL - e.g. debug. See https://crates.io/crates/esp-println

[Szybet]

To enable logs via esp-println your need to initialize the logger - e.g. esp_println::logger::init_logger_from_env(); early in your code. And you need to specify the log-level via the env variable ESP_LOGLEVEL - e.g. debug. See https://crates.io/crates/esp-println

Shouldn't there be an option / feature for esp-hal to enable it by itself? The logs I want are being executed before user code. Basically show esp-hal logs if the user wants it, because otherwise it's not possible to enable them directly from the user code and I have no idea how early can I put the log init for it to work

Well anyway

Does the esp32c6 code for setting the 32 khz oscillator look fine? If yes, can I implement it for other cpu's. I don't want to rework it later for all of them again

[MabezDev]

Shouldn't there be an option / feature for esp-hal to enable it by itself? The logs I want are being executed before user code. Basically show esp-hal logs if the user wants it, because otherwise it's not possible to enable them directly from the user code and I have no idea how early can I put the log init for it to work

Enable it before calling esp_hal::init() and you'll get the logs. Before main almost nothing else happens, at least nothing interesting.

[MabezDev]

I think this PR should contain the code to initialize the external quartz before we can merge it, depending on a custom bootloader to do it isn't acceptable IMO. Is this something you're willing to work on @Szybet or shall we close this?

[Szybet]

I definitely want to do something with it, but working on a custom rust bootloader? that's out of my league.
We maybe could just do a delay, but it would need to be huge to make it cpu independent. Also that's just a bad solution.

Maybe just accept the custom bootloader? both probe-rs and espflash support a custom bootloader binary argument

[MabezDev]

You don't need to work on a custom bootloader, you just need to port the code from the bootloader that does the initialization to esp-hal.

Maybe just accept the custom bootloader?

As I previously stated, depending on a specific bootloader is not an option. Based on #2376, it's extremely likely we won't be using the esp-idf bootloader at all soon.

If you would like to open a new PR with the initialization built in, we'd gladly accept it!

Thanks for your work so far, but I'm going to close this for now.