Skip to content

sys/stdio_uart: do not cast function pointer.#10312

Closed
jcarrano wants to merge 1 commit intoRIOT-OS:masterfrom
jcarrano:uart_stdio_fix_cast
Closed

sys/stdio_uart: do not cast function pointer.#10312
jcarrano wants to merge 1 commit intoRIOT-OS:masterfrom
jcarrano:uart_stdio_fix_cast

Conversation

@jcarrano
Copy link
Contributor

@jcarrano jcarrano commented Nov 1, 2018

Contribution description

There was a function pointer cast in stdio_uart. The warning was being explicitly supressed in the module's makefile (see #9121).

This commit removes the suppression and fixes the warning by providing a wrapper function.

The wrapper was already defined in the AT driver (see #9740), as part of a fix for a similar bug there, so the definition was moved to the isrpipe module, the logic being that using isrpipe in conjunction with a periph_uart is a common enough use-case.

Testing procedure

tests/driver_at should still compile, same as examples/hello_world.

Issues/PRs references

Undoes #9121.

@jcarrano jcarrano added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 1, 2018
@bergzand
Copy link
Member

@jcarrano Is there a reason for providing a wrapper instead of changing the isrpipe_write_one from char c to uint8_t c? From what I can find, with this PR, the only user of isrpipe_write_one is ethos.c and a cast from char to uint8_t can be easily added there.

@jcarrano
Copy link
Contributor Author

@bergzand

  1. I wanted to avoid an API change.
  2. What makes those function pointers incompatible is not the arguments, but the return type (void vs int).

Regarding (2), it's usually innocuous in RISC architectures where the first results are returned in registers, which explains why the code is not blowing up right now.

@jcarrano
Copy link
Contributor Author

jcarrano commented Jun 3, 2019

I rebased on top of master and fixed the conflicts.

There was a function pointer cast in stdio_uart. The warning was
being explicitly supressed in the module's makefile.
This commit removes the suppression and fixes the warning by providing
a wrapper function.
The wrapper was already defined in the AT driver, as part of a fix for
a similar bug there, so the definition was moved to the isrpipe module,
the logic being that using isrpipe in conjunction with a periph uart
is a common enough use-case.
@jcarrano
Copy link
Contributor Author

There are conflicts, I'm not rebasing this again. I give up - fixing bugs is clearly not a priority.

@jcarrano jcarrano closed this Jun 26, 2019
@jcarrano jcarrano added the State: won't fix State: The issue can not or will not be fixed label Jun 26, 2019
@aabadie
Copy link
Contributor

aabadie commented Jun 26, 2019

Is rebasing so difficult ? I don't think there are that much conflicts. I'm surprised this PR didn't get much review: it's not a difficult change, 3 reviewers were assigned.

@jcarrano
Copy link
Contributor Author

No, it is not difficult, but it is a waste of time to rebase every N months (which shows that there is work being done on that module, just no interest in bug fixes). If anyone is interested on this fix, they can feel free to take over it.

@aabadie
Copy link
Contributor

aabadie commented Jun 26, 2019

there is work being done on that module, just no interest in bug fixes

I think the conflict was because #11598 #11668 was merged recently. So yes there's work on that module, and it was for fixing a bug. So your argument doesn't stand.

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

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: won't fix State: The issue can not or will not be fixed Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants