drivers: counter: add support for top value configuration on native_posix #63897

jpwright · 2023-10-13T02:56:15Z

The counter_native_posix driver currently does not support top value configuration, i.e. ctr_set_top_value returns -ENOTSUP. This commit adds support for top value configuration, and with the counter API now fully implemented, adds counter to supported peripherals for native_posix target.

aescolar

Thanks @jpwright
I have a few fix requests.
(Note that @nordic-krch is the maintainer for the counter driver itself)

boards/posix/native_posix/hw_counter.c

boards/posix/native_posix/native_posix.yaml

drivers/counter/counter_native_posix.c

boards/posix/native_posix/hw_counter.c

drivers/counter/counter_native_posix.c

nordic-krch · 2023-10-16T11:01:54Z

drivers/counter/counter_native_posix.c

+	}
+
+	top = *cfg;
+	is_top_set = true;


I'm not sure how native_posix timer works but what if newly set value is TOP_VALUE without callback? Shouldn't it be reset to false and interrupt disabled?

I initially didn't add that because it's not mentioned in Counter API docs and the set_alarm function has no such behavior. But, I agree it's a good idea (without it there's no way of setting an alarm after setting a top value without re-init), so I've added it in. Not sure if there is a good place to document that behavior or if it's already mentioned on some other page though.

aescolar

Thanks again @jpwright
A few more comments from me.
Please note that my knowledge about the counter API is just based on its documentation, so please be critical about my feedback.

boards/posix/native_posix/hw_counter.c

drivers/counter/counter_native_posix.c

aescolar · 2023-10-17T10:11:59Z

drivers/counter/counter_native_posix.c

+	pending_alarm = *alarm_cfg;
+	pending_alarm.ticks = ticks;
+	is_alarm_pending = true;
+
 	hw_counter_set_target(pending_alarm.ticks);


This would seem incorrect, as alarm.ticks may be after a wrap (due to either top.ticks or TOP_VALUE) (see the expression in the other comment about finding the next hit)

drivers/counter/counter_native_posix.c

aescolar · 2023-10-17T10:34:48Z

(Sidetangent) @nordic-krch Note that it seems the documentation in the API of counter_set_guard_period() is self contradictory.

drivers/counter/counter_native_posix.c

jpwright

thanks @aescolar for the help and additional feature! I grabbed your commit and had two questions on it, happy to address on my fork or pull an update from yours either way.

jpwright · 2023-10-19T03:06:56Z

drivers/counter/counter_native_posix.c

-	is_alarm_pending = true;
+	uint32_t ticks = alarm_cfg->ticks;
+
+	if (ticks > top.ticks) {


Should this check not happen after handling COUNTER_ALARM_CFG_ABSOLUTE? If ticks is relative to current_value, the alarm will never be reached.

Should this check not happen after handling COUNTER_ALARM_CFG_ABSOLUTE?

I think not. The API description is not clear about this, but from how the test behaves here:
https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/drivers/counter/counter_basic_api/src/test_counter.c#L396-L400
the test expects the driver to reject a relative alarm with a tick offset > top.ticks

jpwright · 2023-10-19T03:10:28Z

drivers/counter/counter_native_posix.c

+	pending_alarm[chan_id].ticks = ticks;
+	is_alarm_pending[chan_id] = true;
+
+	schedule_next_isr();


Do we also need to call hw_counter_set_wrap_value here? Previous top configuration may have set it less than the alarm ticks.

I'm not sure I follow. The wrap is configured at init, or every time ctr_set_top_value() is called successfully.
And the hw wrap remains in effect after that for ever.

Apart from that, we are checking in 189 and 199 that the alarm target does not go beyond the top.tick / wrap_value.

addressed

aescolar · 2023-10-19T07:35:25Z

Adding DNM to avoid accidental merge before we get the native simulator changes in

aescolar

I'm ok with the PR as it stands
@nordic-krch are you ok with it too?

@jpwright Note I just pushed to your branch updating the PR with an update to the native simulator to match the native_posix hw counter addons. I hope you don't mind, it should just save a bit of time to everybody. (those changes are now in main)

jpwright · 2023-10-21T19:14:58Z

@jpwright Note I just pushed to your branch updating the PR with an update to the native simulator to match the native_posix hw counter addons. I hope you don't mind, it should just save a bit of time to everybody.

Looks good, thank you!

The counter_native_posix driver currently does not support top value configuration, i.e. `ctr_set_top_value` returns `-ENOTSUP`. This commit adds support for top value configuration, and with the counter API now fully implemented, adds `counter` to `supported` peripherals for native_posix target. It also resolves an existing bug in which the counter ISR did not reset upon reaching `TOP_VALUE`. And adds support for multiple channels Signed-off-by: Jason Wright <[email protected]> Signed-off-by: Alberto Escolar Piedras <[email protected]>

aescolar · 2023-10-23T08:27:49Z

The commit with the native simulator changes is not anymore in this branch, and it got in main together with other native simulator updates. The branch now is just rebased on the latest main.
@nordic-krch your review would be very welcome :)

