Paciente8159 / uCNC

µCNC - Universal CNC firmware for microcontrollers
https://github.com/Paciente8159/uCNC/wiki
GNU General Public License v3.0
265 stars 60 forks source link

Updated MCU HAL to support other chips in the ESP32 family #649

Closed lonelycorn closed 6 months ago

lonelycorn commented 6 months ago

in this pull request, the MCU HALL to support other chips in the ESP32 family which have different capabilities than ESP32:

Paciente8159 commented 6 months ago

Awesome job. Thanks.

Just one question regarding ADC. Wouldn't be preferable to simply set the ADC to ADC_WIDTH_BIT_12 and then bit-shift 2 bits? The SDK seems to indicate the higher bit-width ADC are available.

    ADC_WIDTH_BIT_9  = 0, /*!< ADC capture width is 9Bit. */
    ADC_WIDTH_BIT_10 = 1, /*!< ADC capture width is 10Bit. */
    ADC_WIDTH_BIT_11 = 2, /*!< ADC capture width is 11Bit. */
    ADC_WIDTH_BIT_12 = 3, /*!< ADC capture width is 12Bit. */
#elif SOC_ADC_MAX_BITWIDTH == 12
    ADC_WIDTH_BIT_12 = 3, /*!< ADC capture width is 12Bit. */
#elif SOC_ADC_MAX_BITWIDTH == 13
    ADC_WIDTH_BIT_13 = 4, /*!< ADC capture width is 13Bit. */
#endif
    ADC_WIDTH_MAX,

This is transversal to ESP32, ESP32C3, ESP32S2 and ESP32S3. They all seem to support the ADC_WIDTH_BIT_12 definition. Are newer variants that have other definitions?

lonelycorn commented 6 months ago

@Paciente8159 I was just getting started with ESP32 family and playing with a ESP32S3 dev board which only supports 12-bit ADC. I also read that "10bit ADC" is sort of a "breaking change" introduced in v1.8. So I decided to keep the output of mcu_get_analog() in the same range of [0, 4095] just in case.

I did not verify if ADC_WIDTH_BIT_12 was available on all current MCU's of this family. That said, the logic implemented in this PR should work regardless, which IMO is a bit more future-proof?

Paciente8159 commented 6 months ago

Yes. I applied the same logic to attenuation too. Thanks