topic-counter: missing support for cancelled alarm

[pabigot]

AFAICT there's no API to cancel an alarm once it's been set. This needs to be fixed.

One approach is to cancel the alarm by invoking counter_set_channel_alarm() with a null pointer for alarm_cfg. Or add a separate API function. I don't care much what color the bike shed is.

Whatever the API, the non-error result should indicate specifically whether the callback had been invoked before cancellation, but more generally the alarm state at the moment of cancellation, per #12624.

[nordic-krch]

what about counter_disable_ch_alarm?

[nordic-krch]

regarding returning state: i wouldn't like to not have it as return value as i prefer to have one value which means success. I'm also not convinced if that brings any value. User can track if callback was already called or not if needed. Moving that into the driver increases complexity.

[pabigot]

The assumption that the user can track whether the callback was invoked results in the user being required to track whether the callback was invoked, information that may not be readily available without recording it somewhere that both the callback and the alarm-set code can access. This is very similar to the argument underlying #11976: forcing a user to jump through hoops to reconstruct information that the infrastructure could have easily provided is unfriendly.

[pabigot]

As for counter_disable_ch_alarm this brings up an interesting question. Reading the API documentation in the header, it's not clear whether counter_set_ch_alarm is a one-shot configuration (as I expected), or a permanent configuration that will repeat when the counter wraps.

If the alarm is one-shot counter_cancel_ch_alarm would pair with counter_set_ch_alarm.

If the alarm is permanently installed then:

• the lack of ability to cancel is a huge API gap
• the API should be counter_disable_ch_alarm()
• counter_set_ch_alarm() should be renamed to counter_enable_ch_alarm().

In either case the API documentation must clearly describe the behavior so we don't have to make assumptions that end up being inconsistent across vendors.

[nordic-krch]

@pabigot alarm is single shot. I will extend documentation to make it clear. I fine with renaming 'counter_disable_ch_alarmtocounter_cancel_ch_alarm` (@anangl @mnkp @MaureenHelm any opinions?).

Regarding returning state. I see that it could be done but i would prefer to keep it as binary value (was or wasn't active when canceling occured) and keep it as function parameter (counter_cancel_alarm(dev, chan_id, &active)) and not as return value which should be 0 if success. Do you agree?

[pabigot]

Not really. I'd rather see #12624 implemented and return the state in an out argument.

The problem is, as you suggested elsewhere, that a higher-priority interrupt may occur during the counter callback. Assume an implementation where the alarm callback reschedules the alarm for a later count value, but a higher-priority ISR would normally want to adjust the alarm deadline.[*]

In the high-priority ISR it would need to distinguish these cases:

• If the alarm was scheduled but not reached, all is good and it can proceed with rescheduling.
• If the alarm had been reached but the handler not invoked, record somewhere that the alarm owner should process the unhandled alarm manually, and proceed with the rescheduling.
• If the alarm handler is currently being invoked the cancel must do nothing but return that fact so the ISR knows not to manipulate the alarm state, and communicate to the alarm owner through a back-channel that something bad happened.

Yes, that's a complicated use case. But Zephyr's supposed to be usable in production systems that will be complex. If it doesn't support what people want to do they won't want to use it.

[*] This reveals that just as we may need to know the state of an alarm that's been cancelled, we may need to know the state of an alarm that's being re-configured without being cancelled. The simplest way to do that is to reject attempts to set an alarm when one is already set. I don't believe behavior in that case is defined: it should return -EBUSY.

Another way is to define the behavior to be equivalent to atomically cancelling any alarm first and, if the cancellation is successful, proceeding to set the new alarm. There are complexities to be considered for that approach, not least including an out parameter to counter_set_ch_alarm to provide the state resulting from cancelling the alarm.

[nordic-krch]

i wonder if we can make it so such scenarios are handled by higher layer as in majority of cases (i guess) that complex use case will not be applied. By keeping that in the low level driver api we force all vendors to implement that. Maybe channel_status would be enough then the decision based on that would be on higher layer.

[pabigot]

A counter_ch_status() function alone checked before/after the API call isn't enough, but the channel status value, if captured synchronously within the driver and returned to the caller through the different API functions, should be sufficient to use for decisions.

I like the proposal to pass it through an output value; where the caller doesn't care a null pointer can be passed to indicate the value isn't needed.

[nordic-krch]

maybe instead of channel status we could some return remaining ticks? This gives more oportuninties, e.g. when restarting a channel you can adjust it to what was remaining.

[pabigot]

Remaining ticks might be useful in some cases, but it doesn't cover the set of states. E.g. by the time the cancellation is complete, or the ISR is executed, the counter might have already advanced, so there are negative ticks remaining. Also it doesn't represent the state of handler-being-invoked visible when a higher-priority ISR fetches status.