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

sys/stdio_ethos: replace USE_ETHOS_FOR_STDIO by stdio_ethos pseudomodule #11668

Merged
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
cladmi
cladmi previously requested changes Jun 14, 2019
Copy link
Contributor

@cladmi cladmi left a comment

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;

sys/stdio_uart/stdio_uart.c Outdated Show resolved Hide resolved
drivers/ethos/ethos.c Show resolved Hide resolved
Makefile.dep Show resolved Hide resolved
@aabadie
Copy link
Contributor Author

aabadie commented Jun 14, 2019

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.

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.

Makefile.dep Show resolved Hide resolved
@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
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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

3 participants