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

nordic-krch · 2019-05-20T08:40:47Z

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

drivers/counter/counter_ll_stm32_rtc.c

include/counter.h

tests/drivers/counter/counter_basic_api/src/test_counter.c

pabigot · 2019-05-20T22:38:35Z

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 · 2019-05-21T12:42:12Z

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 · 2019-05-21T12:43:16Z

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

pabigot · 2019-05-21T13:48:00Z

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 · 2019-05-22T13:11:07Z

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 · 2019-05-22T13:26:21Z

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

nordic-krch · 2019-05-24T12:20:24Z

@pabigot @anangl @erwango @MaureenHelm ping

pabigot

I entirely support switching from the single bool absolute to a set of flags that control various counter options.

However, as long as there's no clearly defined and accepted counter model that can be used to describe and assess lateness policies and against which vendors can implement, I don't find this feature, or the counter API as a whole, to be robust and useful.

For example, I believe that as long as a relative alarm is specified by a non-negative value lateness should be clearly and universally understood: an offset strictly less than an inspectable minimum advance required to guarantee a compare event is always late; and any offset greater than that minimum advance is never late. This must hold regardless of any latency between the point the application tries to set the alarm and the point where the driver sets the compare value. It's the responsibility of the implementation to comply with this, which for Nordic might mean that the minimum advance would be 3 rather than 2. There is no need for threshold configurability in relative alarms.

Lateness for absolute alarms then reduces to checking the derived relative offset at the point the compare hardware is configured, which is trivial but suggests another behavior option that I won't get into here.

PR #14794, which I have not looked at closely, also appears to be weakened by lack of a counter model to use to assess proposed API.

I stand by my earlier comments, and will neither approve nor reject this.

mnkp

Looks good with some minor comments. I think we need to rebase this PR on the latest master. There are two new drivers in the tree counter_gecko_rtcc.c, counter_sam0_tc32.c which will not work with the new COUNTER_ALARM_CFG_ABSOLUTE flag this PR is introducing.

mnkp · 2019-06-24T19:47:34Z

include/counter.h

- *		converted to microseconds (see @ref counter_ticks_to_us).
- *		Alternatively, counter implementation may count asynchronous
- *		events.
+ * @param ticks Ticks that triggers the alarm. It can be relative (to now) or


Seems like suggestion from @anangl hasn't been implemented.

include/counter.h

nordic-krch · 2019-06-25T06:58:22Z

@mnkp counter_gecko_rtcc.c, counter_sam0_tc32.c updated.
Changed description so that setting alarm will always return -ETIME if late setting is detected.

anangl · 2019-06-26T12:17:20Z

drivers/counter/counter_handlers.c

@@ -23,3 +23,5 @@ COUNTER_HANDLER(stop)
 COUNTER_HANDLER(start)
 COUNTER_HANDLER(get_top_value)
 COUNTER_HANDLER(get_max_relative_alarm)
+COUNTER_HANDLER(get_guard_period)
+COUNTER_HANDLER(set_guard_period)


As the description of COUNTER_HANDLER says, it is "for those APIs that just take one argument which is a counter driver instance and return an integral value". So both these lines are incorrect.

right, i've tried to change it but i'm not familiar with user space macros so please look again.

anangl · 2019-06-26T13:47:10Z

include/counter.h

+ * @brief Identifies guard period needed for detection of late setting of
+ *	  absolute alarm (see @ref counter_set_channel_alarm).
+ */
+#define COUNTER_GUARD_PERIOD_LATE_ABS BIT(0)


I'm not convinced about this name, especially this _ABS part. What about _LATE_TO_SET or _LATE_DETECTION?

+1 for COUNTER_GUARD_PERIOD_LATE_TO_SET

changed to COUNTER_GUARD_PERIOD_LATE_TO_SET, wanted to stress that it is related to absolute alarms.

anangl · 2019-06-26T13:48:21Z

include/counter.h

 /** @brief Alarm callback
 *
 * @param dev       Pointer to the device structure for the driver instance.
 * @param chan_id   Channel ID.
- * @param ticks     Counter value that triggered the callback.
+ * @param ticks     Counter value that triggers the alarm.


I think the past tense was good in this description as it is for the parameter passed to the callback.

anangl · 2019-06-26T13:50:24Z

include/counter.h

@@ -460,6 +501,71 @@ static inline u32_t z_impl_counter_get_max_relative_alarm(struct device *dev)
 	return api->get_max_relative_alarm(dev);
 }

+/**
+ * @brief Set guard period in ticks.


"counter ticks", to avoid confusion with kernel ticks. Or maybe it would be better to omit "in ticks" here (it is stated in the parameter description).

anangl · 2019-06-26T13:51:07Z

include/counter.h

+ * @param dev	Pointer to the device structure for the driver instance.
+ * @param flags	See @ref COUNTER_GUARD_PERIOD_FLAGS.
+ *
+ * @return Guard period given in ticks.


"counter ticks", to avoid confusion with kernel ticks.

anangl · 2019-06-26T13:57:54Z

include/counter.h

+ *
+ * @retval 0 if successful.
+ * @retval -ENOTSUP if not supported.
+ * @retval -EINVAL if invalid guard.


These descriptions are too ambiguous. Which one of these error values should be returned when an unknown flag is supplied?

changed to:
-ENOTSUP if function is not supported.
-EINVAL if invalid flags provided.

anangl · 2019-06-26T14:08:53Z

include/counter.h

+ * See @ref counter_set_guard_period.
+ *
+ * @param dev	Pointer to the device structure for the driver instance.
+ * @param flags	See @ref COUNTER_GUARD_PERIOD_FLAGS.


What is the expected behavior when given flag is not supported? I assume it is safe to return 0, as when the function is not implemented. But maybe it would be better to state it clearly?

changed to Guard period given in counter ticks or 0 if function or flags are not supported.

mnkp · 2019-06-26T16:20:09Z

include/counter.h

+ * @brief Identifies guard period needed for detection of late setting of
+ *	  absolute alarm (see @ref counter_set_channel_alarm).
+ */
+#define COUNTER_GUARD_PERIOD_LATE_ABS BIT(0)


