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.

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

sys/shell/commands/ping: fix dependency & convert to ztimer #18349

Merged
merged 2 commits into from
Jul 21, 2022
Merged

sys/shell/commands/ping: fix dependency & convert to ztimer #18349

merged 2 commits into from
Jul 21, 2022

Conversation

maribu
Copy link
Member

@maribu maribu commented Jul 21, 2022

Contribution description

Add missing dependency to xtimer so that the shell command ping is
again provided when requested.

Testing procedure

Run make BOARD=nucleo-f767zi -C examples/gnrc_networking flash term and then help. If the ping command is listed again, the bug is fixed. (Did so, worked for me.)

Issues/PRs references

None

@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 21, 2022
@github-actions github-actions bot added the Area: sys Area: System label Jul 21, 2022
@maribu maribu changed the title sys/shell/commands/sc_gnrc_icmpv6_echo: fix dependency sys/shell/commands/ping: fix dependency & convert to ztimer Jul 21, 2022
@maribu
Copy link
Member Author

maribu commented Jul 21, 2022

I also added a commit to convert to ztimer on top of the fix.

@chrysn: IMO the fix commit would be backport material. The conversion to ztimer can IMO wait for the next release.

@chrysn
Copy link
Member

chrysn commented Jul 21, 2022

Indeed (it's not part of the test suite but easily verified).

miri64
miri64 reviewed Jul 21, 2022
View reviewed changes
sys/Makefile.dep Show resolved Hide resolved
sys/shell/commands/Makefile Show resolved Hide resolved
sys/shell/commands/sc_gnrc_icmpv6_echo.c Show resolved Hide resolved
maribu added 2 commits July 21, 2022 13:20
@maribu
sys/shell/commands/sc_gnrc_icmpv6_echo: fix dependency
3a5e363 
Add missing dependency to xtimer so that the shell command `ping` is
again provided when requested.
@maribu
sys/shell/commands/sc_gnrc_icmpv6_echo: use ztimer
cab761b
@maribu maribu force-pushed the sys/shell/commands/sc_gnrc_icmpv6_echo branch from e905ee4 to cab761b Compare July 21, 2022 11:20
@github-actions github-actions bot added Area: examples Area: Example Applications Area: tests Area: tests and testing framework labels Jul 21, 2022
kfessel
kfessel reviewed Jul 21, 2022
View reviewed changes
Copy link
Contributor

@kfessel kfessel 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.

seems like we are missing a gnrc_ping_cmd (pseudo)module -in other words why should a icmp-echo-service also add the client part.

I just saw there is an answer agreed on in the a resolved comment, at the same time looking at the line it is danging on some lines later there is an explicit filter for gnrc_udp_cmd so why is ping included if "shell commands" and "icmp-echo-service" but the "udp_cmd" needs an extra module?

miri64
miri64 approved these changes Jul 21, 2022
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Considering that this is meant to be backported and should as thus be as minimal as possible: ACK. Adding a new (pseudo-)module to specifically select the ping command can be done as a follow-up.

@maribu
Copy link
Member Author

maribu commented Jul 21, 2022

I'd much rather see making all shell commands submodules :)

@maribu maribu merged commit de064b7 into RIOT-OS:master Jul 21, 2022
@maribu
Copy link
Member Author

maribu commented Jul 21, 2022

Thx :)

@maribu maribu deleted the sys/shell/commands/sc_gnrc_icmpv6_echo branch July 21, 2022 15:38
@miri64
Copy link
Member

miri64 commented Jul 21, 2022

Indeed (it's not part of the test suite but easily verified).

Please provide a backport.

@maribu maribu mentioned this pull request Jul 21, 2022
Merged
@maribu
Copy link
Member Author

maribu commented Jul 21, 2022

Backport provided in #18354

@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco left review comments

@kfessel kfessel kfessel left review comments

@miri64 miri64 miri64 approved these changes

Assignees
No one assigned
Labels
Area: examples Area: Example Applications Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2022.10
Development

Successfully merging this pull request may close these issues.
5 participants
@maribu @chrysn @miri64 @kfessel @benpicco