[DNM] [RFC] drivers: counter: API proposal unifying counter and rtc drivers. #8340

nordic-krch · 2018-06-12T12:36:18Z

Proposal of the API that would unify rtc.h and counter.h. It is based on existing counter.h and is backward compatible (counter_set_alarm renamed to counter_set_overflow, compatibility define added).

See related issue: #8331

API proposal extends counter.h API by:

adding function for triggering single shot alarms with relative and absolute value
allowing multiple single shot alarms (utilizing multiple capture/compare channels)
clarifying former counter_set_alarm behavior which had vague description and was usually interpreted as periodic overflow (impacting maximum value that can be read by counter_read() ). Renamed to counter_set_overflow().
adding functions for converting milliseconds and microseconds to ticks.

Signed-off-by: Krzysztof Chruściński [email protected]

This change is

nordic-krch · 2018-06-12T12:37:41Z

CC: @nvlsianpu @kl-cruz

nvlsianpu · 2018-06-12T12:50:30Z

cc @cvinayak

codecov-io · 2018-06-12T13:03:57Z

Codecov Report

Merging #8340 into topic-counters will decrease coverage by 4.76%.
The diff coverage is n/a.

@@                Coverage Diff                 @@
##           topic-counters    #8340      +/-   ##
==================================================
- Coverage           53.11%   48.34%   -4.77%     
==================================================
  Files                 218      264      +46     
  Lines               26868    42164   +15296     
  Branches             5952    10135    +4183     
==================================================
+ Hits                14271    20386    +6115     
- Misses              10162    17700    +7538     
- Partials             2435     4078    +1643

Impacted Files	Coverage Δ
subsys/net/ip/trickle.c	`64.83% <0%> (-12.8%)`	⬇️
subsys/net/ip/net_private.h	`70.58% <0%> (-12.75%)`	⬇️
subsys/net/ip/6lo.c	`70.28% <0%> (-9.14%)`	⬇️
subsys/net/ip/net_core.c	`65.78% <0%> (-7.74%)`	⬇️
subsys/net/ip/net_mgmt.c	`74.35% <0%> (-5.1%)`	⬇️
subsys/net/ip/ipv6.c	`53.69% <0%> (-5.03%)`	⬇️
subsys/net/lib/config/init.c	`57.77% <0%> (-4.73%)`	⬇️
subsys/net/l2/ethernet/arp.c	`55.22% <0%> (-4.13%)`	⬇️
subsys/net/ip/route.c	`47.2% <0%> (-3.36%)`	⬇️
subsys/net/ip/net_if.c	`62.46% <0%> (-3.31%)`	⬇️
... and 107 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 564f02e...e609d8b. Read the comment docs.

andrewboie · 2018-06-12T20:07:57Z

Can we make some of these APIs available to user mode?

pizi-nordic · 2018-06-14T11:51:17Z

include/counter.h

 typedef u32_t (*counter_api_get_pending_int)(struct device *dev);
+typedef int (*counter_api_ms_to_ticks)(struct device *dev,


Do we really need separate us/ms calls?
Could we just use us as a unit?
What about tick to ms/us conversion?

u32_t limits us. 2^32 microseconds is 4294 seconds. Ticks to ms/us - why not?

us_to_ticks is enough indeed.

What would be the use case for the reverse function?

us_to_ticks might not be enough. Consider the case when you have RTC prescaled to have 1hz freq and you want to get number of ticks for 12h timeout. 12h (1236001000000 us) needs 36 bits to represent. Unless we use u64_t for us argument.

is there such use case of counting for so many hours? (if so, then yes u64_t it would be)

I see a use case when you would like to put device to very long sleep (e.g. 2 days). Normally, you could use system timer, but it operates on system clock (e.g. 32 khz). 2 days on 32k does not fit in 32 bits. Then you can take counter.h implementation on RTC, set frequency to 1hz and then you can set alarms for much longer periods (more than a month).

In my opinion we should use 64-bit value with us as a unit. This gives us the most flexibility,

pizi-nordic · 2018-06-14T11:53:22Z

include/counter.h

- * counter initial value is set to zero. If it is a 'countdown' counter,
- * the initial value is set to the maximum value supported by the device.
+ * Start the counter device. The counter initial value is set to zero. Counter
+ * wraps at 0xffffffff.


What about about 16, 24 and 64-bit counters?
What about counting down counters?

A more general questions:

Are we going to create low-level API (exposing HW directly) or a higher level thing which implements a "generic" timer/counter?

Are we going to use such timer/counter as a system tick generator (now we need dedicated driver exposing specific API).

What about counting external events (not time related). How to configure counter clock source, prescaler and so on.

This API is meant to be generic. When it comes to hw specific stuff (such as clock source, etc...) that's up to the driver.

16, 24 bits counter -> that fits in 32.
Is there 64 bits counter on 32 bits arch? (could be unrelated, thus why I am asking)

counting down/up does not matter much, besides driver implementation that is. I guess.

yes, my plan was to hide 16,24 bits capability inside the driver. If hw does not support it then it must emulate it. Otherwise we loose abstraction. And if user is aware of hw limitation set_overflow can set top value within the limiation and emulation will not be used.

64 bits - i don't know. Any generic interface cuts extremes. If 64bit general purpose timer is present and needed it will have to go through custom driver. If zephyr supports more and more devices that have 64 bit timers i would prefer that API is extended with dedicated 64 bit functions.

Counting up/down matters if you would like to specify absolute value. If current is tick count is 100 and you specify 200 as a next alarm, you will get different results when your counter counts up and down :) Of course we can assume that our timer model always counts up (then lower level will have to emulate that).

