drivers/periph/gpio_ll: shrink gpio_conf_t#20236
Merged
maribu merged 5 commits intoRIOT-OS:masterfrom Jan 21, 2024
Merged
Conversation
maribu
added
the
State: WIP
maribu
requested review from
MichelRottleuthner,
MrKevinWeiss,
aabadie,
gschorcht,
kYc0o,
leandrolanzieri and
vincent-d
as code owners
github-actions
bot
added
Platform: ARM
gschorcht
reviewed
Jan 8, 2024
Contributor
|
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; |
gschorcht
added
the
Process: API change
maribu
added
CI: ready for build
maribu
force-pushed
the
periph/gpio_ll/smaller_type
branch
2 times, most recently
from
3b3ca2e to
d315010
Compare
benpicco
reviewed
Jan 11, 2024
Contributor
benpicco
left a comment
benpicco
left a comment
There was a problem hiding this comment.
Looks good to me, did you test this on all affected architectures?
maribu
force-pushed
the
periph/gpio_ll/smaller_type
branch
from
d315010 to
3a69bf6
Compare
Member
Author
|
So, finally: EFM32
|
benpicco
approved these changes
Jan 16, 2024
1 task
maribu
added
CI: ready for build
Member
Author
|
@benpicco Is the ACK still valid? The change since ACK boils down to passing the conf no longer as |
maribu
force-pushed
the
periph/gpio_ll/smaller_type
branch
from
4dc9960 to
69ffb62
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
maribu
force-pushed
the
periph/gpio_ll/smaller_type
branch
from
69ffb62 to
7d08c3c
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
maribu
force-pushed
the
periph/gpio_ll/smaller_type
branch
from
7d08c3c to
bd4e920
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
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 <[email protected]>
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.
maribu
force-pushed
the
periph/gpio_ll/smaller_type
branch
from
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.
maribu
force-pushed
the
periph/gpio_ll/smaller_type
branch
from
04d537c to
9222762
Compare
Member
Author
|
Woohooo! 👯 🎉 Thx for the reviews :) |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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