Skip to content

sys/ztimer: fix xtimer_compat#15627

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
maribu:ztimer_xtimer_compat
Dec 14, 2020
Merged

sys/ztimer: fix xtimer_compat#15627
miri64 merged 1 commit intoRIOT-OS:masterfrom
maribu:ztimer_xtimer_compat

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Dec 14, 2020

Contribution description

  • Eagerly include board.h so that potential definitions of XTIMER_WIDTH can be overwritten. Subsequent includes of board.h should hit the include guard
  • assert() timeout value to be representable in 32 bit

Testing procedure

See #15605

Issues/PRs references

Addresses comments to #15605 made after merging

@maribu maribu requested review from benpicco and kaspar030 December 14, 2020 09:20
@maribu maribu requested a review from bergzand as a code owner December 14, 2020 09:20
@maribu maribu added Area: sys Area: System Area: timers Area: timer subsystems 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) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 14, 2020

static inline void xtimer_set_timeout_flag64(xtimer_t *t, uint64_t timeout)
{
assert(timeout <= UINT32_MAX);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@maribu maribu force-pushed the ztimer_xtimer_compat branch from 3b17227 to 2f7470b Compare December 14, 2020 09:47
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

looks good to me, sorry for not spotting this earlier.

@miri64 miri64 merged commit dae714c into RIOT-OS:master Dec 14, 2020
@maribu maribu deleted the ztimer_xtimer_compat branch December 14, 2020 12:17
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Dec 14, 2020

Thanks for the reviews :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: sys Area: System Area: timers Area: timer subsystems 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

Development

Successfully merging this pull request may close these issues.

4 participants