+1 for COUNTER_GUARD_PERIOD_LATE_TO_SET

mnkp · 2019-06-26T16:26:47Z

include/counter.h

+ * After initialization guard period is set to 0 and late detection is disabled.
+ *
+ * @param dev		Pointer to the device structure for the driver instance.
+ * @param guard		Guard period in counter ticks.


Since we are going to update this PR anyway, what about using period or guard_period instead of guard as a parameter name. Guard seems too ambiguous/misleading.

what about using period or guard_period instead of guard as a parameter name.

Or maybe ticks?

changed to ticks

anangl · 2019-07-01T11:04:18Z

include/drivers/counter.h

@@ -145,6 +180,9 @@ typedef int (*counter_api_set_top_value)(struct device *dev,
 typedef u32_t (*counter_api_get_pending_int)(struct device *dev);
 typedef u32_t (*counter_api_get_top_value)(struct device *dev);
 typedef u32_t (*counter_api_get_max_relative_alarm)(struct device *dev);
+typedef u32_t (*counter_api_get_guard_period)(struct device *dev, u32_t flags);
+typedef int (*counter_api_set_guard_period)(struct device *dev, u32_t guard,


guard -> ticks

anangl · 2019-07-01T11:27:14Z

include/drivers/counter.h

+ *
+ * @retval 0 if successful.
+ * @retval -ENOTSUP if function is not supported.
+ * @retval -EINVAL if invalid flags provided.


I think that for both these cases -ENOTSUP should be returned, and -EINVAL should rather indicate an attempt to set an invalid guard period (> top_value is not valid in any case, I'm not sure if for some drivers some more limitation won't be needed).

anangl · 2019-07-01T11:29:17Z

include/drivers/counter.h

+ * @brief Set guard period in counter ticks.
+ *
+ * Setting non-zero guard period enables detection of setting absolute alarm
+ * too late. It limits how far in the future absolute alarm can be set.


Perhaps it would be good to explicitly state that setting the guard period to 0 disables the detection, or is it obvious?

Setting non-zero guard period enables detection of setting absolute alarm too late. i feel like stating that setting 0 means disabling is redundant.

andrewboie · 2019-07-01T15:31:01Z

drivers/counter/counter_gecko_rtcc.c

@@ -172,7 +172,7 @@ static int counter_gecko_set_alarm(struct device *dev, u8_t chan_id,
 		return -EBUSY;
 	}

-	if (alarm_cfg->absolute) {
+	if (alarm_cfg->flags & COUNTER_ALARM_CFG_ABSOLUTE) {


Misra-C pendantry: need to write this as ((alarm_cfg->flags & COUNTER_ALARM_CFG_ABSOLUTE) != 0), here and elsewhere

done for all drivers.

andrewboie · 2019-07-01T15:37:43Z

include/drivers/counter.h

+	const struct counter_driver_api *api =
+				(struct counter_driver_api *)dev->driver_api;
+
+	if (!api->set_guard_period) {


This isn't typical of Zephyr driver subsystems, normally if an API isn't implemented we don't do a NULL check. We might want to consider a consistent system-wide strategy on this.

this approach allows extending api without breaking existing drivers. It's not the first place it is used though i'm not sure how consistent zephyr is in that matter.

i'm not sure how consistent zephyr is in that matter.

It's not, AFAICT.
Fine with this as-is, can address kernel-wide later which direction to go (and codify somewhere)

MaureenHelm

Please rebase and fix the merge conflict

Flags in alarm configuration structure will allow further extention without breaking API. Initially, existing absolute flag was added as the only flag. Signed-off-by: Krzysztof Chruscinski <[email protected]>

Added flags for controling detection of setting alarm to late. Updated drivers to return -ENOTSUP when new option is requested. Signed-off-by: Krzysztof Chruscinski <[email protected]>

nordic-krch · 2019-07-09T09:20:26Z

@ioannisg @carlescufi can you merge that one?

ioannisg

lgtm

nordic-krch added area: Counter area: API Changes to public APIs labels May 20, 2019

nordic-krch requested review from pabigot, MaureenHelm, galak, anangl, pizi-nordic and erwango May 20, 2019 08:40

nordic-krch force-pushed the counter_alarm_flags branch from 1292ba3 to 90044c6 Compare May 20, 2019 08:45

anangl reviewed May 20, 2019

View reviewed changes

drivers/counter/counter_ll_stm32_rtc.c Outdated Show resolved Hide resolved

include/counter.h Outdated Show resolved Hide resolved

nordic-krch force-pushed the counter_alarm_flags branch from 90044c6 to 2db668a Compare May 20, 2019 10:43

nordic-krch requested a review from nashif as a code owner May 20, 2019 10:43

nordic-krch force-pushed the counter_alarm_flags branch from 2db668a to 7cc325e Compare May 20, 2019 10:45

anangl reviewed May 20, 2019

View reviewed changes

tests/drivers/counter/counter_basic_api/src/test_counter.c Outdated Show resolved Hide resolved

nordic-krch force-pushed the counter_alarm_flags branch 2 times, most recently from 5d973e9 to 79570ac Compare May 20, 2019 12:44

nordic-krch mentioned this pull request May 20, 2019

drivers: counter: Add detection of late alarm request for nRF RTC and TIMER drivers #15510

Merged

nordic-krch force-pushed the counter_alarm_flags branch from 79570ac to cd68ea4 Compare May 21, 2019 06:20

nordic-krch force-pushed the counter_alarm_flags branch from cd68ea4 to 9bfe6e2 Compare May 22, 2019 12:55

nordic-krch force-pushed the counter_alarm_flags branch from 9bfe6e2 to 61d0323 Compare May 22, 2019 13:25

nordic-krch force-pushed the counter_alarm_flags branch from 61d0323 to 94acb0d Compare May 23, 2019 05:35

pabigot reviewed May 24, 2019

View reviewed changes

mnkp reviewed Jun 24, 2019

View reviewed changes

nordic-krch force-pushed the counter_alarm_flags branch 2 times, most recently from 9a842b0 to a8107e6 Compare June 25, 2019 06:57

pizi-nordic approved these changes Jun 25, 2019

View reviewed changes

nordic-krch force-pushed the counter_alarm_flags branch from a8107e6 to a639579 Compare June 25, 2019 09:11

mnkp approved these changes Jun 25, 2019

View reviewed changes

anangl requested changes Jun 26, 2019

View reviewed changes

mnkp reviewed Jun 26, 2019

View reviewed changes

nordic-krch force-pushed the counter_alarm_flags branch from a639579 to 61f8ece Compare July 1, 2019 10:58

anangl reviewed Jul 1, 2019

View reviewed changes

nordic-krch force-pushed the counter_alarm_flags branch from 61f8ece to 6f91167 Compare July 1, 2019 11:05

anangl reviewed Jul 1, 2019

View reviewed changes

nordic-krch force-pushed the counter_alarm_flags branch from 6f91167 to 3415466 Compare July 1, 2019 12:08

anangl approved these changes Jul 1, 2019

View reviewed changes

andrewboie reviewed Jul 1, 2019

View reviewed changes

nordic-krch force-pushed the counter_alarm_flags branch 2 times, most recently from cc0aa77 to 333ca0c Compare July 2, 2019 08:36

MaureenHelm approved these changes Jul 8, 2019

View reviewed changes

andrewboie approved these changes Jul 8, 2019

View reviewed changes

nordic-krch added 2 commits July 9, 2019 10:29

drivers: counter: Add optional flags to alarm configuration structure

127a6c7

Flags in alarm configuration structure will allow further extention without breaking API. Initially, existing absolute flag was added as the only flag. Signed-off-by: Krzysztof Chruscinski <[email protected]>

drivers: counter: Extend set channel alarm flags

e76fbfe

Added flags for controling detection of setting alarm to late. Updated drivers to return -ENOTSUP when new option is requested. Signed-off-by: Krzysztof Chruscinski <[email protected]>

nordic-krch force-pushed the counter_alarm_flags branch from 333ca0c to e76fbfe Compare July 9, 2019 08:30

ioannisg added the Enhancement Changes/Updates/Additions to existing features label Jul 10, 2019

ioannisg approved these changes Jul 10, 2019

View reviewed changes

MaureenHelm merged commit 1baae96 into zephyrproject-rtos:master Jul 10, 2019

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

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

Conversation

nordic-krch commented May 20, 2019

pabigot commented May 20, 2019 • edited Loading

nordic-krch commented May 21, 2019

nordic-krch commented May 21, 2019

pabigot commented May 21, 2019

zephyrbot commented May 22, 2019 • edited Loading

nordic-krch commented May 22, 2019

nordic-krch commented May 24, 2019

pabigot left a comment

Choose a reason for hiding this comment

mnkp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nordic-krch commented Jun 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaureenHelm left a comment

Choose a reason for hiding this comment

nordic-krch commented Jul 9, 2019

ioannisg left a comment

Choose a reason for hiding this comment

pabigot commented May 20, 2019 •

edited

Loading

zephyrbot commented May 22, 2019 •

edited

Loading