Conversation
|
|
||
| static inline void xtimer_set_timeout_flag64(xtimer_t *t, uint64_t timeout) | ||
| { | ||
| assert(timeout <= UINT32_MAX); |
There was a problem hiding this comment.
OT-sidenote: This will provide a very confusing assert output, due to RIOT_FILE_RELATIVE being the C-file that includes this, not the header file.
There was a problem hiding this comment.
OT-sidenote: we need to figure out a way to make this more robust. asserting on long timeouts will often not show up in our tests. This needs to be handles properly, maybe just falling back to ms / second resolution. Or maybe adding another ztimer clock for really long timeouts.
There was a problem hiding this comment.
I wonder if we really should spent time and effort on gracefully handling strange corner cases for this compatibility layer. If we focus on switching to ztimer instead, we fix this issue and other more import issues like power consumption at the same time.
But anyway, there are more assert()s in the header now than this one. So if this is considered an issue big enough for needing fixing, we have to provide a mechanism for that anyway.
Generally speaking: I don't think that there is any reason for relative timeouts to ever use more than 32 bit. But if we plan to add absolute timeouts (which would benefit e.g. MAC layer requirements), those could benefit from an 64 bit API. (Maybe ztimer_now_t as unit, so that uses get to pick 64 bit support if they need it.) I wouldn't mind if that API returns an -EOVERFLOW for values too far in the future, so that internal data structures can still be 32 bit. But I really don't want to convert my ztimer_now() + timeout value to 32 bit by hand.
There was a problem hiding this comment.
Yeah, I agree on all points. I'd just like to avoid weird breakage due to the compatibility layer that hits as soon as ztimer is enabled.
We should maybe consider deprecating the xtimer 64bit API.
3b17227 to
2f7470b
Compare
benpicco
left a comment
There was a problem hiding this comment.
looks good to me, sorry for not spotting this earlier.
|
Thanks for the reviews :-) |
Contribution description
board.hso that potential definitions ofXTIMER_WIDTHcan be overwritten. Subsequent includes ofboard.hshould hit the include guardassert()timeout value to be representable in 32 bitTesting procedure
See #15605
Issues/PRs references
Addresses comments to #15605 made after merging