drivers/periph/gpio_ll: shrink gpio_conf_t #20236
8000
Jump to bottom
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gschorcht
reviewed
Jan 8, 2024
|
The following change is needed to avoid compilation errors. diff --git a/cpu/esp32/periph/gpio_ll.c b/cpu/esp32/periph/gpio_ll.c
index 2c3d730d84..13f387b516 100644
--- a/cpu/esp32/periph/gpio_ll.c
+++ b/cpu/esp32/periph/gpio_ll.c
@@ -151,7 +151,6 @@ int gpio_ll_init(gpio_port_t port, uint8_t pin, const gpio_conf_t *conf)
/* since we can't read back the configuration, we have to save it */
_gpio_conf[gpio] = *conf;
- _gpio_conf[gpio].schmitt_trigger = false;
if (esp_idf_gpio_config(&cfg) != ESP_OK) {
return -ENOTSUP; |
3b3ca2e to
d315010
Compare
benpicco
reviewed
Jan 11, 2024
d315010 to
3a69bf6
Compare
|
So, finally: EFM32
|
benpicco
approved these changes
Jan 16, 2024
1 task
|
@benpicco Is the ACK still valid? The change since ACK boils down to passing the conf no longer as |
4dc9960 to
69ffb62
Compare
69ffb62 to
7d08c3c
Compare
7d08c3c to
bd4e920
Compare
This commit optimizes the `gpio_conf_t` type in the following regards: - The "base" `gpio_conf_t` is stripped from members that only some platforms support, e.g. drive strength, slew rate, and disabling of the Schmitt Trigger are no longer universally available but platform-specific extensions - The `gpio_conf_t` is now crammed into a bit-field that is 8 bit or 16 bit wide. This allows for storing lots of them e.g. in `driver_foo_params_t` or `uart_conf_t` etc. - A `union` of the `struct` with bit-field members and a `bits` is used to allow accessing all bits in a simple C statement and to ensure alignment for efficient handling of the type Co-authored-by: Gunar Schorcht <gunar@schorcht.net>
Printing the newline after the state was printed is not optional.
This also moves the call to `gpio_ll_print_conf()` and `puts("")` to
a static function to safe enough ROM so that this still can be flashed
on `nucleo-l011k4`.
The default pin config is only a place holder anyway. But if it is invalid at least on AVR most of the firmware is considered unreachable. This updates the default GPIO config to something that should look plausible to the compiler for all MCUs supporting GPIO LL, so that ROM and RAM size in the CI start making sense.
bd4e920 to
04d537c
Compare
Now that `gpio_conf_t` is on all implemented platforms no larger than a register, we can more efficiently pass it by value rather than via pointer.
04d537c to
9222762
Compare
|
Woohooo! 👯 🎉 Thx for the reviews :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
This commit optimizes the
gpio_conf_ttype in the following regards:gpio_conf_tis stripped from members that only some platforms support, e.g. drive strength, slew rate, and disabling of the Schmitt Trigger are no longer universally available but platform-specific extensionsgpio_conf_tis now crammed into a bit-field that is 8 bit or 16 bit wide. This allows for storing lots of them e.g. indriver_foo_params_toruart_conf_tetc.unionof thestructwith bit-field members and abitsis used to allow accessing all bits in a simple C statement and to ensure alignment for efficient handling of the typeIn addition
gpio_conf_tis no longer passed via pointer, but by value. Because it now is small enough to fit in a register on all supported platforms, this results in less overhead when callinggpio_ll_init().Testing procedure
The provided test app should still work for all supported MCU families.
Trade-Off Involved
Dropping members (such as disabling of the Schmitt Trigger, or selecting the slew rate) that are not universally found from the common interface is IMO uncontroversial. The more fancy MCUs just provide their own
gpio_conf_tand still provide all the features they have, but the basic MCUs have a lot less overhead.The use of bit fields is IMO controversial:
Pros:
gpio_conf_tto the point that it is acceptable to heavily use it infoo_params_tin drivers orfoo_conf_tin the boardsperiph_conf.hCons:
gpio_ll_init(),gpio_ll_query_conf(), andgpio_ll_print_conf.gpio_ll_query_conf()andgpio_ll_print_conf()are super useful for debugging, but I don't see them used for other stuff. IMO overhead here doesn't mattergpio_ll_init()IMO doesn't matter that much, as this is not typically done.gpio_ll_init()is too slow already as it is. But I think it would be possible to add an optional API for just changing the direction. In any case, this is out of scope of this PR.)==> The overhead in terms of ROM specifically for
gpio_ll_init()is the most relevant oneOn Cortex M0 / STM32
This PR:
master:On Cortex M0+ / EFM32ZG
This PR:
master:On Cortex M4(F) / STM32
This PR:
master:On Cortex M4(F) / nRF5x
This PR:
master:On AVR
This PR:
master:On ESP32
This PR:
master:Summary
The size of
gpio_conf_tgoes down from 7 B to 1 B or 2 B, so at least 5B is saved for every storedgpio_conf_t. This comes for a cost for up to 26 B increase ingpio_ll_init().We safe ROM latest when 6 instances of
gpio_conf_tare stored in flash. This would already be the case when two SPI buses with onegpio_conf_tper pin is used.Issues/PRs references
None