Skip to content

Conversation

XenuIsWatching
Copy link
Member

@XenuIsWatching XenuIsWatching commented Aug 6, 2025

This also introduces an api for set the counter time.

Also, this set frequency to be 64 bits rather than 32 bits, as counters,
such as those that use fractional adders, tick at resolutions
greater than 4294967295Hz. This changes the freq in the common info
to a uint64_t. This retains the existing counter_get_freq function which
will still return a 32bit value. This also adds a counter_get_freq_64
function to get a 64bit value for the frequency. Unfortunately, I do not
have a 'public' driver that uses this.

Add inline helper functions for converting ticks to nanoseconds.

Also implement an inline helper for converting microseconds or
nanoseconds with a 64bit tick value.

this was a split from #89127

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think commit drivers: counter: stm32: introduce counter set value api should be split in 2 commits: one for the generic part (counter.h) and one for the STM32 driver. That is not a strong opinion, maybe maintainer is fine as-is.

@XenuIsWatching
Copy link
Member Author

@etienne-lms looking through your comments on formatting, i see the same issues ALL throughout this counter.h, I'll just add a commit and the head which "runs clang-format" on counter.h

etienne-lms
etienne-lms previously approved these changes Aug 20, 2025
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For commit "drivers: counter: run clang-format on counter.h" I leave it to maintainer to tell if the commit is worth it.
LGTM.

@XenuIsWatching
Copy link
Member Author

something I just realized is that the alarm callbacks will need a 64b tick value (as well as likely the top values)

@XenuIsWatching
Copy link
Member Author

XenuIsWatching commented Aug 20, 2025

something I just realized is that the alarm callbacks will need a 64b tick value (as well as likely the top values)

thinking even more bout this... it gets rather complicated for counter drivers which do not use a 64b tick register and to support 64b across the board (top value, alarm ticks, values, max top values)... I'm beginning to think maybe it would be better to just have a KConfig (maybe CONFIG_COUNTER_64BITS) that selects a typedef for either a uint32_t or a uint64_t such as counter_ticks_t which is then used throughout the driver, rather than having to worry about if you need to use the 32b api or the 64b api and having 32b or 64b in some alarm callbacks (and others)

I also can't see a reason to really still have the 32b apis around if you know you've got a 64b counter

such as

typedef int (*counter_api_set_value)(const struct device *dev, uint32_t ticks);

would just simply become

typedef int (*counter_api_set_value)(const struct device *dev, counter_ticks_t ticks);

as well as

typedef void (*counter_alarm_callback_t)(const struct device *dev, uint8_t chan_id, uint32_t ticks,
					 void *user_data);

would become

typedef void (*counter_alarm_callback_t)(const struct device *dev, uint8_t chan_id, counter_ticks_t ticks,
					 void *user_data);

....and the other places where ticks, top values are defined...

@jhedberg
Copy link
Member

@XenuIsWatching there are some merge conflicts which require a rebase

Add a reset counter api to set the time back to 0.

Signed-off-by: Ryan McClelland <[email protected]>
Introduce a counter set value api to set the ticks for 32b and 64b.

Signed-off-by: Ryan McClelland <[email protected]>
Implement the counter_set_value api for the STM32.

Signed-off-by: Ryan McClelland <[email protected]>
Some counters, such as those that use fractional adders, tick at
resolutions greater than 4294967295Hz. This changes the freq in the common
info to a uint64_t. This retains the existing `counter_get_freq` function
which will still return a 32bit value. This also adds a
`counter_get_freq_64` function to get a 64bit value for the frequency.

Signed-off-by: Ryan McClelland <[email protected]>
Add inline helper functions for converting ticks to nanoseconds.

Also implement an inline helper for converting microseconds or
nanoseconds with a 64bit tick value.

Signed-off-by: Ryan McClelland <[email protected]>
Run clang-format on counter.h

Signed-off-by: Ryan McClelland <[email protected]>
@kartben
Copy link
Contributor

kartben commented Oct 17, 2025

[...] it just seems odd and rather useless in first place as having something 64b ticks supported and then just returning 32b ticks within alarm and top values when they really can be 64b really seems odd.

I don't necessarily disagree that the API initially introduced was probably poorly designed, but that doesn't make the change "less" breaking :/ (just similar to if we rename an API that has a typo in it... this does fix the oddity, but breaks folks).

@zephyrbot zephyrbot requested a review from nordic-krch October 17, 2025 15:15
@XenuIsWatching
Copy link
Member Author

@XenuIsWatching there are some merge conflicts which require a rebase

[...] it just seems odd and rather useless in first place as having something 64b ticks supported and then just returning 32b ticks within alarm and top values when they really can be 64b really seems odd.

I don't necessarily disagree that the API initially introduced was probably poorly designed, but that doesn't make the change "less" breaking :/ (just similar to if we rename an API that has a typo in it... this does fix the oddity, but breaks folks).

I understand that, but it should never be too late to do it right :)

@XenuIsWatching
Copy link
Member Author

@XenuIsWatching there are some merge conflicts which require a rebase

done

@XenuIsWatching XenuIsWatching force-pushed the counter-set-64 branch 3 times, most recently from 2f664f7 to 883cead Compare October 17, 2025 15:32
Use a new KConfig, `COUNTER_64BITS`, to define a typedef to be used
(or if driver or application always expects it to be 32b or 64b, it
can use `uint32_t` or `uint64_t` straight) counter_ticks_t which will
define the tick values used to be 64b or 32b.

This removes the `get_value_64` api in favor of just having a
universal `get_value` that is set with the kconfig to return that
typedef which is to be 32b or 64b depending on if the driver selects
the kconfig. Existing drivers are updated removing the 32b in favor
of it's 64b implementation, or just have their get_value_64 api
renamed to the get_value api along with them selecting COUNTER_64BITS.

Signed-off-by: Ryan McClelland <[email protected]>
Add an entry to the migration guide for 4.3 for counter 64b ticks.

Signed-off-by: Ryan McClelland <[email protected]>
@sylvioalves
Copy link
Contributor

@kartben based on your comments, should this be tagged with "breaking API changes" and also added into dev review or TSC meeting? Asking this due to upcoming feature freeze.

Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Counter area: Sensors Sensors area: Tests Issues related to a particular existing or missing test area: Timer Timer manifest manifest-sof platform: ESP32 Espressif ESP32 platform: ITE ITE platform: NXP Drivers NXP Semiconductors, drivers platform: NXP NXP platform: STM32 ST Micro STM32 platform: Xilinx AMD Xilinx

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants