Fix k_sleep() timeouts in net #7849
Conversation
jukkar
requested review from
jhedberg,
mike-scott,
pfalcon and
tbursztyka
as code owners
Codecov Report
@@ Coverage Diff @@
## master #7849 +/- ##
==========================================
- Coverage 55.02% 55.02% -0.01%
==========================================
Files 481 481
Lines 53970 53963 -7
Branches 10483 10483
==========================================
- Hits 29695 29691 -4
+ Misses 19996 19993 -3
Partials 4279 4279
Continue to review full report at Codecov.
|
pfalcon
left a comment
pfalcon
left a comment
There was a problem hiding this comment.
Good! (Was in my queue too ;-) )
Suggested to over "raw" time-related values in net/ip/ and convert them too.
subsys/net/ip/ipv6.c
Outdated
There was a problem hiding this comment.
We have more "raw" values like this, IMHO makes sense to convert them within the scope of this PR too.
There was a problem hiding this comment.
Should convert this to msecs for better granularity. In a separate PR apparently.
subsys/net/ip/rpl.c
Outdated
There was a problem hiding this comment.
Maybe K_MSEC(500) after all?
There was a problem hiding this comment.
The comment few lines above says "Set up another DAO within half the expiration time" so in this respect it makes sense to leave the division in place.
subsys/net/buf.c
Outdated
There was a problem hiding this comment.
The rest of the fixes in this PR seem good, but I'm not sure I agree with this. The way the K_SECONDS, K_MSEC, K_MINUTES, K_HOURS macros work, is that you don't need to know what the "native" representation of time is that's passed to the kernel APIs. It might be milliseconds, but it could be something else that the macros convert the input to. The above change assumes that the macro converts to milliseconds, whereas with MSEC_PER_SEC it's at least explicit.
There was a problem hiding this comment.
+1 to @jhedberg's comment, I'd also say that division by MSEC_PER_SEC is easier to understand than division by K_SECONDS(1).
There was a problem hiding this comment.
Ok, lets leave MSEC_PER_SEC there then.
subsys/net/buf.c
Outdated
Convert couple of MSEC() calls to K_MSEC() as the timeouts when using MSEC() are just too long. Signed-off-by: Jukka Rissanen <[email protected]>
Use of K_SECONDS() macro is more intuitive so use that instead of plain MSEC_PER_SEC define. Signed-off-by: Jukka Rissanen <[email protected]>
This one converts "raw" timeout value to use K_MSEC() macro in order to make clear how long the timeout is. Signed-off-by: Jukka Rissanen <[email protected]>
|
Updated according to comments. |
5 participants
This converts remaining MSEC() calls to K_MSEC() in networking code.
Also convert MSEC_PER_SEC to K_SECONDS() as that is more intuitive.
Fixes #7657