counter: Update counter API in order to provide more flexibility #14794

pizi-nordic · 2019-03-21T15:41:34Z

This PR introduces new "reset" parameter to the set_top_value()
making resetting counter optional during change of top value.

Such change allows for #12068 implementation on hardware which
does not provide alarms.

zephyrbot · 2019-03-21T15:47:53Z

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-03-22T06:34:38Z

include/counter.h

@@ -329,6 +330,7 @@ static inline int counter_cancel_channel_alarm(struct device *dev,
 * @retval -EBUSY if any alarm is active.


add -ENOTSUP error if option is not supported

nordic-krch · 2019-03-22T06:37:26Z

include/counter.h

@@ -329,6 +330,7 @@ static inline int counter_cancel_channel_alarm(struct device *dev,
 * @retval -EBUSY if any alarm is active.
 */
 static inline int counter_set_top_value(struct device *dev, u32_t ticks,
+					bool reset,


i wonder if it's not worth to bundle options to cfg structure like in counter_set_channel_alarm. It's then easier to add new features without braking backward compatibility (as long as false in option means legacy behavior). Now we exceed boundary of 4 arguments.

I think that with 5 arguments we are still more efficient that using separate data structure.

pizi-nordic · 2019-03-27T11:14:48Z

@nordic-krch: Could you please look at the last update?

include/counter.h

drivers/counter/timer_dtmr_cmsdk_apb.c

ioannisg

Looks ok to me. Does the counter test still pass with the modified API?

pizi-nordic · 2019-05-02T15:57:07Z

@ioannisg: As seen in the shippable.

ioannisg · 2019-05-02T15:58:26Z

@ioannisg: As seen in the shippable.

Does Shippable actually run this test on target?

anangl

The idea of adding the reset parameter is good, but I think it would be indeed better to use a structure for passing the parameters to the counter_set_top_value function, as @nordic-krch suggested earlier. In most cases it will be more efficient than passing 5 arguments to the function (4 via registers and the fifth via stack), especially when the contents of the structure will be constant, what I think can be expected to happen often. And we'll gain the flexibility mentioned by @nordic-krch. Please, consider it once more. The amount of work needed to switch to this approach does not seem to be that big. I can help a bit if needed.

nordic-krch · 2019-05-15T11:54:58Z

@pizi-nordic @anangl can you take a look as I've updated it to use configuration struct instead of function arguments.

include/counter.h

drivers/counter/counter_ll_stm32_rtc.c

drivers/counter/counter_rtc_qmsi.c

include/counter.h

carlescufi · 2019-06-11T12:37:25Z

@mnkp can you please take another look and approve if happy?
@andyross if you have had a chance to look at the counter driver API PRs, could you voice your opinion on this one?

carlescufi · 2019-06-11T12:40:29Z

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

nordic-krch · 2019-06-12T10:12:43Z

I've added following updates:

driver after changing top value checks if it does not go outside and return -ETIME if that happened.
added COUNTER_TOP_CFG_RESET_WHEN_LATE flag which additionally can instruct driver to reset counter when such situation occurs.

Added implementation for 4 drivers which can support that now:

counter_sam0_tc32.c @benpicco can you check?
counter_gecko_rtcc.c @mnkp can you check?
counter_nrfx_timer.c and counter_nrfx_rtc.c

benpicco · 2019-06-12T10:26:35Z

looks good to me.

nordic-krch · 2019-06-14T07:28:04Z

@mnkp can you take a look at the recent changes? Lets make goal to merge it in 3 months from submitting. One week left.

nordic-krch · 2019-06-14T07:28:59Z

@anangl could you also take a look? Since you approved there were some changes.

mnkp

@mnkp can you take a look at the recent changes? Lets make goal to merge it in 3 months from submitting. One week left.

Sorry, you're right complaining. The #16252 is introducing the concept of a guard time. I believe it would be best to reuse it. We would keep the API consistent.

mnkp · 2019-06-14T11:04:30Z

drivers/counter/counter_mcux_rtc.c

-	if (ticks != info->max_top_value) {
-		LOG_ERR("Wrap can only be set to 0x%x", info->max_top_value);
+	if ((cfg->ticks != info->max_top_value) ||
+		!(cfg->flags & COUNTER_TOP_CFG_DONT_RESET)) {


By default COUNTER_TOP_CFG_DONT_RESET is not set so this if statement will evaluate to true. This is change of behavior. I didn't check the datasheet but how come this counter doesn't start counting from 0 when it's reaching the top value?

i've digged into the manual. Counter can be reset or left free running. I've added implementation to support resetting the counter.

This is change of behavior

well, behavior was a bug according to counter_set_top_value description in master:
Function sets top value and resets the counter to 0 or top value depending on counter direction.

Now it will be clarified.

mnkp · 2019-06-14T11:09:10Z

drivers/counter/counter_gecko_rtcc.c

@@ -91,10 +91,12 @@ static u32_t counter_gecko_read(struct device *dev)
 }

 static int counter_gecko_set_top_value(struct device *dev, u32_t ticks,
-				       counter_top_callback_t callback,
-				       void *user_data)
+				       const struct counter_top_cfg *top_cfg)


Could we use cfg instead of top_cfg like in all other drivers. To make it more consistent.

anangl · 2019-06-14T12:08:23Z

drivers/counter/counter_nrfx_timer.c

+				    cfg->callback ? true : false);
+
+	if ((cfg->flags & COUNTER_TOP_CFG_DONT_RESET) &&
+		(counter_nrfx_read(dev) > cfg->ticks)) {


It should be >=.

anangl · 2019-06-14T12:17:32Z

drivers/counter/counter_nrfx_timer.c

+				    cfg->ticks, COUNTER_OVERFLOW_SHORT,
+				    cfg->callback ? true : false);
+
+	if ((cfg->flags & COUNTER_TOP_CFG_DONT_RESET) &&


Actually, even if the counter is reset, we can end up with the top value set lower than the current counter value. Although it could happen only for very low top values and accordingly high latencies between clearing the counter and setting the new compare value, but maybe such unusual scenario should be handled as well?

Cool. Should we use the same approach in counter_nrfx_rtc, for consistency and for people having the need to set top value to 1?

anangl · 2019-06-14T12:27:31Z

drivers/counter/counter_gecko_rtcc.c

@@ -91,10 +91,12 @@ static u32_t counter_gecko_read(struct device *dev)
 }

 static int counter_gecko_set_top_value(struct device *dev, u32_t ticks,


ticks is now a field of the counter_top_cfg structure. Does this compile successfully?

there is no test that uses that apparently. Fixed.

anangl · 2019-06-14T12:45:25Z

drivers/counter/counter_mcux_rtc.c

-		LOG_ERR("Wrap can only be set to 0x%x", info->max_top_value);
+	if (cfg->ticks != info->max_top_value){
+		LOG_ERR("Wrap can only be set to 0x%x. "
+			"Counter reset is not supported.", info->max_top_value);


The second part of the message is no longer valid.

drivers/counter/counter_nrfx_rtc.c

drivers/counter/counter_nrfx_timer.c

nordic-krch · 2019-06-18T12:26:38Z

@mnkp your comments applied. Anything more?

nordic-krch · 2019-06-24T08:03:50Z

@mnkp gentle ping.

mnkp

LGTM with minor comments.

drivers/counter/counter_nrfx_rtc.c

include/counter.h

This commit introduces new top_value setting configuration structure with flag for controlling resetting of the counter during change of top value. Such change allows for zephyrproject-rtos#12068 implementation on hardware which does not provide alarms. Signed-off-by: Piotr Zięcik <[email protected]> Signed-off-by: Krzysztof Chruscinski <[email protected]> Signed-off-by: Benjamin Valentin <[email protected]>

pizi-nordic requested review from ioannisg, nordic-krch and anangl March 21, 2019 15:41

pizi-nordic force-pushed the counter-api-update branch from 5bfcdcb to 21c43d3 Compare March 21, 2019 15:45

pizi-nordic force-pushed the counter-api-update branch from 21c43d3 to cbe1f20 Compare March 21, 2019 15:50

nordic-krch requested changes Mar 22, 2019

View reviewed changes

pizi-nordic force-pushed the counter-api-update branch 3 times, most recently from 206796d to de46337 Compare March 22, 2019 13:17

pizi-nordic force-pushed the counter-api-update branch from de46337 to 25adb2c Compare March 27, 2019 11:28

Sizurka mentioned this pull request Apr 2, 2019

drivers: counter: Add SAM0 basic counter support #15105

Merged

nordic-krch approved these changes Apr 17, 2019

View reviewed changes

galak added area: API Changes to public APIs area: Counter labels Apr 19, 2019

ioannisg reviewed Apr 29, 2019

View reviewed changes

include/counter.h Outdated Show resolved Hide resolved

ioannisg reviewed Apr 29, 2019

View reviewed changes

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

zephyrproject-rtos deleted a comment from codecov-io Apr 30, 2019

pizi-nordic force-pushed the counter-api-update branch from 25adb2c to 86d5134 Compare May 2, 2019 14:03

ioannisg approved these changes May 2, 2019

View reviewed changes

anangl reviewed May 6, 2019

View reviewed changes

nordic-krch force-pushed the counter-api-update branch from 86d5134 to 79e4c82 Compare May 15, 2019 11:51

pizi-nordic requested a review from nashif as a code owner May 15, 2019 11:51

anangl reviewed May 15, 2019

View reviewed changes

include/counter.h Outdated Show resolved Hide resolved

pizi-nordic commented May 15, 2019

View reviewed changes

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

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

include/counter.h Outdated Show resolved Hide resolved

nordic-krch force-pushed the counter-api-update branch from 79e4c82 to 4f3e5fe Compare May 15, 2019 12:13

nordic-krch force-pushed the counter-api-update branch from 3be3396 to 36dc9b6 Compare June 12, 2019 10:07

mnkp reviewed Jun 14, 2019

View reviewed changes

nordic-krch force-pushed the counter-api-update branch from 36dc9b6 to f549332 Compare June 14, 2019 11:44

anangl reviewed Jun 14, 2019

View reviewed changes

nordic-krch force-pushed the counter-api-update branch from f549332 to f7b04e8 Compare June 14, 2019 12:36

anangl reviewed Jun 14, 2019

View reviewed changes

nordic-krch force-pushed the counter-api-update branch 2 times, most recently from 645a321 to 039f9b9 Compare June 17, 2019 06:12

anangl requested changes Jun 17, 2019

View reviewed changes

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

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

nordic-krch force-pushed the counter-api-update branch from 039f9b9 to 9757265 Compare June 17, 2019 06:21

anangl approved these changes Jun 17, 2019

View reviewed changes

nordic-krch force-pushed the counter-api-update branch from 9757265 to 64a3667 Compare June 18, 2019 12:25

nordic-krch force-pushed the counter-api-update branch from 64a3667 to b6c4214 Compare June 19, 2019 06:02

mnkp reviewed Jun 24, 2019

View reviewed changes

drivers/counter/counter_nrfx_rtc.c Show resolved Hide resolved

include/counter.h Outdated Show resolved Hide resolved

include/counter.h Show resolved Hide resolved

nordic-krch force-pushed the counter-api-update branch from b6c4214 to 10329ce Compare June 25, 2019 06:27

mnkp approved these changes Jun 25, 2019

View reviewed changes

carlescufi merged commit 69406e0 into zephyrproject-rtos:master Jun 25, 2019

pizi-nordic deleted the counter-api-update branch July 11, 2019 14:30

		@@ -329,6 +330,7 @@ static inline int counter_cancel_channel_alarm(struct device *dev,
		* @retval -EBUSY if any alarm is active.

		@@ -91,10 +91,12 @@ static u32_t counter_gecko_read(struct device *dev)
		}

		static int counter_gecko_set_top_value(struct device *dev, u32_t ticks,

counter: Update counter API in order to provide more flexibility #14794

counter: Update counter API in order to provide more flexibility #14794

Conversation

pizi-nordic commented Mar 21, 2019

zephyrbot commented Mar 21, 2019 • edited Loading

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 commented Mar 27, 2019

ioannisg left a comment

Choose a reason for hiding this comment

pizi-nordic commented May 2, 2019

ioannisg commented May 2, 2019

anangl left a comment

Choose a reason for hiding this comment

nordic-krch commented May 15, 2019

carlescufi commented Jun 11, 2019

carlescufi commented Jun 11, 2019

nordic-krch commented Jun 12, 2019

benpicco commented Jun 12, 2019

nordic-krch commented Jun 14, 2019

nordic-krch commented Jun 14, 2019

mnkp left a 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

anangl Jun 14, 2019 • edited Loading

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

nordic-krch commented Jun 18, 2019

nordic-krch commented Jun 24, 2019

mnkp left a comment

Choose a reason for hiding this comment

zephyrbot commented Mar 21, 2019 •

edited

Loading

anangl Jun 14, 2019 •

edited

Loading