jpwright requested review from nordic-krch, aescolar and daor-oti as code owners October 13, 2023 02:56

zephyrbot added area: native port Host native arch port (native_sim) area: Counter labels Oct 13, 2023

zephyrbot assigned aescolar Oct 13, 2023

jpwright force-pushed the native-posix-counter-top branch from 5d01a08 to 4a9c003 Compare October 13, 2023 03:02

aescolar added this to the v3.6.0 milestone Oct 13, 2023

aescolar requested changes Oct 13, 2023

View reviewed changes

jpwright force-pushed the native-posix-counter-top branch 10 times, most recently from 0d87024 to 56683cc Compare October 15, 2023 02:20

jpwright requested a review from aescolar October 15, 2023 03:11

nordic-krch reviewed Oct 16, 2023

View reviewed changes

jpwright force-pushed the native-posix-counter-top branch 2 times, most recently from f0c190b to cfc167e Compare October 17, 2023 05:39

aescolar previously requested changes Oct 17, 2023

View reviewed changes

jpwright force-pushed the native-posix-counter-top branch 4 times, most recently from 8d9c3ee to 1cd4adf Compare October 18, 2023 03:27

aescolar reviewed Oct 18, 2023

View reviewed changes

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

jpwright force-pushed the native-posix-counter-top branch from 1cd4adf to 7a666e0 Compare October 19, 2023 02:58

jpwright commented Oct 19, 2023

View reviewed changes

aescolar added the DNM This PR should not be merged (Do Not Merge) label Oct 19, 2023

aescolar previously approved these changes Oct 19, 2023

View reviewed changes

aescolar dismissed their stale review via 2a7db9d October 19, 2023 08:41

aescolar force-pushed the native-posix-counter-top branch from 7a666e0 to 2a7db9d Compare October 19, 2023 08:41

zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Oct 19, 2023

aescolar approved these changes Oct 19, 2023

View reviewed changes

aescolar force-pushed the native-posix-counter-top branch from 2a7db9d to 39fd121 Compare October 23, 2023 08:26

nordic-krch approved these changes Oct 23, 2023

View reviewed changes

aescolar merged commit 7e02a03 into zephyrproject-rtos:main Oct 23, 2023
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: counter: add support for top value configuration on native_posix #63897

drivers: counter: add support for top value configuration on native_posix #63897

jpwright commented Oct 13, 2023

aescolar left a comment •

edited

Loading

nordic-krch Oct 16, 2023

jpwright Oct 17, 2023

aescolar left a comment

aescolar Oct 17, 2023

aescolar commented Oct 17, 2023 •

edited

Loading

jpwright left a comment

jpwright Oct 19, 2023

aescolar Oct 19, 2023

jpwright Oct 19, 2023

aescolar Oct 19, 2023

aescolar commented Oct 19, 2023

aescolar left a comment •

edited

Loading

jpwright commented Oct 21, 2023

aescolar commented Oct 23, 2023

drivers: counter: add support for top value configuration on native_posix #63897

drivers: counter: add support for top value configuration on native_posix #63897

Conversation

jpwright commented Oct 13, 2023

aescolar left a comment • edited Loading

Choose a reason for hiding this comment

nordic-krch Oct 16, 2023

Choose a reason for hiding this comment

jpwright Oct 17, 2023

Choose a reason for hiding this comment

aescolar left a comment

Choose a reason for hiding this comment

aescolar Oct 17, 2023

Choose a reason for hiding this comment

aescolar commented Oct 17, 2023 • edited Loading

jpwright left a comment

Choose a reason for hiding this comment

jpwright Oct 19, 2023

Choose a reason for hiding this comment

aescolar Oct 19, 2023

Choose a reason for hiding this comment

jpwright Oct 19, 2023

Choose a reason for hiding this comment

aescolar Oct 19, 2023

Choose a reason for hiding this comment

aescolar commented Oct 19, 2023

aescolar left a comment • edited Loading

Choose a reason for hiding this comment

jpwright commented Oct 21, 2023

aescolar commented Oct 23, 2023

aescolar left a comment •

edited

Loading

aescolar commented Oct 17, 2023 •

edited

Loading

aescolar left a comment •

edited

Loading