Updates to ESP32 GPIO driver to match other MCU platforms #1408

UncleGrumpy · 2024-12-21T19:15:04Z

These changes move the GPIO specific atoms out of the ESP32 platform default atoms (created at boot) into the GPIO driver where they are only created as needed.

Breaking changes update the error returns to match the spec. Nifs raise Error, and port functions return {error, Reason} when errors are encountered.

Adds gpio driver tests to the Wokwi sim to test driver functionality and error return correctness for invalid parameters.

With #1448 closes #894

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

petermm · 2024-12-21T20:11:00Z

do we need to remove RISING_ATOM_INDEX et al in platform_defaultatoms.h ?

AtomVM/src/platforms/esp32/components/avm_sys/include/platform_defaultatoms.h

Line 28 in 63ba966

#define RISING_ATOM_INDEX (PLATFORM_ATOMS_BASE_INDEX + 2)

UncleGrumpy · 2024-12-21T21:11:31Z

do we need to remove RISING_ATOM_INDEX et al in platform_defaultatoms.h ?

AtomVM/src/platforms/esp32/components/avm_sys/include/platform_defaultatoms.h

Line 28 in 63ba966

#define RISING_ATOM_INDEX (PLATFORM_ATOMS_BASE_INDEX + 2)

This is done in commit bede918

UncleGrumpy · 2024-12-21T22:05:12Z

My mistake, you are correct! Those changes should have been included in the commit I mentioned, but were not added. Correction has been pushed.

petermm · 2024-12-27T11:21:51Z

LGTM, but assume it needs rebasing to new defaultatoms setup.

UncleGrumpy · 2024-12-27T19:37:19Z

Indeed. I also realized that I should be using port_create_error_tuple, and remove the unnecessary error_tuple_maybe_gc and update/remove the usage of create_pair as well.

UncleGrumpy · 2024-12-27T20:45:25Z

All fixed. This should be ready now. We needed to keep create_pair, because we need to preserve a root, if gc is necessary.

petermm · 2024-12-27T20:50:02Z

CHANGELOG.md

@@ -17,6 +17,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 - ESP32: improved sntp sync speed from a cold boot.

+### Fixed


duplicated.. ##Fixed

oops rebase went wrong. thanks.

bettio · 2024-12-28T23:55:02Z

src/platforms/esp32/components/avm_builtins/gpio_driver.c

+static const char *const high_atom = ATOM_STR("\x4", "high");
+static const char *const low_atom_str = ATOM_STR("\x3", "low");
+#define HIGH_ATOM globalcontext_make_atom(glb, high_atom)
+#define LOW_ATOM globalcontext_make_atom(glb, low_atom_str)


I don't agree with having something that looks like a constant but hides a function with some side effect.

bettio · 2024-12-28T23:59:35Z

src/platforms/esp32/components/avm_builtins/gpio_driver.c

