Skip to content

sys/stdio: cleanup of STDIO abstraction#9503

Merged
haukepetersen merged 4 commits intoRIOT-OS:masterfrom
haukepetersen:opt_stdio_cleanup
Aug 22, 2018
Merged

sys/stdio: cleanup of STDIO abstraction#9503
haukepetersen merged 4 commits intoRIOT-OS:masterfrom
haukepetersen:opt_stdio_cleanup

Conversation

@haukepetersen
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen commented Jul 5, 2018

Contribution description

Currently, the design of our STDIO mapping is partly a mess - e.g. why does the Segger RTT STDIO need to call uart_stdio_x functions?!

So this PR is a first step towards cleaning up the situation: I simply factored out the init/read/write functions in a more generic header (stdio_base.h), while mapping the actual implementation to this interface during compile-time base on the selected modules. The 'users' of these backend functions now don't need to care about anything implementation specific (hence no uart left in the name)...

P.S.: my driver for this cleanup is to run the RIOT shell over BLE...

Issues/PRs references

none

@haukepetersen haukepetersen added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 5, 2018
vincent-d
vincent-d requested changes Jul 16, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@vincent-d vincent-d left a comment

Choose a reason for hiding this comment

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

Apart from the minor renaming issues which have already been spotted by Murdock, I'm wondering if the vfs initialization code should not be moved out of stdio_uart to a generic module, as this code should be shared for all stdio_* implementations.

sys/stdio_uart/stdio_uart.c Outdated
return -EBADF;
}
return uart_stdio_read(dest, nbytes);
return stdio_uart_read(dest, nbytes);
Copy link
Copy Markdown
Member

@vincent-d vincent-d Jul 16, 2018

Choose a reason for hiding this comment

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

stdio_read?

sys/stdio_uart/stdio_uart.c Outdated
return -EBADF;
}
return uart_stdio_write(src, nbytes);
return stdio_uart_write(src, nbytes);
Copy link
Copy Markdown
Member

@vincent-d vincent-d Jul 16, 2018

Choose a reason for hiding this comment

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

stdio_write?

@miri64 miri64 mentioned this pull request Aug 9, 2018
Closed
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Aug 16, 2018

Apart from the minor renaming issues which have already been spotted by Murdock, I'm wondering if the vfs initialization code should not be moved out of stdio_uart to a generic module, as this code should be shared for all stdio_* implementations.

yes, that makes certainly sense. I would however like to do it in a follow up, so this PR doesn't get blown up by too many changes. Are you ok with that?

@vincent-d
Copy link
Copy Markdown
Member

vincent-d commented Aug 16, 2018

Are you ok with that?

I am

@haukepetersen haukepetersen mentioned this pull request Aug 16, 2018
Merged
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Aug 16, 2018

prefect. See #9790

vincent-d
vincent-d approved these changes Aug 16, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@vincent-d vincent-d left a comment

Choose a reason for hiding this comment

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

I'm OK with the changes, tests/periph_uart works on my board.

@vincent-d
Copy link
Copy Markdown
Member

vincent-d commented Aug 16, 2018

You can squash ;)

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Aug 17, 2018

squashed.

@haukepetersen haukepetersen force-pushed the opt_stdio_cleanup branch 2 times, most recently from 7563168 to 2163ad0 Compare August 17, 2018 10:27
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Aug 17, 2018

and fixed a minor doxygen misshap

@basilfx
Copy link
Copy Markdown
Member

basilfx commented Aug 17, 2018 •
edited
Loading

Maybe it's good to add the API change label, because this changes USEMODULE += rtt_stdio into USEMODULE += stdio_rtt? At least I have to change it for my own board :-)

@haukepetersen haukepetersen added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Aug 17, 2018
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Aug 17, 2018

done.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Aug 17, 2018

@gebart @bergzand @kaspar030 are you all good with this change? If so, please feel free to press the green button...

@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Aug 17, 2018

I agree with the intent and general idea of this PR, but I did not have time to look at the code. It's good to see that the stdio code is being cleaned up 👍

basilfx
basilfx reviewed Aug 17, 2018
View reviewed changes
sys/stdio_rtt/stdio_rtt.c Outdated

void uart_stdio_init(void) {
void stdio_init(void) {
#ifndef RTT_STDIO_DISABLE_STDIN
Copy link
Copy Markdown
Member

@basilfx basilfx Aug 17, 2018

Choose a reason for hiding this comment

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

Similar to STDIO_UART_DEV, shouldn't this then be named STDIO_RTT_DISABLE_STDIN?

basilfx
basilfx reviewed Aug 17, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@basilfx basilfx left a comment

Choose a reason for hiding this comment

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

One small question/comment.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Aug 22, 2018

@basilfx good catch, fixed and squashed.

@basilfx
Copy link
Copy Markdown
Member

basilfx commented Aug 22, 2018

Then I also ACK :-)

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Aug 22, 2018

Alright, I think the level of agreement is sufficient :-) -> let's go then!

@haukepetersen haukepetersen merged commit a6c7b0a into RIOT-OS:master Aug 22, 2018
@haukepetersen haukepetersen deleted the opt_stdio_cleanup branch August 22, 2018 15:25
@miri64 miri64 mentioned this pull request Oct 1, 2018
Closed
@basilfx basilfx mentioned this pull request Jan 9, 2019
Merged
chrysn added a commit to RIOT-OS/rust-riot-sys that referenced this pull request May 31, 2022
@chrysn
Follow the restructuring of [9503]
b8ba494 
Were this project any further I'd consider keeping compatibility (not to
old RIOT versions, but to applications using uartstdio::UartStdio).

[9503]: RIOT-OS/RIOT#9503
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@basilfx basilfx basilfx left review comments

@vincent-d vincent-d vincent-d approved these changes

Assignees

@jnohlgard jnohlgard

@kaspar030 kaspar030

@bergzand bergzand

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@haukepetersen @vincent-d @basilfx @jnohlgard @kaspar030 @bergzand