esp32/DAC: Add cosine generator capability #5514
Conversation
The ESP32's DAC has the capability to generate constant waveforms. This commit extends the machine.DAC interface of the ESP32 port to expose this feature.
Decided to split the call into two such that the RTC clock divider can be left alone, in case another peripheral is using and setting it already.
There was a problem hiding this comment.
Cool! I'm looking forward to using this, I also appreciate that you wrote docs! :-)
The main issue I see is that the driver doesn't follow the model of the others I've seen WRT initialization. In general, I see an init method that takes a slew of KW args to set all the features and a constructor that accepts the same set of KW args (usually all handled in one init_helper function). I would prefer new drivers to follow this model, but I don't really know what the general strategy is.
Unit tests?
| # frequency steps for the CW generator, (7, 1) -> ~15 Hz | ||
|
|
||
|
|
||
| ESP32 specific DAC class method reference: |
There was a problem hiding this comment.
This doc is just the quick reference. You should place the detailed docs into docs/library/esp32.rst - IMHO
There was a problem hiding this comment.
Hmm, I mirrored what the ADC does? Should that be moved too?
| ``clk_8m_div`` is the divider and must be between 0 and 7. | ||
|
|
||
| .. Warning:: | ||
| Be cautious in changing this value as it might affect other peripherals that use the 8 MHz RTC clock! |
There was a problem hiding this comment.
Should this method be in a different class if it affects the system globally? E.g. ESP32.rtc_clk_div? I know it's more work, but it would be awful to have 2-3 peripherals that each have an rtc_clk_div method and that conflict with one another because they really adjust the same thing.
There was a problem hiding this comment.
Agree. Will move that over and make a note about it in the reference for the frequency set function
| SET_PERI_REG_MASK(SENS_SAR_DAC_CTRL1_REG, SENS_SW_TONE_EN); | ||
| switch(self->dac_id) { | ||
| case DAC_CHANNEL_1: | ||
| // Enable / connect tone tone generator on / to this channel |
|
|
||
| ESP32 specific DAC class method reference: | ||
|
|
||
| .. method:: DAC.cosine_enable() |
There was a problem hiding this comment.
Why did you decide to break cosine_enable/disable out into their own methods as opposed to simply providing a cosine=True/False keyword parameter to the make_new constructor or actually to an init method? This really also applies to the frequency_step method. Unless these get called while the DAC is operating, they would be simpler just as KW args in the constructor/init IMHO.
There was a problem hiding this comment.
Will have to do both as you can enable/disable this feature during run-time.
|
This is an automated heads-up that we've just merged a Pull Request See #13763 A search suggests this PR might apply the STATIC macro to some C code. If it Although this is an automated message, feel free to @-reply to me directly if |
The ESP32's DAC has the capability to generate constant waveforms. This
commit extends the machine.DAC interface of the ESP32 port to expose
this feature.
TODO:
rtc_clk_divto esp library