New, orthogonal and complete time conversion API

[andyross]

Before pushing a big refactoring of the organization of #17155, it seems prudent to extract the bits that weren't controversial and see if we can merge those in isolation.

First up is the time conversion API. This is the code as it appears in the original, with two additions:

1. The automatic conversion generator is extended to emit nanosecond conversions, and the test rig is updated accordingly for coverage. This is done as a separate commit for simplicity, but I can fold it in if people want.

2. The older conversion utilities have been deprecated and replaced throughout the tree with the new calls instead of leaving the usages in place.

@carlescufi / @nashif - I've lost track of exactly who the gatekeepeers are supposed to be on this work. Please add appropriate reviewers.

Closes #12509

[andyross]

Note there are some checkpatch false positives with the way __DEPRECATED_MACRO works, and some 80 character lines that are due to the automated changes (and seem in most cases to be pre-existing). I don't currently plan on cleaning those up manually but will if requested.

[carlescufi]

@andyross can you respond to #19591 (comment)?
It's mainly about documenting negative timeout values and catching if it is misued.

I agree. @andyross, I wrote a small summary of items I'd like to see addressed in this PR, but you don't seem to have addressed those so far.

[andyross]

Returning to this again. This isn't the timeout patch. No change in timeout behavior is happening here. This is only the conversion API, which I was trying to get merged early since for a long time it seemed uncontroversial.

I guess I need more specific guidance about exactly what needs to be changed in this PR. I can't do the requested fixes here, they're comments on a different PR.

[nordic-krch]

@andyross

No change in timeout behavior is happening here.

That's not true. Because conversion API is used in the kernel to convert input s32_t ms timeout to ticks. If negative number is provided behavior changes.

[andyross]

It is true, though... The old ad-hoc conversion routines only notionally took a signed argument, the rounding and truncation logic being applied (both in and outside of the conversions) was assuming positive arguments. The patch here isn't changing any of that logic, the instructions emitted are going to be exactly the same. It's just cleaning up the API to make clear what's happening.

But that seems like a digression anyway, and another argument. What you wanted to see, I thought, were documentation changes, and I genuinely don't know where to put them in this PR.

Can you please be really, really, hyper-specific about exactly what you want to see here before you'll agree to merge this? I mean seriously: treat my like I'm six and tell me where to type the bytes you want to see. I'm just completely out of ideas on how to please everyone.

(It needs yet another rebase too, obviously. But at this point I'm out of energy to try to keep maintaining this out of tree.)

[pabigot]

No change in timeout behavior is happening here.

That's not true. Because conversion API is used in the kernel to convert input s32_t ms timeout to ticks. If negative number is provided behavior changes.

I agree with @andyross on this point. I can't confirm a behavior change with k_sleep(), k_poll(), or k_timer_start() (for initial delay) resulting from how this PR changes the processing of negative durations. Tested on nrf52840_pca10055, nucleo_l476rg, efr32mg_sltb004a, and sam_e70_xplained.

I can confirm problems with negative durations on qemu_cortex_m3, but they are not introduced by changes related to this PR so may be considered a bug in the way those delays are handled by qemu.

My reasons for abstaining from approve/reject are unrelated to these specifically identified concerns, and have not changed from #19591 (comment). A reworked PR that I would approve is in https://github.com/pabigot/zephyr/commits/nordic/19591 though it still has a small risk resulting from performing translations on unsigned values.

[nordic-krch]

I see the change in behavior. Lets look at k_sleep because it has simple implementation which is:

ticks = k_ms_to_ticks_ceil32(ms);
ticks = z_tick_sleep(ticks);

where ms and ticks are signed values. There is a range of values where conversion will result in the change of sign and it depends on system tick frequency. For example if SYS_CLOCK_TICKS_PER_SEC=100 (like in qemu_cortex_m3) then k_ms_to_tocks_ceil32(-10) gives 429496729. As a result k_sleep(-10) on qemu_cortex_m3 freezes and on master returns immediately.

There are SYS_CLOCK_TICKS_PER_SEC values which will cause trouble for wide range of values. For example setting it to 1001 will result in positive result for all small negative values.

[nordic-krch]

@andyross

Can you please be really, really, hyper-specific about exactly what you want to see here

@carlescufi pointed it out here: #19591 (comment)

As for me, I would like to see clarification in the kernel API (timeouts and k_sleep) that even though timeout argument is s32_t negative values are not accepted. Imo, this clarification is needed because this PR changes negative timeouts behavior (not all, but in some cases). Currently, as argument is s32_t, user may assume (i would assume) that if i pass negative number then it will be interpretted as the past and it will expire immediately.

assert is welcomed as well. Note that z_impl_k_timer_start has such assert and pend(), z_impl_k_sleep() and z_impl_k_usleep() in sched.c don't.

[jhedberg]

Currently, as argument is s32_t, user may assume (i would assume) that if i pass negative number then it will be interpretted as the past and it will expire immediately.

K_FOREVER is defined as (-1) so I don't think that's an obvious assumption.

[nordic-krch]

@jhedberg of course that it's not obvious assumption (that's why i wrote user may assume). Even if it's not assumption, it's the implementation:
first K_FOREVER is excluded:

