Conversation
deadprogram
left a comment
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.
Re-add SAR ADC clock selection (PLL_F80M, div 1) and FSM timing parameters rather than relying on hardware default values.
…ents - Rename PWDET constants and variables to follow camelCase conventions. - Direct pointer initialization for c6PwDetConfReg instead of a custom type wrapper. - Clean up redundant end-of-line comment annotations in InitADC(). - Remove SARADC2 register write in Get() as ESP32-C6 only features ADC1.
|
@deadprogram Thanks for the feedback! I've addressed all of your comments and updated the code accordingly. |
|
Thanks @m-anti for the updates. The notes below are lightly edited from an automated review of the four new commits. Points 2, 5, 6, 7 and 8 from the last review are resolved, and point 4 now has the correct reference. Points 1 and 3 are addressed by the new clock configuration, but that configuration has the problems below. I built The references below come from 1. The PWDET writes go to the wrong register. var c6PwDetConfReg = (*volatile.Register32)(unsafe.Pointer(c6AnaConfigReg))
2. Clock source 2 is RC_FAST, not XTAL.
3. The field has 2 bits (28:27). 4. 5. On the removed |
@deadprogram Sorry... will be updated.
Again, sorry! I will set it to the esp-idf default PLL_F80M.
These are the old values from the existing code. I didn't verify them, but when I use the value 3, the serial output sometimes prints garbage. Using XPD_SAR_FORCE=1, the serial output works. I will check again, but the value 3 does not work with the current code.
I will delete the first one. |
|
For reference, this is a register dump after running a one-shot ADC using esp-idf 5.5.5 on an ESP32-C6. It is interesting that |
2 participants
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.