esp-hal icon indicating copy to clipboard operation
esp-hal copied to clipboard

Allow setting external 32khz quartz for slow rtc clock

Open Szybet opened this issue 1 year ago • 17 comments

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 :smiley:

Szybet avatar Aug 29 '24 13:08 Szybet

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

bugadani avatar Aug 30 '24 15:08 bugadani

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

Szybet avatar Sep 02 '24 07:09 Szybet

@bugadani I have questions on how to exactly do it

https://github.com/esp-rs/esp-hal/blob/08d406ee2b240384610bea8199d444f44942f157/esp-hal/src/lib.rs#L726-L742

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.

Szybet avatar Sep 03 '24 06:09 Szybet

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.

bugadani avatar Sep 03 '24 07:09 bugadani

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

Szybet avatar Sep 03 '24 07:09 Szybet

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

bugadani avatar Sep 03 '24 08:09 bugadani

Why does it exist in the first place?

Szybet avatar Sep 03 '24 08:09 Szybet

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 https://github.com/esp-rs/esp-hal/issues/1698 as well. (Arguably out of scope for this PR but it should solve your problem I think)

Dominaezzz avatar Sep 03 '24 09:09 Dominaezzz

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 avatar Sep 04 '24 09:09 Szybet

Silence means it's fine, right :smiley: ?

Szybet avatar Sep 06 '24 05:09 Szybet

I think so. Please rebase :)

bugadani avatar Sep 06 '24 06:09 bugadani

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

Szybet avatar Sep 09 '24 12:09 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

bjoernQ avatar Sep 10 '24 06:09 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

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

Szybet avatar Sep 20 '24 09:09 Szybet

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 avatar Sep 20 '24 15:09 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?

MabezDev avatar Oct 21 '24 13:10 MabezDev

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

Szybet avatar Oct 21 '24 14:10 Szybet

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.

MabezDev avatar Nov 14 '24 10:11 MabezDev