Skip to content

sys/stdio_ethos: replace USE_ETHOS_FOR_STDIO by stdio_ethos pseudomodule #11668

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jun 24, 2019
Merged

sys/stdio_ethos: replace USE_ETHOS_FOR_STDIO by stdio_ethos pseudomodule #11668

merged 5 commits into from
Jun 24, 2019

Conversation

aabadie
Copy link
Contributor

@aabadie aabadie commented Jun 10, 2019

Contribution description

This PR provides a new module stdio_ethos as a replacement for USE_ETHOS_FOR_STDIO. Applications using this macro now depends on this new module.

stdio_ethos also depends on the ethos and stdin, provided in #11598.

Testing procedure

  • examples/gnrc_border_router should still work
  • related tests application should still pass when using stdio_ethos: tests/riotboot_flashwrite, tests/gnrc_ipv6_ext, tests/gnrc_rpl_srh and tests/gnrc_sock_dns.

Issues/PRs references

Based on #11598

@aabadie aabadie added Area: network Area: Networking State: waiting for other PR State: The PR requires another PR to be merged first labels Jun 10, 2019
@aabadie aabadie requested review from cladmi and kaspar030 June 10, 2019 16:13
@aabadie aabadie mentioned this pull request Jun 10, 2019
Merged
cladmi
cladmi previously requested changes Jun 14, 2019
View reviewed changes
Copy link
Contributor

@cladmi cladmi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It would be nice to have it implemented without #11598 as this one could be done by adding stdio_uart_rx dependency alone.

Some minor remarks inline.

Further steps for other pull requests.

From the implementation, it looks like stdio_ethos should be moved to its own module like stdio_rtt. But it is another task that requires many changes.

Also the code is doing implicit circular dependency, the code in drivers/ethos for stdio references variables defined in stdio_uart…

drivers/ethos/ethos.c:extern isrpipe_t stdio_uart_isrpipe;

@aabadie
Copy link
Contributor Author

aabadie commented Jun 14, 2019 •
edited
Loading

It would be nice to have it implemented without #11598 as this one could be done by adding stdio_uart_rx dependency alone.

Once #11598 is merged (and I hope it will be soon), there will be no need to rework this one. Let's finish #11598, which will fix #11525 btw, and continue improving other things afterwards.

@aabadie aabadie mentioned this pull request Jun 20, 2019
Merged
kaspar030
kaspar030 requested changes Jun 20, 2019
View reviewed changes
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

Almost good to go. Please add the missing "stdio_uart" dependency.

@kaspar030
Copy link
Contributor

please rebase

@kaspar030 kaspar030 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 20, 2019
@aabadie aabadie force-pushed the pr/sys/stdio_ethos branch from 5c3555a to 2859849 Compare June 22, 2019 20:11
@kaspar030
Copy link
Contributor

Please squash!

@aabadie aabadie force-pushed the pr/sys/stdio_ethos branch from d53c85b to 5e2f267 Compare June 23, 2019 20:16
@aabadie
Copy link
Contributor Author

aabadie commented Jun 23, 2019

squashed!

@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 24, 2019
@kaspar030 kaspar030 added the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Jun 24, 2019
kaspar030
kaspar030 approved these changes Jun 24, 2019
View reviewed changes
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

I found another reference of USE_ETHOS_STDIO in ethos' own README.md and pushed a commit that updates it.
Please take a quick look.
Otherwise I tested that gnrc_border_router still works using make term.

@kaspar030 kaspar030 dismissed cladmi’s stale review June 24, 2019 08:01

Comments have been addressed, reviewer on holidays. Thx @cladmi!

@aabadie
Copy link
Contributor Author

aabadie commented Jun 24, 2019

Please take a quick look.

Looks good. Thanks!

@kaspar030 kaspar030 merged commit 110e718 into RIOT-OS:master Jun 24, 2019
This was referenced Jun 25, 2019
Merged
Closed
@aabadie aabadie deleted the pr/sys/stdio_ethos branch July 2, 2019 11:24
@miri64 miri64 mentioned this pull request Jul 25, 2019
Merged
@miri64 miri64 mentioned this pull request Jul 25, 2019
Merged
@aabadie aabadie mentioned this pull request Jan 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaspar030 kaspar030 kaspar030 approved these changes

+1 more reviewer

@cladmi cladmi cladmi left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aabadie @kaspar030 @cladmi