Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.2k
esp32/DAC: Add cosine generator capability#5514
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Conversation
The ESP32's DAC has the capability to generate constant waveforms. Thiscommit extends the machine.DAC interface of the ESP32 port to exposethis feature.
Decided to split the call into two such that the RTC clock dividercan be left alone, in case another peripheral is using and settingit already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 anrtc_clk_div method and that conflict with one another because they really adjust the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
"tone tone"
| ESP32 specific DAC class method reference: | ||
| .. method:: DAC.cosine_enable() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why did you decide to break cosine_enable/disable out into their own methods as opposed to simply providing acosine=True/False keyword parameter to themake_new constructor or actually to an init method? This really also applies to thefrequency_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 |
Uh oh!
There was an error while loading.Please reload this page.
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