Regarding bits emulation: This will be tricky. We cannot just shift timer value, as we loose precision in such case. Without shift, we will have to support alarms set over several timer overflows and similar corner cases. This will result in number of very similar but complex timer drivers. Moreover such "emulation" already exists in the upper layers of the system (look for kernel timers).

IMHO we will need more than 1 layer here:

Abstraction of HW (number of bits, up/down, frequency, prescaler/clock source selection, callback on compare and overflow).

Generic layer translating HW into generic timer (32-bit, always up, multiple single-shoot and periodic notifications)

Interface allowing selecting one of generic timers as a system clock source.

Interface allowing using selected generic timer in userspace.

I agree that 32bit emulation is generic and could be done by layer above the driver to make the driver simpler. Probably same with up/down. Configuration of counting source (clock vs other events) is hw dependent and should be done outside (e.g. kconfig option specific to driver implementation or external call to lower layer driver).

In order to make counter.h closer to hw and allow building generic timer on that it would need following changes:

renaming counter_set_overflow to something like counter_set_top() which would set overflow/reload value depending on direction.

adding function counter_get_max_top() which would return capacity of the driver.

adding counter_get_max_alarm() which would return maximal value that can be user for alarm (duration and period)

adding counter_get_dir() - up or down. I assume that direction would be fixed and configured at compile time (if hw supports up and down)

I also see now a need to add ticks_to_us(). It is needed to put counter_read() value on scale.

pizi-nordic · 2018-06-14T12:07:34Z

@andrewboie: How do you see handling that in the userspace? Are you thinking about interrupt-like interface (similar to Unix signals) or just about ability to wakeup a thread when event will be emitted?

tbursztyka · 2018-06-13T08:19:55Z

include/counter.h

+	bool abs;
+	u32_t ticks;
+	counter_single_shot_callback_t callback;
+	void *user_data;	


Couldn't we do like in gpio: user CONTAINER_OF(), the callback would pass the pointer of the struct counter_single_shot then, saves a pointer on function signature and in this struct.

tbursztyka · 2018-06-13T08:25:30Z

include/counter.h

