Skip to content

gpio: Refactor use of RIOT's gpio_read()#132

Merged
chrysn merged 1 commit intoRIOT-OS:mainfrom
maribu:gpio/gpio_read_return_bool
Oct 23, 2024
Merged

gpio: Refactor use of RIOT's gpio_read()#132
chrysn merged 1 commit intoRIOT-OS:mainfrom
maribu:gpio/gpio_read_return_bool

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Oct 23, 2024

C callers of gpio_read() do not care whether gpio_read() returns an pre C99 style bool (which is an int with every non-zero number being a valid representation of true) or C99 style bool, as long as the stay within spec (treating representation of true as true with no regard to the encoding).

This refactors the rust use to match the C behavior. This would allow updating gpio_read() to return C99 style bool as type.

C callers of gpio_read() do not care whether gpio_read() returns an
pre C99 style `bool` (which is an `int` with every non-zero number
being a valid representation of `true`) or C99 style `bool`, as long
as the stay within spec (treating representation of `true` as `true`
with no regard to the encoding).

This refactors the rust use to match the C behavior. This would allow
updating `gpio_read()` to return C99 style `bool` as type.
@maribu maribu marked this pull request as ready for review October 23, 2024 11:21
Copy link
Copy Markdown
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM.

(I do suspect that it's technically a breaking API change in C, and as we can't get rid of static inline right now I'd like it better if our macros were at least static inlines internally, but anyhow, for the given situation in RIOT, this seems to be the right fix).

@chrysn chrysn merged commit b531cc9 into RIOT-OS:main Oct 23, 2024
@chrysn
Copy link
Copy Markdown
Member

chrysn commented Oct 24, 2024

Looking at the PR actually using this (RIOT-OS/RIOT#20936), my comment was misguided, and this is already an actual API (and a non-static function even).

This does not alter the review assessment that this PR is doing the right thing, but had no simple .into() been available, knowing that it'll always just be i32 or bool (and not, maybe, some odd platform doing its own thing) would also have allowed the use of a custom conversion trait.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants