drivers: counter: Extend set channel alarm flags (late-to-set)

[nordic-krch]

Added flags for controling detection of setting alarm to late. Updated drivers to return -ENOTSUP when new option is requested.

[pabigot]

Thanks for working this up. With some clarification it looks like it could handle the use cases I had in mind when I opened #12626.

"Lateness" is really only meaningful in the context of absolute alarms. A late relative alarm is easily identified: it's one where the offset is non-positive. That this was the intent in the original counter API is indicated by the decision to express a relative alarm as a value that can't be negative. (How to interpret a relative alarm where .ticks=0 appears to be under-defined.)

I see that "lateness" in this proposal is defined in terms of counter_get_max_relative_alarm. This definition has some uses, but in practice it doesn't make a lot of sense since at least Nordic defines this value as top. As such the only value that could be "late" would be a relative advance expressed by setting .ticks=top+1. (Maybe. I really don't understand the idea behind counter_get_max_relative_alarm.)

I don't see a rigorous definition of counter terminology anywhere. So here's my attempt:

• A counter is an object that changes its value between adjacent integers in the closed range [B, T] where B is the bottom (least) value the counter can take on, T is the top (greatest) value the counter can take on, and B < T.
• A counter advances when its value changes from V to the next value not equal to V (the successor of V).
• A counter can count up, or it can count down. For a value V such that B < V < T the successor of V is V+1 for a counter that counts up, and is V-1 for a counter that counts down.
• A counter may wrap by changing its value past a range bound. The successor of T in a wrapping counter that counts up is B. The successor of B in a wrapping counter that counts down is T. (Counters that do not wrap stop advancing when they reach the terminal bound.)
• The span S of a counter is the (positive) value T-B+1, i.e. the number of distinct observable counter values.
• The half-span H of a counter is the smallest positive integer greater than or equal to S/2. The set of acceptable relative offsets from a counter value is the half-open interval [H-S, H). (Thus the half-span for S=8 is H=4 with relative values [-4, 4), while the half-span for S=7 is H=4 with relative values [-3, 4).)
• The (absolute) distance Dbetween a counter value A and a counter value B is 0 if A=B, and otherwise is the number of times a counter at value A must advance before it takes the value B. (For a counter that counts up this is the value B-A, or S+B-A if B < A.) If B cannot be reached from A without wrapping and the counter does not wrap then D is undefined.
• The relative distance R between counter value A and counter value B is:
  • D if D < H
  • D-S if D >= H
  • undefined if D is undefined
• A counter observed to be at value V laps when it next advances to V. This will happen when the counter advances S times.
• A counter alarm set to A fires when the counter advances to A.

With all that in place, the motivation for #12626 was in the context of counters where B is zero, T is 2^n-1, and the counter counts up continuously (meaning it wraps): i.e. the sort of counter we'd use for a system timer. In that context I want to specify alarms as absolute value that is a fixed offset from the time the previous alarm was scheduled to fire. Latency means this time might have already passed at the point the alarm is requested. To detect that I use the following natural interpretation:

• The operation of setting an alarm to the counter value A when the counter is currently at C is timely if and only if the value A-C when interpreted as an n-bit signed number is positive.
• The operation of setting an alarm to the counter value A when the counter is currently at C is late if and only if it is not timely.

The generalization of this to arbitrary S replaces the definition of timely:

• The operation of setting an alarm to the counter value A when the counter is currently at C is timely (or in time) if and only if the distance from C to A is positive and less than the half-span H.

Assume B=0, T=15, so S=16. With D the distance from C to A, and R being D interpreted as a signed 4-bit value, we have the following example cases:

C	A	D	R	late?
6	10	4	4	no
14	2	4	4	no
2	14	12	-4	yes
0	7	7	7	no
0	8	8	-8	yes
6	6	0	0	yes

Note that A=C is "late" because the counter must advance S times before it becomes A again, and 16 as a signed 4-bit number is zero, which is non-negative. (And that's why the condition is "non-negative" rather than "positive".)

• The definition of lateness for counters that count down is a straightforward extension when S is 2^n-1.
• When S is even the generalization is straightforward.
• When S is odd you get an edge case, but the definition given above still handles it (though maybe not as people would want).
• If the counter does not wrap then timely can be compatibly defined as a positive R.

None of this addresses issues like Nordic's RTC where there's a hardware requirement that A-C >= 2 in order for the alarm to be guaranteed to fire. Thus A=C+1 is not late, but it also can't be scheduled in a normal way. The counter concept definition should explain what to do in that situation. (In my implementation, the alarm gets scheduled at A+1, so it's set in time, but calls back late.)

Alternatively everything above could be ignored, and the meaning of "late" could mean something like attempting to set a relative alarm with a value that is not less than S, or an absolute alarm to a value A where A < C. I don't personally see the value for either of those interpretations, but I think this commit hints that they're what was intended.

I guess what this all comes down to is the API looks fine, but the concept of "late" is not adequately defined or agreed upon, so this is still premature.

