net: sntp: Handle case of request timeout and fix up sample #15582
Conversation
pfalcon
force-pushed
the
sntp-fixes
branch
2 times, most recently
from
634069a to
fd6bac5
Compare
|
@galak, Getting SNTP into a workable state is on critical path to get Google IoT SDK working, please help to review/merge this. |
pfalcon
changed the title
jukkar
left a comment
jukkar
left a comment
There was a problem hiding this comment.
Minor nit pick, looks ok otherwise.
There was a problem hiding this comment.
For timeout, better use K_SECONDS(4) instead, preferably via a define.
There was a problem hiding this comment.
Used K_SECONDS(4). As these are supposed to be learning samples, using too much defines would only make them more complicated to follow (and yes, there're different delays, e.g. IPv4 case vs IPv6 case one; better just to have them inline).
There was a problem hiding this comment.
As these are supposed to be learning samples, using too much defines would only make them more complicated to follow
I am disagreeing here. For the novice user reading the code, it is much easier if the parameter says SNTP_TIMEOUT than K_SECONDS(4). Not a big issue thou, it is still better now than before.
d3zd3z
left a comment
d3zd3z
left a comment
There was a problem hiding this comment.
The fix seems good. I'm not sure if the config file and logging changes should be in the same commit.
There was a problem hiding this comment.
I think you have a typo here. Should be SNTP IPv6 right?
There was a problem hiding this comment.
Should be SNTP IPv6 right?
Fixed, thanks.
Previously, a case when poll() call timed out wasn't handled, and recv() was called unconditionally. In the case of timeout, recv() itself would hang indefinitely. Signed-off-by: Paul Sokolovsky <[email protected]>
1. Output what steps the app performs, so it doesn't look to user that it simply hanged. 2. Don't use infinite timeouts, because that will hang. 3. Clearly note which requests are for IPv4 vs IPv6 server. 4. Define IPv4 gateway. This sample is configured to run against SNTP on local Linux host, but standard distros (e.g. Ubuntu) don't run SNTP server by default, so usual outcome for running this sample will be timeout. A realistic way to get successful output would be to run it against a server on the Internet, for what a gateway is required. Signed-off-by: Paul Sokolovsky <[email protected]>
|
@jukkar: The issue resolved, CI passed. |
4 participants
Previously, a case when poll() call timed out wasn't handled, and
recv() was called unconditionally. In the case of timeout, recv()
itself would hang indefinitely.
Signed-off-by: Paul Sokolovsky [email protected]