Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

maribu
Copy link
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
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
81 checks passed
@chrysn
Copy link
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