[nordic-krch]

I see that "lateness" in this proposal is defined in terms of counter_get_max_relative_alarm. This definition has some uses, but in practice it doesn't make a lot of sense since at least Nordic defines this value as top.

counter_get_max_relative_alarm returning top was just temporary value as there was no late to set detection there.

If i understand correctly, your proposal divides full range in half and half of the range is the upper alarm limit. I was having configurable limit (like in #15510 ) that could be read using counter_get_max_relative_alarm. After setting an alarm, driver calculates the diff between current counter value and alarm value. If it is bigger than counter_get_max_relative_alarm() it is interpreted as late setting since.

[nordic-krch]

@pabigot I'm planning to join todays API call to discuss this and other counter API PR's.

[pabigot]

counter_get_max_relative_alarm, which is already in and people may be depending on it returning top, still makes no sense. The maximum relative value that can be used to set an alarm is top+1, being the largest offset that can be added without requiring the counter to lap a value it hasn't reached yet. (You can argue for top, but using my definitions it's top+1).

Again, late-to-set only makes sense for absolute alarms: you can't even request a late relative alarm because the ticks field is unsigned. You can ask for a relative alarm 0 ticks in the future, and I have no idea what you're supposed to get.

If you want lateness for absolute alarms to have a configurable boundary instead of the half-span I could accept that even though I personally don't have a case where a different value is useful. Changing the domain model so R is defined relative to a user-provided L which defaults to H would be fine.

However, whatever API is used to provide L should avoid using relative which means something else.

[zephyrbot]

All checks are passing now.

Review history of this comment for details about previous failed status. Note that some checks might have not completed yet.

[nordic-krch]

added long description before late set flags where i tried to explained the concept.

[nordic-krch]

@pabigot @anangl @erwango @MaureenHelm ping

[nordic-krch]

