Conversation
deadprogram
left a comment
There was a problem hiding this comment.
Thanks @m-anti for working on this. The notes below are lightly edited from an automated review.
General
The approach looks correct. The previous code did not enable the modem FE clock or the PWDET SAR power path, thus the SAR analog block stayed off and the INT_RAW loop in Get() continued without end. The new sequence agrees with sar_periph_ctrl_init() in ESP-IDF. The order in InitADC is correct, because the PWDET register is in the modem address range and needs the modem clock first.
Points to examine
1. Removed clock configuration. The previous code set PCR.saradc_conf.saradc_clk_en, saradc_clkm_sel = 2 (PLL_F80M), and the divider fields. This PR sets only saradc_reg_clk_en and saradc_clkm_en. In ESP-IDF, adc_ll_digi_clk_sel() sets saradc_clkm_en and also saradc_clk_en, together with the clock source. Please confirm that the removal is intended and that the reset values are sufficient. Dependence on reset values is not safe.
2. Old comment. The InitADC comment continues to tell that "the SARADC CLKM divider configuration also lives in PCR", but the divider configuration is removed.
3. The new delay comment depends on the removed configuration. In Get():
// No delay needed here because adc_ctrl_clk is fast (>= APB_CLK_FREQ/8).ESP-IDF calculates this delay from the configured clock source and divider. This PR does not configure either one, thus the statement depends on the reset values. Set the clock source again, or change the comment to tell which clock it assumes.
4. The reference does not point to the correct file. c6PWDET_CONF_REG = 0x600A0810 and the two bit masks refer to soc/esp32c6/register/soc/reg_base.h, which contains only base addresses. The offset and the PWDET_LL_SAR_POWER_FORCE and _CNTL bits are in hal/esp32c6/include/hal/sar_ctrl_ll.h. Please add a reference to the definitive source.
5. Quantity of comments. Approximately 40 lines have a C statement that repeats the Go line above it, for example // PCR.saradc_conf.saradc_reg_clk_en = 1 and // hw->clk_conf_power_st.clk_wifi_st_map. It is best to omit extra redundant comments. One reference to the ESP-IDF source for each function gives the same information.
6. c6PWDET_Type is more than necessary. For one register, the same file already uses this form:
cfg := (*volatile.Register32)(unsafe.Pointer(c6AnaConfigReg))Also, c6PWDET_Type, c6PWDET_CONF_REG, and c6PWDET_LL_SAR_POWER_FORCE_BIT use upper case with underscores, but the other names in the file use the c6AnaConfigReg and c6SarForcePD form.
7. panic("unhandled domain") in two switch statements. The loop sends only the domains 0 to Max-1, and each one has a case. Thus the panic cannot occur, but it uses flash in the machine package. A return removes it.
8. initialGatingMode is a var and uses 40 bytes of RAM. A const switch keeps the data in flash. Saves some memory.
9. Two small items. SetONETIME_SAMPLE_SARADC2_ONETIME_SAMPLE(0) has no effect, because the ESP32-C6 has no ADC2, as a comment in the file tells. Also, modemClockModuleEnableForADC writes the ICG maps for the WiFi, BT, and 802.15.4 domains, with no count of users and no disable function. ESP-IDF counts the users, because other drivers use these domains. There is no radio support for the C6 in TinyGo yet, thus there is no effect, but a comment in the code would help.
Fixes #5647
Initialize ADC and read values using an ESP32-C6. The ADC driver setup
code was extracted and ported from a working ESP-IDF C program.