+					       void *user_data);
+
+struct counter_single_shot {
+	/** Index of the request. Maximal index is implementation dependent.*/


you meant device driver dependent? maybe it's better to hide that, and if there are slots: fine, if not then return an error.

tbursztyka · 2018-06-14T12:31:44Z

include/counter.h

+	/** Index of the request. Maximal index is implementation dependent.*/
+	u8_t id;
+	/** If true ticks are treated as absolute value, else it is relative to
+	 * the counter reading performed during the call.


I would assume relative should be the only way. I don't get why there would be 2 different ways.

Absolute value is needed to implement precise periodic timer as you have to schedule new event in relation to last event compare value, not current tick count.

tbursztyka · 2018-06-14T12:34:37Z

include/counter.h

@@ -132,7 +205,7 @@ static inline int counter_set_alarm(struct device *dev,
 *
 * @param dev Pointer to the device structure for the driver instance.
 *
- * @retval 1 if the counter interrupt is pending.
+ * @retval 1 if the counter any interrupt is pending.
 * @retval 0 if no counter interrupt is pending.
 */
 __syscall int counter_get_pending_int(struct device *dev);


As we are at reworking the whole API, can we challenge that function? is it really useful?

I am fairly new to zephyr. I've seen that it is mainly used to determine wakeup source. Is it only for testing?

I am not sure why it's there. We have it also for gpio, aio... But it's unused anywhere, I don't see why user code would need to know about pending interrupts, moreover that we are fully interrupt based. Unless you lock interrupt for a long time, and want to behave as a polling-based code.
The only code using it is a PM test. I kind of get why it does that there, but perhaps there is another way.
Let's see next device driver API meeting, will put that on the agenda.

What's the conclusion, is this function to stay?

tbursztyka · 2018-06-14T12:35:48Z

include/counter.h

+{
+	struct counter_driver_api *api;
+
+	api = (struct counter_driver_api *)dev->driver_api;


assign api at its declaration, like it's done elswhere.

tbursztyka · 2018-06-14T12:35:56Z

include/counter.h

+{
+	struct counter_driver_api *api;
+
+	api = (struct counter_driver_api *)dev->driver_api;


tbursztyka · 2018-06-14T12:38:00Z

include/counter.h

 typedef u32_t (*counter_api_get_pending_int)(struct device *dev);
+typedef int (*counter_api_ms_to_ticks)(struct device *dev,


us_to_ticks is enough indeed.

What would be the use case for the reverse function?

tbursztyka · 2018-06-14T12:38:56Z

include/counter.h

+typedef int (*counter_api_ms_to_ticks)(struct device *dev,
+				       u32_t ms,
+				       u32_t *ticks );
+typedef int (*counter_api_us_to_ticks)(struct device *dev,


I would just return the ticks. I don't see any error that could happen (maybe I am wrong)

returning ticks would simplify conversion. The only thing that could go wrong is when requested ms/us does not fit in 32b ticks. In that case function would saturate at max ticks. Of course, having ticks as return value has 'style' advantage ( counter_set_overflow(... counter_us_to_ticks(1000))).

@tbursztyka: We will need reverse functions for precise time measurement. We can use k_cycle_32_get() for that, but on some architectures this is bound to the most energy efficient timer (read: low frequency). There are faster timers in the system, but there is no official API for controlling them.

andrewboie · 2018-06-14T19:53:39Z

@andrewboie: How do you see handling that in the userspace?

Same APIs just write handlers for those APIs which can be exposed to user mode. There's lots of example of this in the other driver subsystems.

Looking at this API, you could expose all the APIs which don't take callback functions or struct counter_single_shot as parameters (since we don't allow registering callbacks which run in IRQ context from user mode).

Don't need to do it in this PR, can leave for another day.

tbursztyka · 2018-06-15T07:59:57Z

I am considering the fact that we may get rid of the ticks/count exposure altogether. Instead user would provide 1 unique parameter: time. Time is something generic that user understands, and thus would not have to manage ticks directly. We have to get decided on µs or ms. I guess there is a use-case for µs so we would have to stick with it, and thus maybe u64_t. Function could return -ERANGE if really it can't handle given time (i.e.: time to ticks below in driver overflows). What do you think?

pizi-nordic · 2018-06-15T10:49:07Z

Time is valid for timer, which is a counter counting clock cycles. But counter could also count some other things. If we would like to have unified API for all type of counters (including timers), we will have to provide access to raw ticks.

nordic-krch · 2018-06-15T12:22:14Z

Pushed update:

changed counter_us_to_ticks to get u64_t us parameter. Removed counter_ms_to_ticks.
renamed counter_set_single_shot to counter_set_alarm. Changed a bit functionality: alarm parameters contain now duration and period instead of ticks (similar to k_timer_start). So alarm can be single shot or periodic, kept abs flag.

The CMSDK Timer can be used as a timer or as a counter. The unified interface proposed in zephyrproject-rtos#8340 unifies counter.h and rtc.h to provide a common interface. This patch modifies the timer implementation of the single timer to make it compliant with the new proposed interface. Signed-off-by: Vincenzo Frascino <[email protected]> Signed-off-by: Kumar Gala <[email protected]>

The CMSDK Dual Timer can be used as a timer or as a counter. The unified interface proposed in zephyrproject-rtos#8340 unifies counter.h and rtc.h to provide a common interface. This patch modifies the timer implementation of the dual timer to make it compliant with the new proposed interface. Signed-off-by: Vincenzo Frascino <[email protected]> Signed-off-by: Kumar Gala <[email protected]>

The CMSDK Timer can be used as a timer or as a counter. The unified interface proposed in #8340 unifies counter.h and rtc.h to provide a common interface. This patch modifies the timer implementation of the single timer to make it compliant with the new proposed interface. Signed-off-by: Vincenzo Frascino <[email protected]> Signed-off-by: Kumar Gala <[email protected]>

The CMSDK Dual Timer can be used as a timer or as a counter. The unified interface proposed in #8340 unifies counter.h and rtc.h to provide a common interface. This patch modifies the timer implementation of the dual timer to make it compliant with the new proposed interface. Signed-off-by: Vincenzo Frascino <[email protected]> Signed-off-by: Kumar Gala <[email protected]>

The CMSDK Timer can be used as a timer or as a counter. The unified interface proposed in #8340 unifies counter.h and rtc.h to provide a common interface. This patch modifies the timer implementation of the single timer to make it compliant with the new proposed interface. Signed-off-by: Vincenzo Frascino <[email protected]> Signed-off-by: Kumar Gala <[email protected]>

The CMSDK Dual Timer can be used as a timer or as a counter. The unified interface proposed in #8340 unifies counter.h and rtc.h to provide a common interface. This patch modifies the timer implementation of the dual timer to make it compliant with the new proposed interface. Signed-off-by: Vincenzo Frascino <[email protected]> Signed-off-by: Kumar Gala <[email protected]>

The CMSDK Timer can be used as a timer or as a counter. The unified interface proposed in #8340 unifies counter.h and rtc.h to provide a common interface. This patch modifies the timer implementation of the single timer to make it compliant with the new proposed interface. Signed-off-by: Vincenzo Frascino <[email protected]> Signed-off-by: Kumar Gala <[email protected]>

The CMSDK Dual Timer can be used as a timer or as a counter. The unified interface proposed in #8340 unifies counter.h and rtc.h to provide a common interface. This patch modifies the timer implementation of the dual timer to make it compliant with the new proposed interface. Signed-off-by: Vincenzo Frascino <[email protected]> Signed-off-by: Kumar Gala <[email protected]>

nordic-krch added RFC Request For Comments: want input from the community DNM This PR should not be merged (Do Not Merge) labels Jun 12, 2018

nordic-krch requested a review from anangl as a code owner June 12, 2018 12:36

nordic-krch changed the title ~~drivers: counter: API proposal unifying counter and rtc drivers.~~ [DNM][RFC] drivers: counter: API proposal unifying counter and rtc drivers. Jun 12, 2018

nordic-krch changed the title ~~[DNM][RFC] drivers: counter: API proposal unifying counter and rtc drivers.~~ [DNM] [RFC] drivers: counter: API proposal unifying counter and rtc drivers. Jun 12, 2018

carlescufi requested review from pizi-nordic, tbursztyka, erwango, cvinayak and galak June 12, 2018 16:33

carlescufi added the area: API Changes to public APIs label Jun 12, 2018

carlescufi requested review from ioannisg, andrewboie and andyross June 12, 2018 16:35

MaureenHelm requested review from MaureenHelm, agross-oss and dleach02 June 12, 2018 16:37

pizi-nordic reviewed Jun 14, 2018

View reviewed changes

tbursztyka previously requested changes Jun 14, 2018

View reviewed changes

nordic-krch force-pushed the counter_rtc_unification branch from 96c1430 to 511ffbc Compare June 15, 2018 12:17

SavinayDharmappa self-requested a review June 17, 2018 14:46

stanislav-poboril mentioned this pull request Nov 16, 2018

i.MX EPIT Timer Driver #11441

Merged

MaureenHelm added area: Counter area: API Changes to public APIs and removed area: API Changes to public APIs labels Nov 28, 2018

MaureenHelm mentioned this pull request Nov 28, 2018

drivers: counter: Add user_data to alarm callback #11572

Merged

pabigot mentioned this pull request Dec 9, 2018

APIs that support a callback should provide both the device pointer and a generic pointer #11976

Closed

		typedef u32_t (counter_api_get_pending_int)(struct device dev);
		typedef int (counter_api_ms_to_ticks)(struct device dev,

[DNM] [RFC] drivers: counter: API proposal unifying counter and rtc drivers. #8340

[DNM] [RFC] drivers: counter: API proposal unifying counter and rtc drivers. #8340

Conversation

nordic-krch commented Jun 12, 2018 • edited Loading

nordic-krch commented Jun 12, 2018

nvlsianpu commented Jun 12, 2018

codecov-io commented Jun 12, 2018 • edited Loading

Codecov Report

andrewboie commented Jun 12, 2018

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

pizi-nordic Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pizi-nordic Jun 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pizi-nordic commented Jun 14, 2018

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

andrewboie commented Jun 14, 2018

tbursztyka commented Jun 15, 2018

pizi-nordic commented Jun 15, 2018

nordic-krch commented Jun 15, 2018

nordic-krch commented Jun 12, 2018 •

edited

Loading

codecov-io commented Jun 12, 2018 •

edited

Loading

pizi-nordic Jun 14, 2018 •

edited

Loading

pizi-nordic Jun 15, 2018 •

edited

Loading