@pabigot regarding lateness. In your PR (#16398) you defined lateness in kernel.h (https://github.com/zephyrproject-rtos/zephyr/blob/9c8ecef757b7e4afe998bea89ec358f9683a6b0c/include/kernel.h) as

If this is not less than 2^31 ticks in the future the deadline will be interpreted to have passed

This is exactly the same what i'm trying to define here except for the fact that in your case limitation is fixed and i wanted to make the limitation more flexible and configurable. In that particular case counter_get_max_relative_alarm would return 2^31 for 32 bit counter. We can remove or deprecate counter_get_max_relative_alarm and stick to half of the top value. That should work for most of the cases as long as top value is big enough. It may generate issues for cases like slow counter with small top value (e.g. top value 10, freq 1hz, 5 tick limitation is a problem).

[pabigot]

@pabigot regarding lateness. In your PR (#16398) you defined lateness in kernel.h (https://github.com/zephyrproject-rtos/zephyr/blob/9c8ecef757b7e4afe998bea89ec358f9683a6b0c/include/kernel.h) as

If this is not less than 2^31 ticks in the future the deadline will be interpreted to have passed

This is exactly the same what i'm trying to define here except for the fact that in your case limitation is fixed and i wanted to make the limitation more flexible and configurable.

In that case it's known that the counter has a span of 2^32 and a fast advance rate. The whole solution was designed based on a restricted version of the domain model I outlined.

In that particular case counter_get_max_relative_alarm would return 2^31 for 32 bit counter. We can remove or deprecate counter_get_max_relative_alarm and stick to half of the top value. That should work for most of the cases as long as top value is big enough. It may generate issues for cases like slow counter with small top value (e.g. top value 10, freq 1hz, 5 tick limitation is a problem).

I'm not clear on what you're saying here, specifically whether you want the ability to change the upper bound for positive relative offsets, or are satisfied with always making it the half-span.

As I said, I don't object to extending the concept of relative offset so that somebody can specify the upper bound rather than restricting it to the the half-span, which would provide exactly the flexibility you originally proposed. But I would like to see subsystem behaviors in Zephyr specified in a much more rigorous manner, with the implementation validated against the specification, rather than against nothing (for the initial proposal) or another implementation that may be for significantly different counter hardware (for people trying to port the feature to another platform).

To apply that policy in this situation: it must be very clear what "relative" means, how it relates to setting alarms using either relative and absolute values (deadlines), and how it relates to assessing late-to-set. I don't believe those distinctions are clear in the existing API, nor in the proposal implemented here.

[nordic-krch]

I'm not clear on what you're saying here, specifically whether you want the ability to change the upper bound for positive relative offsets, or are satisfied with always making it the half-span.

I just wanted to highlight that half-span may have limitations in certain cases because of that we may want to keep it. I know that half-span allows for easy lateness calculation but i think that it can be easily achieved in the driver build on counter API by forcing half-span (if(counter_max_relative_alarm(dev)!=counter_get_top_value(dev)/2) error()) and working with that assumption further. I'm leaning towards having configurable limitation.

But I would like to see subsystem behaviors in Zephyr specified in a much more rigorous manner, with the implementation validated against the specification, rather than against nothing (for the initial proposal) or another implementation that may be for significantly different counter hardware (for people trying to port the feature to another platform).

I represent single vendor so i can validate only against one hardware and it seems like Nordic is the only one vendor pushing it so I don't want to end up in deadlock (no other implementations -> no feature).

As for technical details of the review. I would like to summarize where review is right now (at least how i perceive it). I would like to agree on high level details and then focus on details:

1. adding flags to setting alarm. We agree on that.
2. Flags turning on late setting and defining behavior (return error or callback). I think that we agree on that.
3. Defining how to detect late setting. It seems that we all agree that detection requires limiting how far in future alarm can be set. There are 2 options: fixed or configurable. If we go for configurable then my proposal is to use get_max_relative_alarm for that as from the start my intention for that function was to use it for that purpose. I would like to see agreement here. Then we can focus on naming, documentation.

[nordic-krch]

I've pushed following update: added function counter_get_guard_period() and deprecated counter_get_max_relative_alarm() which seems to confuse much more. With good guard_period explanation it can be cleared for the user than usage of counter_get_max_relative_alarm().

[nordic-krch]

@pabigot any thoughts about new proposal?

[carlescufi]

@mnkp @erwango @MaureenHelm can you please take a look and give feedback?

[nordic-krch]

From the concept point of view I agree with @pabigot: "Lateness" is really only meaningful in the context of absolute alarms.

What about setting relative alarm to expire in 1 tick? It can get late during setup and you might get an error (or callback depending on flag). Second thing: guard period limits maximum relative alarm as well. Setting relative alarm for 2^24-1 will be forbidden on 24 bit counter (it will be interpreted as late alarm). Maximum possible value will be top_value - guard_period.

[pabigot]

I'm moving this out of its original context because I've seen conversations become inaccessible because they were tied to commits or reviews that have been replaced.

I think this is important: any time API is extended, we should have a clear understanding of what the change is intended to accomplish, why it's worth doing, and how the proposed change affects stakeholders (implementers and current and potential users). I really don't think we have that here. I don't think this PR is unique in that gap, but it's the one where I'm looking right now.

Hm, when one sets the counter it might be set too late. User would probably would like to get notified about this and would like the driver to detect that. Is that one specific use case for one vendor?

No, because it's not a use case. It's a high-level description of the API shortcoming that I originally pointed out in #12626. I don't really know the motivation for this change, but assume it's in support of using counters for system timers, with use cases based on the problems that occur if a system timer expires in 36 hours instead of 5 ms because something delayed the set operation.

My objections might be reduced if you can clearly describe and justify what lateness means. At the moment, lateness appears to be defined relative to a concept of guard period that is proposed to be a driver-specific build-time constant, but I haven't seen the motivation for introducing that concept, or enough information about what it means that other vendors can be expected to be able to determine the appropriate value to hard-code into their drivers.

So let me try to reconstruct it, starting with a simple initial position: an alarm is late if at the point the compare register is set the counter would have to advance more than some count (e.g. the half-span) in order for the compare to be detected. I.e. the alarm was intended to fire very quickly, but because the counter advances asynchronously it had already gone past the desired deadline.

I can see that maybe you need a guard period for Nordic RTC because there's a hardware requirement that the compare register be set no less than 2 advances past the counter in order to guarantee detecting the compare. Is the guard period in the device-tree set to the constant 2?

That could be OK, but it works for RTC because that counter can only be driven by a 32 KiHz clock, and with the assumption that we can disable interrupts between the counter read and compare set. Those two assumptions allow lateness to be detected because the counter advances at a fixed rate and we can make decisions based on that rate and other constants, e.g. that with interrupts disabled the processor speed guarantees the compare will be set within 40 us of the counter read used to assess lateness, so using a guard period of 4 will guarantee the compare will not be missed.

I believe you were considering an undescribed use case where interrupts were not disabled, so your guard period was time-based and large to be sure the set could be guaranteed to not be late.

If that was and still is true, then (a) that seems really fragile; (b) the constant guard period needs to be specified so other vendors can match it; and (c) we haven't considered the case of counters that are not time-driven.

Let's say I'm using a Nordic TIMER in counter mode that advances on an external signal of some sort. Maybe it's a really low late, like a door opening. Or rotations of a racing bicycle wheel. Or a Geiger counter that might trigger at 100 kHz.

What does lateness mean in those situations? Is it time-based, or value based?

The answer probably depends on the use case. But absent details, is it really that clear that we should assume an application would never want to set the guard period?

For what it's worth, I'm now leaning more toward accepting that the guard period concept has value (for non-timer counter applications), but am even less accepting of the position that it doesn't need to be configurable by the application at runtime.

[nordic-krch]

My objections might be reduced if you can clearly describe and justify what lateness means. At the moment, lateness appears to be defined relative to a concept of guard period that is proposed to be a driver-specific build-time constant,

I thought that concept of lateness is clear and it seems to be because you described it well but let me describe it again:

From late setting perspective counter has two properties:

• it wraps since it is limited to up to 32 bits
• it progresses asynchronously from the CPU (when it counts time or external events). There can be a counter which is stopped when CPU is reconfiguring it but that means loosing precision or track of counted events and i haven't seen such implementations.

Because of this properties, it is possible that setting alarm for value which is very soon in the future will result in alarm very far in the future. Chances are smaller when interrupts are locked during operation or when frequency of the counter source is small but it is always possible. It is vital that counter detects that and reports it.

That's late setting. Now, the question is how counter can detect it? That's where guard period comes in because without that counter cannot determine if alarm was set too late or alarm was suppose to expire in the far future. Limiting alarm creates window in which when counter is found after setting an alarm it means that alarm was set too late.

I can see that maybe you need a guard period for Nordic RTC

Guard period has nothing to do with nordic RTC limitation. If you look at the algorithm for late setting detection in nRF RTC counter driver then part handling 2 tick limitation does not use guard period value: https://github.com/zephyrproject-rtos/zephyr/blob/027cd2b9cb2d1091b93d9b9d1356bebda3f2b061/drivers/counter/counter_nrfx_rtc.c#L177

In case of relative alarms guard period length is the time needed to perform counter reconfiguration given in counter ticks. If operation can be interrupted then time needed increases by maximal latency introduced by interruption. In case of absolute alarms it is very application dependent. Because of that, it is recommended to pick guard period as big as possible. Like in k_alarm PR where guard period was half of the span.

but am even less accepting of the position that it doesn't need to be configurable by the application at runtime.

Would you prefer to have guard period set to 0 (no late setting detection) on init? what's the gain compared to compile time setting? I think it would only make sense if same counter would be used for different purposes during application.

[mnkp]

From the concept point of view I agree with @pabigot: "Lateness" is really only meaningful in the context of absolute alarms.

What about setting relative alarm to expire in 1 tick? It can get late during setup and you might get an error (or callback depending on flag).

That's a good question the current PR doesn't seem to address. Here we have an issue with setting an alarm too soon rather than too late, but we should cover this case too. We could add a restriction that relative alarm should be higher or equal to the guard time or otherwise, depending on the flag settings, the function will return an error or execute the callback immediately.

Second thing: guard period limits maximum relative alarm as well. Setting relative alarm for 2^24-1 will be forbidden on 24 bit counter (it will be interpreted as late alarm). Maximum possible value will be top_value - guard_period.

This limitation is artificial and not needed.

[nordic-krch]

@mnkp

This limitation is artificial and not needed.

Can you elaborate how would you distinguish following situations (assuming top value 32 bit)?

• relative alarm setting to 2^32-1 ticks from now
• relative alarm setting to 1 tick from now being set too late

[mnkp]

Can you elaborate how would you distinguish following situations (assuming top value 32 bit)?

• relative alarm setting to 2^32-1 ticks from now
• relative alarm setting to 1 tick from now being set too late

In the first case the ticks value passed to the counter_set_channel_alarm function will be 1, in the second the value will be 2^32-1. When setting the relative alarm we have a problem only if the ticks value is less than the guard time.

[nordic-krch]

In the first case the ticks value passed to the counter_set_channel_alarm function will be 1,

yes, then counter driver write compare register with counter_read() + 1 and expects to get interrupt. right? what happens then?

[nordic-krch]

@mnkp since you contribute one of the drivers (https://github.com/zephyrproject-rtos/zephyr/blob/master/drivers/counter/counter_gecko_rtcc.c) you can try that couple of times (setting relative alarm with 1 tick) and check if you will always get interrupt.

[pabigot]

That's where guard period comes in because without that counter cannot determine if alarm was set too late or alarm was suppose to expire in the far future. Limiting alarm creates window in which when counter is found after setting an alarm it means that alarm was set too late.

If what you're saying is that "guard period" is the name you're using for a value similar to L described here based on the domain model here then that's great.

The description in counter_get_guard_period does not make that clear.

In case of relative alarms guard period length is the time needed to perform counter reconfiguration given in counter ticks.

Here you suggest a guard period is time-based and covers a few instructions, like maybe 10 us for a 64 MHz processor.

If operation can be interrupted then time needed increases by maximal latency introduced by interruption.

So maybe 10 ms, and it's application-dependent.

In case of absolute alarms it is very application dependent. Because of that, it is recommended to pick guard period as big as possible.

So on the order of a half span, like 18 hours for a 24-bit 32 KiHz clock.

It appears that the appropriate value for the guard period depends on whether an alarm is relative or absolute, and whether the set operation is interruptible or not. I think you're using the same concept to solve several independent problems, which makes it hard to understand and very hard to make consistent across independent implementations.

But I'm spending way too much time on this. As I said above I'm never going to approve this PR, but I'm also not going to reject it. I'm just providing feedback.

That feedback is: I think you would be better served by providing an API to set the guard period at runtime. I don't care whether the default value is zero, the half span, or span-1, but in any case I don't see a need for compile-time configurability.

And I think you should specify the expected behavior of a relative alarm with offset zero.

[mnkp]

In the first case the ticks value passed to the counter_set_channel_alarm function will be 1,

yes, then counter driver write compare register with counter_read() + 1 and expects to get interrupt. right? what happens then?

If between the counter read and write to the compare register the counter advances we are going to miss the interrupt. This part is clear. That's why I wrote

We could add a restriction that relative alarm should be higher or equal to the guard time or otherwise, depending on the flag settings, the function will return an error or execute the callback immediately.

There is no problem with setting relative alarm to 2^32-1 however. The counter driver will have plenty of time to write to the compare register after reading out the current counter value. The relative time is counted from the moment the driver has a chance to read the current counter value, not the time counter_set_channel_alarm function is called. The driver has no way of knowing it.

[pabigot]

We could add a restriction that relative alarm should be higher or equal to the guard time or otherwise, depending on the flag settings, the function will return an error or execute the callback immediately.

"Execute the callback immediately" should be clarified: is the driver expected to initiate the call from the context of the set function, or by programming the compare register to the next value guaranteed to cause an interrupt and let the callback be executed from there?

I prefer the latter behavior but the specification must be clear, and the choice not left to individual driver implementations. There's a significant behavioral difference when the set is performed with interrupts disabled.

[nordic-krch]

@mnkp

We could add a restriction that relative alarm should be higher or equal to the guard time

I think that it's less problematic to set that restriction to maximum possible alarm then minimum. Not being able to set short alarm gives much bigger limitation than not being albo to set very very long alarms.

Also minimum distance restriction is much more difficult to achieve with absolute values.

Let say that I want to have periodic alarm, every 10 cycles and minimum restriction is 5 ticks. I keep adding 10 to last alarm and set new alarm. Somehow my last alarm got delayed and falls into that restriction. I am not able to find it out before setting new alarm. I need to delay the alarm. After few delays like that I start to have accumulative error.

On the other hand, guard period impacting maximum value allows me to freely setup every alarm. If it already expired I will get immediate alarm, if i got interrupted for 20 ticks, next alarm will also expire immediately. And cost for that is to limit maximum alarm which is around hundreds of seconds or hours by one second (just an example for RTC case). Such limitation won't even be noticed in majority of cases because it's so rarely used and even if needed it can be fragmented by higher layer.

[mnkp]

"Execute the callback immediately" should be clarified: is the driver expected to initiate the call from the context of the set function, or by programming the compare register to the next value guaranteed to cause an interrupt and let the callback be executed from there?

I prefer the latter behavior [...]

+1, we should avoid changing execution context. I.e. counter callbacks should always be executed in the interrupt context.

[mnkp]

I think that it's less problematic to set that restriction to maximum possible alarm then minimum. Not being able to set short alarm gives much bigger limitation than not being albo to set very very long alarms.

Do you plan to let API users set the relative alarm with ticks value 1 but restrict them from setting the relative alarm with ticks value 2^32-1? (Assuming we're talking about a 32 bit counter)

[nordic-krch]

Do you plan to let API users set the relative alarm with ticks value 1 but restrict them from setting the relative alarm with ticks value 2^32-1?

Yes, that's exactly the plan. Guard period limits how far in future alarm can be set to top_value - guard_period from the moment alarm is scheduled. That applies for absolute and relative alarms.

[nordic-krch]

@mnkp and it's not only me proposing that. See @pabigot k_alarm PR where k_alarm is using very similar concept with fixed guard_period (half of full span): https://github.com/zephyrproject-rtos/zephyr/blob/9c8ecef757b7e4afe998bea89ec358f9683a6b0c/include/kernel.h#L1634

[pabigot]

@mnkp and it's not only me proposing that. See @pabigot k_alarm PR where k_alarm is using very similar concept with fixed guard_period (half of full span):

Well, yes, but:

• that's a decision made because for Nordic the half span gives me over 18 hours as the maximum alarm offset, which more than meets my needs right now;
• I'm only interested in absolute alarms;
• I need to have late-to-set function correctly; and
• I have to maintain multiple callbacks at different times queued on the same "counter".

Which I do by inserting new alarms at the proper position based on their relative delay to the current now (not by maintaining a delta list as in k_timer where you lose information about the original deadline of the alarm). This means alarms that are late-to-set (have non-positive offsets from now when activated) are placed in the proper order, then cut off the head and processed in the proper order (first-in-first-out within deadline). (And, writing that, I realized there's probably a bug, but that's what it's supposed to do.)

The proper solution for my approach is k_cycle_get_64() and support for 64-bit alarms. I know how to do it, but I don't need it now.

I do not support a limitation on the maximum relative delay when there's only support for a single callback and we know the alarm isn't late because its set with a non-negative offset from the current time.

And I still want to know what's supposed to happen with a zero offset relative alarm. In both k_alarm and in the domain model I described in the comment above, it fires immediately: that's the only relative alarm offset that can be late, and it's always late. I suspect most existing implementations of Zephyr's counter API would fire when the counter laps.

[nordic-krch]

I do not support a limitation on the maximum relative delay

Me neither 😄 , it's not like i support it. It is needed to detect late to set alarm. If longer alarm is needed it can always be achieved by fragmentation.

And I still want to know what's supposed to happen with a zero offset relative alarm

It should expire immediately. Unless guard_period=0 then there is no control.

[pabigot]

I do not support a limitation on the maximum relative delay

Me neither , it's not like i support it. It is needed to detect late to set alarm.

I don't believe that's correct. As @mnkp said:

There is no problem with setting relative alarm to 2^32-1 however. The counter driver will have plenty of time to write to the compare register after reading out the current counter value. The relative time is counted from the moment the driver has a chance to read the current counter value, not the time counter_set_channel_alarm function is called. The driver has no way of knowing it.

I believe this statement is correct unless you're concerned about "plenty of time" versus the 2-tick minimum offset for RTC, but you've said that offset is unrelated to the guard period.

For relative alarms you know right away if it's late, and if it's not then you set the counter as necessary to guarantee the alarm will go off as soon as possible.

If you support a relative setting of 1, you don't need to put a cap on the maximum relative offset as long as it is no more than the span where it would cause a lap back to the current timer value.

Now, it may be necessary in your implementation. You have not presented a compelling argument that it's necessary by the problem definition.

[nordic-krch]

I believe this statement is correct unless you're concerned about "plenty of time" versus the 2-tick minimum offset for RTC, but you've said that offset is unrelated to the guard period.

What about counters based on timers running at frequency comparable with CPU frequency? Of course you assume that every one implementing the driver will want to disable interrupts during that procedure. I don't think it is necessary as long as guard period is bigger. For those drivers that block interrupts and has frequency much lower than code execution then guard period probably can be set to small value, even 1.

[pabigot]

Sorry about the empty comment; apparently ctrl-enter posts immediately.

I believe this statement is correct unless you're concerned about "plenty of time" versus the 2-tick minimum offset for RTC, but you've said that offset is unrelated to the guard period.

What about counters based on timers running at frequency comparable with CPU frequency? Of course you assume that every one implementing the driver will want to disable interrupts during that procedure.

No, you don't need a guard period. I don't care how fast the clock is or whether interrupts are disabled. If the relative offset is zero, it's late (a requirement I make, which I haven't seen anybody else acknowledge). Otherwise you add the offset to the current counter value to get an absolute deadline, then set the compare register as necessary to make sure it fires as soon as possible but no sooner than the deadline.

I don't know how to make this more clear, so I'm going to stop yapping again.

[nordic-krch]

Otherwise you add the offset to the current counter value to get an absolute deadline, then set the compare register as necessary to make sure it fires as soon as possible but no sooner than the deadline.

So procedure looks like:

if (ticks == 0) expire();
else {
  now = reg->COUNTER;
  reg->CC = now + ticks; 
}

what if ticks = N and between reading COUNTER and writing to CC counter progressed by N ticks. Event is not generated until next wrap.

Then there is a second potential issue. What if previous CC of cancelled alarm was set to now. Then you may get interrupt now but it may be misinterpreted as new alarm. Something like

disable_counter_int();
->CC = COUNTER, CC event is generated
now = reg->COUNTER;
reg->CC = now + ticks;
enable_counter_int();
-> CC interrupt is generated

Anyway, we seem to discuss necessity of guard period for relative alarm. Is the same doubt for absolute alarms? or we agree on the need for guard period there?

[nordic-krch]

I'm getting lost when it comes to what things we agree on. I will list them and please let me know if i understand correctly.

• reaction to late setting detection is configurable by flag with following option:
  • return error
  • trigger callback from interrupt context
• late setting detection in case of absolute alarm needs a limitation of how long in future alarm can be set (determined by guard period).
• relative alarms should have no need for guard period any value allowed.

disagreement (at least from @pabigot) is regarding guard period configuration. Static vs runtime. @mnkp what is your opinion regarding that. I can make it runtime setting (without static at all - default 0).

[mnkp]

Do you plan to let API users set the relative alarm with ticks value 1 but restrict them from setting the relative alarm with ticks value 2^32-1?

Yes, that's exactly the plan. Guard period limits how far in future alarm can be set to top_value - guard_period from the moment alarm is scheduled. That applies for absolute and relative alarms.

We've already established that setting a relative alarm with a small ticks value is not safe

If between the counter read and write to the compare register the counter advances [behind the reg->CC value] we are going to miss the interrupt.

so how come we want to ignore it? Restricting artificially maximum value of a relative alarm does nothing to solve the issue.

[nordic-krch]

@mnkp

We've already established that setting a relative alarm with a small ticks value is not safe

I think that it should be handled by the driver otherwise such limit is a pita on the user side. Driver will probably delay the alarm. It will have to ensure that alarm is called from interrupt context.

[nordic-krch]

I've updated api with following changes:

• added counter_set_guard_period
• lateness detection flags and guard period are only for absolute alarm
• added clarification that late alarm should be called from the same context as alarm set on time
• renamed COUNTER_ALARM_FLAG_DETECT_LATE -> COUNTER_ALARM_CFG_DETECT_LATE
• renamed COUNTER_ALARM_FLAG_ERROR_WHEN_LATE -> COUNTER_ALARM_CFG_ERROR_WHEN_LATE
• COUNTER_ALARM_CFG_DETECT_LATE for enabling detection and additional COUNTER_ALARM_CFG_ERROR_WHEN_LATE to enable error reporting instead of callback.

[nordic-krch]

@mnkp

As was already mentioned, if a counter is running on a predefined clock the driver should be able to figure out what is the minimum guard period required to safely set a relative alarm. But if the driver counts external events then we would need to tell the driver what's the guard value.

That's why i was initially pushing for guard period for relative and absolute alarms. If we want to set short relative alarm then i see following options:

1. we know frequency so we set alarm further to be sure that by the time alarm is set is still in future. This requires that interrupts are blocked. It also become problematic if counter has higher frequency because then delay depends on code optimization, etc. As you pointer out this does not work if external events are counted.
2. we set the limit to maximal possible relative alarm (guard period) and then after setting an alarm we read the counter. If compare channel is in the past within guard period it means that counter was set too late. Repeat setting compare channel in the future (possible increase the distance). After setting check again counter vs compare channel. Do that in the loop until compare channel is in the future which means that we set alarm on time.
3. relative alarm setting algorithm can check input value and if it is small then algorithm 2. is applied else alarm can be safely set. Question is how to define small value? Lower half should be sufficient.

I'm leaning towards option 3 since it's fairly easy to apply it compared to option 2 and it removes top limitation for relative alarms. It's not like it's a big limitation but with that approach user working with relative alarm only does not need to learn anything about guard period (and does not need to set it).

[nordic-krch]

@mnkp

One more thing, I strongly believe that at least one driver should implement COUNTER_ALARM_CFG_ERROR_WHEN_LATE flag to make sure we didn't overlook anything and have the same understanding of the proposal. We will also need a testcase.

See #15510 I have there nRF RTC driver with late setting detection. It's based on this PR and implements tests for late setting detection.

[pabigot]

We've agreed that the late-to-set flag is not applicable to relative alarms. And while this PR clearly states that it aims to handle this flag only - deal with absolute alarms - we need to ensure that the guard concept is adequate and can be extended to address issues also with relative alarms. If needed.

We need to decide how the driver should handle a case of a relative alarm set to a small value: 'early-to-set'. The most important question, in regard to this PR, is if we need to define another guard time for such a case. If we do, should the guard time for late-to-set and early-to-set be the same? If it's the same then its fine, if not then we may need two sets of counter_get_guard_period, counter_set_guard_period functions, not one, to distinguish between different scenarios.

I'm confused by this new "early-to-set" concept.

@mnkp In my original conception and model, you can't have a late-to-set relative alarm because the offset is non-negative, so whatever value you end up with cannot be in the past. At the worst you pass zero, get the current counter value, and have to figure out how to make the alarm fire ASAP. That's not true for absolute alarms because you don't have an explicit zero (the current counter value, for relative) you can use to determine the relative offset, so you need a threshold (L in my terminology).

I thought we had agreement that the driver is required to be able to force an "as-soon-as-possible" invocation from ISR context even though the deadline had crept by, for cases where the user submits a late alarm and doesn't want a diagnostic and for relative alarms that are "short". This suggests there is no "short": if somebody passes zero (short on any driver) the driver implementer simply must make it work.

@nordic-krch has repeatedly stated that the guard period was not related to the hardware limitation that makes "short" alarms unavoidably "delayed" on Nordic.

It appears that this is all being made up as we go along, which is a lousy way to design APIs, especially with input only from a narrow group of people. Incredible amounts of wasted time on this PR could have been avoided if the whole thing was based on a well defined and accepted concept of what a counter is, what an alarm is, and how they interact normally, so we can share an understanding what "late-to-set" and "early-to-set" might mean and can precisely define their behavior.

[mnkp]

I'm confused by this new "early-to-set" concept.

In my original conception and model, you can't have a late-to-set relative alarm [...]

Yes, we already agreed and I stated so on a few occasions that late-to-set doesn't apply for relative alarms.

It looks like this PR handles late-to-set absolute alarms quite well but we still have an issue with short relative alarms. Setting relative alarm with value 0 means that the driver should schedule callback as soon as possible. But what if the driver needs to handle relative alarm with value 4. Can it read the current counter value, add 4, write it to the compare register and be sure that the callback will be executed? I called it "early-to-set" scenario. Also here we need a guard time. It should likely be different that the guard time for late-to-set absolute alarms since in between reading the current counter value and writing to the compare register we can block interrupts. This sequence of events is short and much more deterministic.

As I wrote already counter that runs on a predetermined clock can likely figure out this "early-to-set" guard time. That's more problematic for a counter that counts external events.

In any case, we should define the expected behavior so users of the API understand its behavior/limitations and the drivers can implement it. None does currently (also, not the one I recently contributed).

I've already made a proposal

We could add a restriction that relative alarm should be higher or equal to the guard time or otherwise, depending on the flag settings, the function will return an error or execute the callback immediately.

Returning an error in case of short relative alarm is probably not the right approach. Relative alarms are not precise anyway. Unless there are cases where the early-to-set guard time would be relatively large. Better approach would likely be to simply require driver to schedule the callback immediately or after the early-to-set guard time. I.e. If the value of a relative alarm is less than early-to-set guard time the driver always schedules a callback at a relative time equal to early-to-set guard time. This way it can be sure that the callback is executed.

What's important for this PR is that if we need another guard time that's different than the one already defined then we should use different naming. E.g. call it not 'guard period' but 'late guard period'.

[nordic-krch]

@mnkp

Can it read the current counter value, add 4, write it to the compare register and be sure that the callback will be executed? I called it "early-to-set" scenario. Also here we need a guard time.

As i pointed in option 3 here: https://github.com/zephyrproject-rtos/zephyr/pull/16252#issuecomment-502970676 Once you know that counter sets short relative alarm you can do something like:

disable_cc_interrupt();
do {
  set_compare_channel(now + rel_ticks + repeat_factor)
  now = counter_read()
  repeat_factor++
} while ( (diff(cc, now) < half_span) && !is_cc_int_pending())
enable_cc_interrupt();

because you know that rel_ticks is small compared to top_value.

[mnkp]

Once you know that counter sets short relative alarm you can do something like:

disable_cc_interrupt();
do {
  set_compare_channel(now + rel_ticks + repeat_factor)
  now = counter_read()
  repeat_factor++
} while ( (diff(cc, now) < half_span) && !is_cc_int_pending())
enable_cc_interrupt();

We can't use the while loop with increasing relative alarm. It may take several iterations to find out value that works and the sum of durations of all the attempts made will be ultimately non-deterministic and very different to what was originally requested.

I like the way you use disable_cc_interrupt() and is_cc_int_pending() however. If we write the procedure as

  t1 = counter_read();
  disable_cc_interrupt();
  set_compare_channel(t1 + rel_ticks);
  t2 = counter_read();
  if ((t2 - t1 >= rel_ticks) && !is_cc_int_pending()) {
    set_cc_int_pending();
  }
  enable_cc_interrupt();

it would work well on any hardware that allows to trigger an interrupt by writing to the interrupt set register. This is not a generic solution however.

[nordic-krch]

@mnkp if set_cc_int_pending(); is available (and at least on arm should be doable). My approach does not require it but as you pointed out it may take longer time. As long as interrupts are blocked then that's not necessary a problem.

Anyway, it seems to me that as for relative alarms we can leave without any guard period. Do you agree?

[mnkp]

Anyway, it seems to me that as for relative alarms we can leave without any guard period. Do you agree?

I agree, we can handle relative alarms later. Still, to be on the safe side, I propose to rename 'guard period' to 'late guard period' (or something else if you have a better name in mind). But let's leave possibility of adding another guard period in the future.

I have another proposal, let's remove COUNTER_ALARM_CFG_ERROR_WHEN_LATE flag and make it the default behavior. It's nice to have well behaved drivers. This approach will be backwards compatible. Drivers that don't implement set guard period function, will behave as if guard time was set to 0. That's equivalent to not having COUNTER_ALARM_CFG_ERROR_WHEN_LATE flag. Drivers that implement set guard period function will by default exhibit a safe behavior and report an error if the guard time was violated. Still, by setting the guard period to 0 user will be able to turn the check off.

[nordic-krch]

@mnkp

let's remove COUNTER_ALARM_CFG_ERROR_WHEN_LATE

Agree. Then we don't have conditional error code -ETIME.

Regarding renaming guard period. I'm leaning toward keeping it as it is. Personally, i don' t foreseen need to have another guard period in the future but if that is the case then we can always deprecate current calls and 'late' calls which will call the current one.

[mnkp]

let's remove COUNTER_ALARM_CFG_ERROR_WHEN_LATE

Agree. Then we don't have conditional error code -ETIME.

That's not what I proposed.

Regarding renaming guard period. I'm leaning toward keeping it as it is. Personally, i don' t foreseen need to have another guard period in the future but if that is the case then we can always deprecate current calls and 'late' calls which will call the current one.

We know that we need to deal with setting short relative alarms, ignoring the problem doesn't solve it. We should avoid deprecating functions and modifying the API if we can foreseen future changes. The proposals discussed so far are not good enough to be used as a general solution suitable to all architectures. In any case, this PR deals with late alarm setting so it makes sense to use name 'late guard period'.

[nordic-krch]

@mnkp

That's not what I proposed.

Did you mean to make it default behavior when late detection is enabled? And then have additional flag to enable immediate expiration instead of error returned?