Corrections to stm32 gpio driver #899
Conversation
UncleGrumpy
force-pushed
the
stm32_gpio_driver
branch
2 times, most recently
from
d15dc88
to
9fed680
Compare
UncleGrumpy
force-pushed
the
stm32_gpio_driver
branch
4 times, most recently
from
7afb2aa
to
c0905e9
Compare
UncleGrumpy
force-pushed
the
stm32_gpio_driver
branch
3 times, most recently
from
9453e6a
to
6d1ef38
Compare
| } | ||
|
|
||
| uint16_t pin_levels = gpio_get(gpio_bank, (1U << gpio_pin_num)); | ||
| uint16_t level = (pin_levels >> gpio_pin_num); | ||
| TRACE("Read: Bank 0x%08lX Pin %u. RESULT: %u\n", gpio_bank, gpio_pin_num, level); | ||
|
|
||
| return level ? HIGH_ATOM : LOW_ATOM; | ||
| return level ? globalcontext_make_atom(ctx->global, ATOM_STR("\x4", "high")) : globalcontext_make_atom(ctx->global, ATOM_STR("\x3", "low")); |
There was a problem hiding this comment.
I suggest a level_to_atom inline helper function for an improved readability.
UncleGrumpy
force-pushed
the
stm32_gpio_driver
branch
from
6d1ef38
to
937ac81
Compare
| static const char *const invalid_level_atom = ATOM_STR("\xD", "invalid_level"); | ||
| static const char *const invalid_trigger_atom = ATOM_STR("\xF", "invalid_trigger"); | ||
| static const char *const invalid_irq_atom = ATOM_STR("\xB", "invalid_irq"); | ||
| static const char *const gpio_interrupt_atom = ATOM_STR("\xE", "gpio_interrupt"); |
There was a problem hiding this comment.
The usual habit for defining variables in header files is using extern to avoid allocating them every time the .h file is included, and deciding an .c file that allocates them (and defines the value to which they will be initialized).
However,
regardless of what I wrote just before,
do we really have any advantage in defining all those variables in gpio_driver.h? I don't think so, lot of them aren't even used outside of GPIO driver (such as set_direction).
There was a problem hiding this comment.
I was expecting that most of those will be used in the SPI driver. There is already a mechanism to include or not the gpio nifs and port driver, I plan on the same for SPI. For extremely constrained systems it is possible SPI will be desired, but gpio functionality will not be required. In that case gpio_driver.h could be included in the SPI driver, even if the gpio port driver and nifs were excluded.
But now I think I was wrong about that. Not nearly as many will be reused as I thought.
There was a problem hiding this comment.
I moved these back into gpio_driver.c and reduced the number of static const char *const to the minimum necessary atoms.
| { ATOM_STR("\x1", "f"), GPIOF }, | ||
| { ATOM_STR("\x1", "g"), GPIOG }, | ||
| { ATOM_STR("\x1", "h"), GPIOH }, | ||
| { a_atom, GPIOA }, |
There was a problem hiding this comment.
Let's take a_atom as an example, that is used just here at this line.
I don't see any practical vantage in defining it elsewhere, on the other side I need to jump to other places when doing refactoring, renaming them to something else, adding new ones (then we have to be consistent) etc...
Also, I don't feel like we are improving much readability since ATOM_STR("\x1", "a") tells very well what kind of thing we are creating and what is its value.
But maybe I'm not considering other aspects, so please share your point here so we can take a decision about the style that we should keep.
There was a problem hiding this comment.
These have been reverted to keep the original format. I agree there was no benefit in changing this.
UncleGrumpy
force-pushed
the
stm32_gpio_driver
branch
2 times, most recently
from
98e8ca6
to
bafa5c2
Compare
Removes the TRACE calls from interrupt hanlers becase these are blocking calls and should not be used in interrupt handlers. Extra diagnostic info is added to the `isr_handler` (that maintains the context and relays the interrupt information to the `gpio_interrupt_callback`) that can be used to determint the same information as the removed TRACE calls. Signed-off-by: Winford <[email protected]>
Fixes several incorrect format types in printed logs. Signed-off-by: Winford <[email protected]>
Fixes compiler warnings about previously undeclared functions: - `gpioregister_nif_collection` - `gpioregister_port_driver` Signed-off-by: Winford <[email protected]>
There were several placesd inputs were not being propery validated, which could lead to unexpected behaviors or VM crashes. Signed-off-by: Winford <[email protected]>
There was an occurrence of `MUTABLE_LIST_FOR_EACH` where the list was not being changed in any way that has been replaced with `LIST_FOR_EACH`. Signed-off-by: Winford <[email protected]>
Add declarations for several functions that cause the compiler to emmit a `no previous prototype` warining. Signed-off-by: Winford <[email protected]>
Remove the use of macros to make atoms. Signed-off-by: Winford <[email protected]>
Fixes nifs to RAISE_ERROR(error_type) instead of return {error, Reason}.
Signed-off-by: Winford <[email protected]>
Moves several variable declarations to immediately before use, rather than declaring them at the top of the function. Signed-off-by: Winford <[email protected]>
UncleGrumpy
force-pushed
the
stm32_gpio_driver
branch
from
bafa5c2
to
ff9e627
Compare
Fix several bugs in the stm32 gpio driver.
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