zephyr/kernel/sched.c

Line 370 in 556f3cb

if (timeout != K_FOREVER) {

then everything signed smaller that 1 is treated as 1 tick ahead:

zephyr/kernel/timeout.c

Line 88 in 556f3cb

ticks = MAX(1, ticks);

[jhedberg]

@nordic-krch I think I misunderstood you - I thought you were suggesting to change something in the implementation (either old or new) wrt. this. Clearly documenting which values which are invalid and will result in undefined behaviour sounds like a good thing to me.

[pabigot]

There are SYS_CLOCK_TICKS_PER_SEC values which will cause trouble for wide range of values. For example setting it to 1001 will result in positive result for all small negative values.

Yes, this is the clue: I can confirm the breakage with negative timeouts in this situation. Reviewing and updating the documentation what a timeout is, taking into account future changes such as a requirement that they go through something like K_MSEC() rather than be bare integers, seems appropriate. I'm not convinced that should block getting the core functionality of safe low-level conversion functions in. (I still prefer the reduced approach of https://github.com/pabigot/zephyr/commits/nordic/19591 which doesn't deprecate API or change it in a way that could increase MISRA essential type violations.)

[nordic-krch]

@andyross if kernal API doxygen update apart from stating that timeout must be non-negative would include K_MSEC macro requirement (as suggest by @pabigot) then it may require longer review and thus independent PR.

Alternatively, you could change all Waiting period to wait for and similar to Non-negative waiting period to wait for that would clarify that negative numbers shall not be used.

Additionally, i would insist to add asserts in 3 places (mentioned here: #19591 (comment)) (unless im mistaken and there is more) to catch any negative arguments in timeout API.

[andyross]

I still don't get it. The API has never accepted negative values (other than special tokens). It's never worked with negative values. None of the code in this pull request changes what happens with any of those timeout parameters when you pass acceptable values.

(Now, I agree that this misfeature is sort of a flaw, and that we should have clearer behavior about exactly how timeout parameters are specified. That, after all, was part of the reason behind the timeout change from which this PR was split).

Your complaint, as I see it, reduces to the fact that if you use the new conversion routines with a value that used to be invalid, that the new invalid behavior might not match the old invalid behavior. And I don't see that I can fix that in this pull request, with either documentation or anything else.

I mean, maybe you're asking that I merge this back into the pull request with the timeout changes? This got split out specifically because of complaints that it was too large and I thought maybe I could get this in, as no one had ever argued about this part. But that seems not to be the case either.

Please reconsider. At this stage that's all I can ask.

[nordic-krch]

@andyross

The API has never accepted negative values

Where is that stated? argument is s32_t and nowhere is stated that only limited range is accepted.

i've prepared #20439.

[carlescufi]

@nashif @andyross can we merge this as-is or can CI actually pass?

[nashif]

dont merge yet, this needs a rebase

[andyross]

OK, rebased to fix the use of a deprecated macro that crossed with this since it was written. Should be good to go.