-    return gpio_digital_read(argv[0]);
+    GlobalContext *glb = ctx->global;  // needed for HIGH_ATOM & LOW_ATOM macros
+    term result = gpio_digital_read(ctx, argv[0]);
+    if (UNLIKELY((result != HIGH_ATOM) && (result != LOW_ATOM))) {


We have globalcontext_is_term_equal_to_atom_string function that allows us to compare a term against an atom string without adding it to the atom table.
I suggest using it.
Also (result != HIGH_ATOM) && (result != LOW_ATOM) might be factored out to a helper function, such as is_logic_level

bettio · 2024-12-29T00:13:08Z

src/platforms/esp32/components/avm_builtins/gpio_driver.c

 #endif

+static const char *const high_atom = ATOM_STR("\x4", "high");
+static const char *const low_atom_str = ATOM_STR("\x3", "low");


After some thoughts I realized we don't really want this pattern for defining atom strings:

the compiler is going to de-duplicate strings regardless using this pattern or not

we add lines in place of writing things such as ATOM_STR("\x3", "low")

maintaining code requires jumping back and forth, while inline atom strings are "local"

I see you point completely, and the comments about the macros as well. Originally I wrote this without either, but thought the macros might improve readability, which led to defining the static const char strings, but as you point out jumping back and forth is worse for readability.

bettio · 2024-12-29T14:16:03Z

Please, see also #1442

UncleGrumpy · 2024-12-30T05:22:33Z

Please, see also #1442

I believe I have followed the guidelines you laid out in #1442, and the comments you made above about atom creation and fixed all of the atoms used by the driver to insure they are created in a safe way. I also used ATOM_STR() inline, removing the const char* declarations.

Standardizes STM32 error returns. Nif functions raise `Error`, port functions return `{error, Reason}` on errors. Addresses concerns raised in issue atomvm#1442. With atomvm#1408 this closes issue atomvm#894 Signed-off-by: Winford <[email protected]>

Standardizes stm32 gpio driver error returns. Nif functions raise `Error`, port functions return `{error, Reason}` on errors. Addresses some concerns raised in issue atomvm#1442. With atomvm#1408 this closes issue atomvm#894 Signed-off-by: Winford <[email protected]>

Reduce the number of GPIO related atoms created at boot by using more efficient matching to atom string where possible. Removes `static const char *const` declarations for atoms, replacing with `ATOM_STR()` in place for better readability. Handles the creations of atoms used by the driver in a safer way, checking their validity where necessary. Signed-off-by: Winford <[email protected]>

Adds the `internal_error` atom from OTP for returning errors for "thing that shouldn't happen". There are rare occasions where user inputs may be valid, but an internal operation fails for some reason (potentially an internal bug to the VM, not the users application code). For example, this may be an invalid internal state when setting the direction for a gpio pin. If the users inputs are valid `internal_error` should be returned rather than `badarg`, which could mislead application developers into chasing a bug in their application that isn't there. Signed-off-by: Winford <[email protected]>

Changes error returns to match the spec and ofther platforms. Nifs now raise any errors, and port function errors return `{error, Reason}`. Updates gpio.erl to reflect the correct returns, and make it more clear that errors for nifs will be raised. Closes atomvm#894 Signed-off-by: Winford <[email protected]>

Adds a test to run on hardware with a jumper wire, or in the wokwi simulator to test the basic esp32 gpio driver funtionality and error handling, with tests for all boards when full_sim_test is run. The sim-test for P4 depends on PR atomvm#1438 being merged to be able to select the correct pins for the device for the the GPIO tests. Signed-off-by: Winford <[email protected]>

UncleGrumpy force-pushed the esp32_gpio_spec branch from 587b4d4 to aca3489 Compare December 21, 2024 22:03

UncleGrumpy force-pushed the esp32_gpio_spec branch from aca3489 to fa94ff3 Compare December 21, 2024 23:17

UncleGrumpy marked this pull request as ready for review December 21, 2024 23:32

UncleGrumpy force-pushed the esp32_gpio_spec branch 2 times, most recently from c4c5343 to 24fdbf9 Compare December 22, 2024 03:23

UncleGrumpy force-pushed the esp32_gpio_spec branch from 24fdbf9 to d2d259c Compare December 27, 2024 20:42

petermm reviewed Dec 27, 2024

View reviewed changes

UncleGrumpy force-pushed the esp32_gpio_spec branch 3 times, most recently from f3a4ad8 to 8f81b88 Compare December 27, 2024 21:21

bettio requested changes Dec 29, 2024

View reviewed changes

UncleGrumpy force-pushed the esp32_gpio_spec branch from 8f81b88 to 669c0f1 Compare December 30, 2024 05:16

UncleGrumpy force-pushed the esp32_gpio_spec branch 2 times, most recently from cb6e30e to 35b5bb9 Compare January 3, 2025 08:54

UncleGrumpy mentioned this pull request Jan 3, 2025

Standardize stm32 gpio driver error returns #1448

Open

UncleGrumpy force-pushed the esp32_gpio_spec branch 2 times, most recently from 8400531 to 71e1443 Compare January 9, 2025 04:59

UncleGrumpy force-pushed the esp32_gpio_spec branch from 71e1443 to fd4ebdc Compare January 9, 2025 05:02

UncleGrumpy added 4 commits January 9, 2025 05:14

UncleGrumpy force-pushed the esp32_gpio_spec branch from fd4ebdc to cffd8ff Compare January 9, 2025 05:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to ESP32 GPIO driver to match other MCU platforms #1408

Updates to ESP32 GPIO driver to match other MCU platforms #1408

UncleGrumpy commented Dec 21, 2024 •

edited

Loading

petermm commented Dec 21, 2024

UncleGrumpy commented Dec 21, 2024

UncleGrumpy commented Dec 21, 2024

petermm commented Dec 27, 2024

UncleGrumpy commented Dec 27, 2024

UncleGrumpy commented Dec 27, 2024 •

edited

Loading

petermm Dec 27, 2024

UncleGrumpy Dec 27, 2024

bettio Dec 28, 2024

bettio Dec 28, 2024

bettio Dec 29, 2024

UncleGrumpy Dec 29, 2024

bettio commented Dec 29, 2024

UncleGrumpy commented Dec 30, 2024 •

edited

Loading

Updates to ESP32 GPIO driver to match other MCU platforms #1408

Are you sure you want to change the base?

Updates to ESP32 GPIO driver to match other MCU platforms #1408

Conversation

UncleGrumpy commented Dec 21, 2024 • edited Loading

petermm commented Dec 21, 2024

UncleGrumpy commented Dec 21, 2024

UncleGrumpy commented Dec 21, 2024

petermm commented Dec 27, 2024

UncleGrumpy commented Dec 27, 2024

UncleGrumpy commented Dec 27, 2024 • edited Loading

petermm Dec 27, 2024

Choose a reason for hiding this comment

UncleGrumpy Dec 27, 2024

Choose a reason for hiding this comment

bettio Dec 28, 2024

Choose a reason for hiding this comment

bettio Dec 28, 2024

Choose a reason for hiding this comment

bettio Dec 29, 2024

Choose a reason for hiding this comment

UncleGrumpy Dec 29, 2024

Choose a reason for hiding this comment

bettio commented Dec 29, 2024

UncleGrumpy commented Dec 30, 2024 • edited Loading

UncleGrumpy commented Dec 21, 2024 •

edited

Loading

UncleGrumpy commented Dec 27, 2024 •

edited

Loading

UncleGrumpy commented Dec 30, 2024 •

edited

Loading