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

[shell] make shell ping command asynchronous #7544

Merged
merged 1 commit into from
Jun 16, 2021

Conversation

gjc13
Copy link
Contributor

@gjc13 gjc13 commented Jun 11, 2021

Problem

Fixes #7519 shell app ping cannot receive

Change overview

This is caused by the sleep in the ping client blocking the stack's event loop.
This PR moves the timeout handling logic to the timer handlers to resolve the issue.

Testing

Manually tested with chip-shell and chip-echo-responder


return (now >= gPingArguments.GetLastEchoTime() + gPingArguments.GetEchoInterval());
return Transport::PeerAddress::UDP(gDestAddr, gPingArguments.GetEchoPort(), INET_NULL_INTERFACEID);
Copy link
Contributor

Choose a reason for hiding this comment

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

should we at some point add interface id arguments?
If we start to use link-local addreses, the interface will matter.

likely not for this PR.

@gjc13 gjc13 force-pushed the fix-shell-ping-block branch from c08b968 to 3e0a926 Compare June 15, 2021 04:10
@gjc13 gjc13 requested a review from yufengwangca June 15, 2021 04:14
@gjc13 gjc13 force-pushed the fix-shell-ping-block branch from 3e0a926 to 8215b58 Compare June 15, 2021 07:42
Copy link
Contributor

@yufengwangca yufengwangca left a comment

Choose a reason for hiding this comment

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

The conversation has been closed, and I am not able to reopen it, suppose the first SendEchoRequest succeed, and the second SendEchoRequest failed with error within EchoTimerHandler. How Shutdown() is triggered in this case?

@gjc13
Copy link
Contributor Author

gjc13 commented Jun 16, 2021

The conversation has been closed, and I am not able to reopen it, suppose the first SendEchoRequest succeed, and the second SendEchoRequest failed with error within EchoTimerHandler. How Shutdown() is triggered in this case?

Each SendEchoRequet will schedule the next send in the timer handler by calling StartTimer. If the SendEchoRequest fails it will cancel the next send by calling CancelTimer. The ping will be aborted once any of the send fails just as the behavior in the original version.

@woody-apple
Copy link
Contributor

/rebase

@woody-apple woody-apple force-pushed the fix-shell-ping-block branch from 8215b58 to 2c7f162 Compare June 16, 2021 06:06
@github-actions
Copy link

Size increase report for "nrfconnect-example-build" from 1397b4b

File Section File VM
chip-shell.elf bss 0 1280
chip-shell.elf text 128 128
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
bss,1280,0
.debug_info,0,281
.strtab,0,202
.symtab,0,192
.debug_loc,0,129
.debug_ranges,0,128
text,128,128
.debug_line,0,121
.debug_frame,0,108
.debug_abbrev,0,70
.debug_aranges,0,16
.debug_str,0,11
.shstrtab,0,-2

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize


@woody-apple woody-apple merged commit 41d5696 into project-chip:master Jun 16, 2021
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[shell] Shell app is not able to receive the incoming message after refactor
5 participants