Introduce ESP32 ADC driver #1264
Conversation
UncleGrumpy
force-pushed
the
esp32_adc
branch
4 times, most recently
from
281f00c
to
5f8f0c2
Compare
UncleGrumpy
force-pushed
the
esp32_adc
branch
from
5f8f0c2
to
2099e34
Compare
|
After these changes are merged atomvm/exatomvm#26 should also be merged to enable Elixir use. |
| term raw = interop_kv_get_value_default(read_options, ATOM_STR("\x3", "raw"), FALSE_ATOM, ctx->global); | ||
| term voltage = interop_kv_get_value_default(read_options, ATOM_STR("\x7", "voltage"), FALSE_ATOM, ctx->global); | ||
|
|
||
| int adc_reading[samples_val]; |
There was a problem hiding this comment.
This is super dangerous, it might lead to stack corruption if a big enough value of samples_val is provided. I suggest allocating memory rather than this construction.
But, it looks like old array values are not even used, and it looks like we are just using the element at index i, am I wrong?
There was a problem hiding this comment.
No, you are absolutely right all the way around. I had just used a static array in my early testing, but completely forgot so change that when I implemented the code in the driver.
This should just be an int.
There was a problem hiding this comment.
Fixed and tested.
UncleGrumpy
force-pushed
the
esp32_adc
branch
from
2099e34
to
f30fa9b
Compare
|
|
||
| struct UnitResource *unit_rsrc = (struct UnitResource *) obj; | ||
|
|
||
| if (!IS_NULL_PTR(unit_rsrc->unit_handle)) { |
There was a problem hiding this comment.
Is unit_rsrc->unit_handle being NULL an unlikely/error situation?
Let's remember that IS_NULL_PTR is just a convenience macro (mostly aimed to allocation checks) to avoid to type every time UNLIKELY(a == NULL), and UNLIKELY purpose is to tell compiler that a branch is unlikely.
Also, I'm not sure that !LIKELY works as we expect.
The same comment applies to every occurrence of IS_NULL_PTR(...).
There was a problem hiding this comment.
These occurrences should all be fixed now. The usage these was very ambiguous now that you point it out, and probably not at all what I intended.
| term read_options = argv[2]; | ||
| VALIDATE_VALUE(read_options, term_is_list); | ||
| term samples = interop_kv_get_value_default(read_options, ATOM_STR("\x7", "samples"), term_from_int32(DEFAULT_SAMPLES), ctx->global); | ||
| int samples_val = term_to_int32(samples); |
There was a problem hiding this comment.
We are missing validity checks here. The default values guarantees that a value is always assigned, but it doesn't guarantee us that the value will be correct too.
Please, let's check they are integer, and let's also check the are in the accept valid range (e.g. what happens if a negative number is provided?)
There was a problem hiding this comment.
Indeed. That was an oversight, I try to be careful about always validating input parameters, I don't know why I forgot to here. I even did the calculation to determine the maximum number of samples that can be taken without overflowing the int that holds the accumulated results before diving by the number of samples. I don't know why I didn't just use that as a failsafe (except that over 62000 samples seemed an unrealistic concern).
|
|
||
| const struct Nif *atomvm_adc_get_nif(const char *nifname) | ||
| { | ||
| TRACE("Locating nif %s ...", nifname); |
There was a problem hiding this comment.
I don't know how much this API can be made generic also for non EPS32 devices, if this operation is not straightforward I suggest calling this module esp_adc or something like that, so that would make the transition path to a generic and cross-platform adc API smoother.
There was a problem hiding this comment.
Very good point. When I first started I thought that stm32 was similar enough that they could use a common API. After closer examination it is quite different, and Pico will be more similar to stm32, but there are enough differences that starting with platform specific interfaces and later writing a common API is probably the best approach here.
There was a problem hiding this comment.
The module, and Elixir exports have been renamed.
UncleGrumpy
force-pushed
the
esp32_adc
branch
from
f30fa9b
to
76827a4
Compare
This driver offers resource based nif APIs using unit and channel resources, as well as a set of convenient gen_server based APIs, which use pin numbers, as an alternative to using the nif resource handles. Unit 2 is currently disabled for the ESP32C3 by default due to known hardware limitations, see: https://docs.espressif.com/projects/esp-idf/en/latest/esp32c3/api-reference/peripherals/adc_oneshot.html#hardware-limitations Unit 2 is currently disabled by default for the ESP32 (classic) due to WiFi conflicts. This decision can be revisited after PR atomvm#1137 is merged, and the WiFi driver can be successfully stopped after it has been started. Unit 2 is enabled by default for all other chips with more than one ADC unit, as the chips released after the ESP32 classic have a functional ADC arbitrator peripheral that allows ADC unit 2 use while WiFi is enabled. Signed-off-by: Winford <winford@object.stream>
Adds Erlang and Elixir examples for the ESP32 ADC driver, and documentation to the Programmers Guide. Signed-off-by: Winford <winford@object.stream>
UncleGrumpy
force-pushed
the
esp32_adc
branch
from
76827a4
to
aabb956
Compare
|
retested on c3card - good to go! |
This set of changes adds support for the ESP32 ADC peripheral, with documentation and examples.
Closes #250
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