-
Notifications
You must be signed in to change notification settings - Fork 133
Add esp_dac, for now only with oneshot mode #1998
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: schnittchen <[email protected]>
Signed-off-by: schnittchen <[email protected]>
bettio
left a comment
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.
Did you take the code verbatim from the component, or did you apply any change? If so, can you let me know which changes?
Also, let's add this to our sdkconfig so we can make it optional when building AtomVM from scratch.
|
|
||
| // References | ||
| // https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-reference/peripherals/dac.html | ||
|
|
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.
Let's make build optional:
#include <sdkconfig.h>
#ifdef CONFIG_AVM_ENABLE_ADC_NIFSThere 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 is intended to make it possible to select/deselect this functionality in AtomVM, correct? If so, there probably is a "config layer" that this PR should also modify, where would that be?
Looking at the build matrix, all esp32c3 and p4 builds are failing, I assume because these models lack an DAC (The product matrix filtering on https://products.espressif.com/#/product-selector seems broken, so this is just a guess). Is there a mechanism to handle this?
I am also seeing this error
HINT: The component esp_driver_dac could not be found. This could be because:...
for example here:
https://github.com/atomvm/AtomVM/actions/runs/19595789857/job/56245058954?pr=1998
My guess is that my CMakeList.txt changes don't work here. I don't know how to deal with this case.
| return NULL; | ||
| }; | ||
|
|
||
| REGISTER_NIF_COLLECTION(atomvm_dac, atomvm_dac_init, NULL, atomvm_dac_get_nif) |
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.
#endif /* CONFIG_AVM_ENABLE_ADC_NIFS */
I don't understand what you mean here, which component? |
I saw the following copyright: So my assumption is that this PR is based on this code: https://github.com/atomvm/atomvm_adc . Am I right? If so, what kind of changes did you do to it? |
bettio
left a comment
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.
In order to merge this code this issue has to be fixed: esp_dac.erl:19: Call to missing or unexported function esp_dac:oneshot_new_channel_p/1
Ah, thanks for clarifying. The new code started out as the adc code but with most of the details removed. Few things should really have survived: some of the #include lines; create_pair and error_return_tuple; how to expose the resource to the Erlang world (although with significantly different code). The happy case result building on nif_oneshot_new_channel_p should be 1:1 the same. I don't really know the implications on the copyright. |
Where did you see that? |
Most CI is red eg. https://github.com/atomvm/AtomVM/actions/runs/19595789851/job/56245058688?pr=1998 |
These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later