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

Switch RMT clock source to 1MHz REF #22

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

RadekHvizdos
Copy link

@RadekHvizdos RadekHvizdos commented Mar 31, 2021

Hello,

When using Dynamic Frequency Scaling to reduce power consumption, the APB clock used by RMT changes between max_freq_mhz and min_freq_mhz (typicaly between 80 and 10MHz). This causes errors when using the RMT peripheral for 1wire communication.

We could use either the 1Mhz REF or 40Mhz XTAL clock sources instead, they are stable even during DFS operation:

#if SOC_RMT_SUPPORT_REF_TICK
    RMT_BASECLK_REF = 0, /*!< RMT source clock is REF_TICK, 1MHz by default */
#endif
    RMT_BASECLK_APB = 1, /*!< RMT source clock is APB CLK, 80Mhz by default */
#if SOC_RMT_SUPPORT_XTAL
    RMT_BASECLK_XTAL = 3, /*!< RMT source clock is XTAL clock, 40Mhz by default */
#endif

Here is the availability of various clock sources depending on target device:

RMT_BASECLK_REF RMT_BASECLK_APB RMT_BASECLK_XTAL
ESP32 ESP32
ESP32-S2 ESP32-S2
ESP32-S3 ESP32-S3
ESP32-C3 ESP32-C3

I am a bit puzzled that the documentation still states:
REF tick clock, which would be 1Mhz (not supported in this version).
When using this has clear benefits. Perhaps they just forgot to remove this?

We should probably have the clock source selected automatically based on the target device, not hard-coded. My patch is just for demonstration purposes.

What do you think?

When using Dynamic Frequency Scaling, the APB clock changes between max_freq_mhz and min_freq_mhz. This will create errors when using the RMT peripheral for 1wire communication.

Use 1Mhz REF clock source instead, it is stable even during DFS operation.
@DavidAntliff
Copy link
Owner

DavidAntliff commented Mar 31, 2021

Hi @RadekHvizdos, thank you for your suggestion. This seems to me like a reasonable precursor to a proper change, however I'm not able to test this because I only have access to the original ESP32 device.

Therefore I'd be happy to include something like this if you or someone else is able to write a target-specific clock selection routine and test OWB operation on all four(?) chips.

Am I right in my interpretation of your table that the code would simply choose between RMT_BASECLK_REF and RMT_BASECLK_XTAL depending on the device type? Then various timing factors would need to be set based on the expected clock frequency, I assume?

Can you link me to the docs that say "REF tick clock, which would be 1Mhz (not supported in this version)." please?

@RadekHvizdos
Copy link
Author

Hi @DavidAntliff,

Here is a link to the relevant section in the documentation:
https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/peripherals/rmt.html#_CPPv418rmt_set_source_clk13rmt_channel_t16rmt_source_clk_t

I found this forum post which clarifies the situation a little bit. The explanation makes me feel a bit better about using the REF clock:
https://www.esp32.com/viewtopic.php?t=3011

I will amend the patch with device selection code and then we can discuss it further.

Adding logic which should use the best clock source available on each device. The capability is flagged in esp-idf's soc_caps.h file, which is different to each device family.
@RadekHvizdos
Copy link
Author

See the latest patch which implements the clock selection logic.

When testing it I have found that:

  • The official support for RMT REF clock in ESP32 seems to be coming in esp-idf 4.3 👍
  • It still works with 4.2 if you force define SOC_RMT_SUPPORT_REF_TICK
  • The clock selection code gracefully falls back to APB clock for esp-idf <= 4.2

I was only able to test with plain ESP32, but I am confident that implementing the logic will not introduce any regressions. I have tested both esp-idf 4.2 and latest 4.3.

@DavidAntliff
Copy link
Owner

Ok, thanks, I'll go over this as soon as I can (might be a few days at least before I get a chance).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants