-
Notifications
You must be signed in to change notification settings - Fork 8.1k
drivers: counter: introduce counter capture api #89127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
f3dec28
to
8b678d7
Compare
How is this functionally different from what is offered by the PWM capture API? |
That PWM capture measures the period and duty cycle of a pulse. This counter capture captures the exact time of a rising and/or falling edge when it happens. |
@XenuIsWatching when would it be a good time for you to present this in the Architecture WG meeting on Tuesdays? |
Hey @carlescufi , sorry for getting back to a bit late, I do plan on getting back to this, but right now I"m dealing with other priorities. I'll ping you went I can get around to making some slides |
uint32_t flags); | ||
typedef uint32_t (*counter_api_get_freq)(const struct device *dev); | ||
typedef uint64_t (*counter_api_get_freq_64)(const struct device *dev); | ||
typedef int (*counter_api_capture_callback_set)(const struct device *dev, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if I understand correctly but this API assumes that:
- it captures pin event (what about other possible capture sources?)
- there is a known pin associated with the channel (is it setup in DT or fixed in the peripheral?)
At least for Nordic peripheral capture can be used with any pin and any event in the system (e.g. end of SPI transfer, start of UART RX, etc.). It might be too generic to have "any event" approach so it can be narrowed down to pin only but then it would be good to include pin configuration.
Maybe similar to struct counter_alarm_cfg
:
struct counter_captrure_cfg {
gpio_dt_spec gpio;
uint32_t flags; //may contain flags like one_shot vs all pin events
counter_capture_cb_t cb;
void *user_data;
}
int counter_set_channel_capture(const struct device *dev, uint8_t chan_id,
const struct counter_capture_cfg *cfg);
int counter_cancel_channel_capture(const struct device *dev, uint8_t chan_id);
uint32_t counter_get_channel_capture(const struct device *dev, uint8_t chan_id);
uint64_t counter_get_channel_capture_64(const struct device *dev, uint8_t chan_id);
Callback can be null as there would be also an option to read the captured value using get_capture()
(it is not always needed to get the interrupt on capture).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this makes much sense, there should be no need to have a gpio_dt_spec
within there. A lot of these "input capture" or "event capture" (depending on which term the implementer uses for their SoC) implementations make NO USE of the GPIO peripheral and just have an internal wire to a mux which can go directly to the counter peripheral to the 'pad' which is used as the event signal to latch a timestamp. For at least the STM32 implementation here, just the pinctrl is used to handle the pad muxing. Example of the dts overlay is showing below.
#include <st/h5/stm32h503rbtx-pinctrl.dtsi>
&timers2 {
st,prescaler = <239>;
status = "okay";
capture: counter {
status = "okay";
pinctrl-0 = <&tim2_ch1_pa0 &tim2_ch2_pa1>;
pinctrl-names = "default";
};
};
I'm not super familiar with Nordic, but perhaps that driver implementation could have that gpio defined in the dts to be with it's dts (and may need to have some sort of PPI defined using that driver @carlescufi mentioned)
for the events, most implementations i've seen at least for the Atmel's SAM and STM32 line, the only useful event that can latch a time on is off a pad (which is typically connect to a sensor (or whatever)). I suppose, peripherals an SoC could provide a Header with some SoC specific event flags on which 'event' (be it UART, SPI, GPIO, etc) they want to have that counter event on.. but that make already make use of that uint32_t flags
with in there. (already only 2 bits are already used)
For being similiar to struct counter_alarm_cfg
, I was aiming to be similar to the apis implemented with CONFIG_PWM_CAPTURE
.
Architecture WG meeting:
|
I'm not sure that is totally blocking this implementation, likely that would only need to be implemented for the Nordic implementation, and if there are any needs of gaps found in this api I'm sure as this could be 'experimental' I'm sure those changes (if any) could come when that comes, but I'm happy to mostly certainly consider those now, I'm just not sure what those would need to be.
Thinking more about this, I'm not sure where this fits in, as the pinctrl is used in this implementation to select the mux on the "pad" to be routed to the counter.
@henrikbrixandersen Sounds great!!! We can discuss your comments and proposes over Discord if thats best. On other note 🎵 , I'm thinking about splitting this PR, there appears to be a lot more discussion on the 'input capture' part of this... and the other changes I had to make such as changing some |
8b678d7
to
2d6b777
Compare
|
The point is related to the fact that other people brought up this recurring topic about on chip triggers. There intention was to suggest expanding the scope of this implementation to consider more than just external ios as capture triggers. |
We would be interested in such an generic interface too. We use the same functionality on different chipsets. (NXP, Nordic, STM, ..) and had to make chip specific low level (work-around) drivers for them. Loosing the ease of use of the DTS for instance as the counter drivers do not support pinctrl. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments
To get this going, we could agree to a similar approach to what we do for comparators, let vendors define the capture inputs in whatever way makes sense using the devicetree, where its all vendor specific and flexible, and add vendor specific APIs for runtime configuration of mux or other hardware options to be used optionally. It seems the API itself is portable, store tick at event, as long as the chan is the counter, not some vendor specific input index, so if its just the event routing, I think the static approach may be sufficient for many cases For reference https://docs.zephyrproject.org/latest/hardware/peripherals/comparator.html#configuration |
include/zephyr/drivers/counter.h
Outdated
|
||
/**@} */ | ||
|
||
typedef void (*counter_capture_cb_t)(const struct device *dev, uint8_t chan, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other APIs use chan_id
for the channel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done... renamed
2d6b777
to
24aed08
Compare
24aed08
to
8947583
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the commit message, would you mind replacing apis
with APIs
.
}; | ||
#endif | ||
#ifdef CONFIG_COUNTER_CAPTURE | ||
static uint32_t (*const get_timer_capture[TIMER_MAX_CH])(const TIM_TypeDef *) = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** Channel to timestamp capture function mapping. */
Could you add an empty line before #ifdef ...
above and after #endif
below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
const struct counter_stm32_config *config = dev->config; | ||
struct counter_stm32_ch_data *chdata = &config->ch_data[chan]; | ||
TIM_TypeDef *timer = config->timer; | ||
uint32_t config_flags = LL_TIM_ACTIVEINPUT_DIRECTTI; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be worth the call disable_it[chan](timer);
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it shouldn't be disabling the interrupt in the case where it's being used as a counter capture, the alarm is a one-shot alarm just disables it in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of preventing the interrupt to trigger in between chdata->capture
and chdata->user_data
update right below.
|
||
/**@} */ | ||
|
||
typedef void (*counter_capture_cb_t)(const struct device *dev, uint8_t chan_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion
/** @brief Counter capture callback
*
* @param dev Pointer to the device structure for the driver instance
* @param chan_id Channel ID
* @param flags Configuration flags (COUNTER_CAPTURE_*)
* @param ticks Counter value that triggered the capture
* @param user_data User data
*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI compliance check taints on the space char missing before *
characters above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
include/zephyr/drivers/counter.h
Outdated
const struct counter_driver_api *api = | ||
(struct counter_driver_api *)dev->api; | ||
|
||
if (!api->capture_callback_set) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!api->capture_callback_set) { | |
if (api->capture_callback_set == NULL) { |
Ditto for z_impl_counter_capture_enable()
and z_impl_counter_capture_disable()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the other way, it's also more defensive (misra) so you can avoid doing the single =
, it's also consistent with the rest of the file doing it the way it's done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the whole file should be updated. Zephyr coding guideline explicitly requires comparison against NULL here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
8947583
to
b6988b5
Compare
const struct counter_stm32_config *config = dev->config; | ||
struct counter_stm32_ch_data *chdata = &config->ch_data[chan]; | ||
TIM_TypeDef *timer = config->timer; | ||
uint32_t config_flags = LL_TIM_ACTIVEINPUT_DIRECTTI; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of preventing the interrupt to trigger in between chdata->capture
and chdata->user_data
update right below.
|
||
/**@} */ | ||
|
||
typedef void (*counter_capture_cb_t)(const struct device *dev, uint8_t chan_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI compliance check taints on the space char missing before *
characters above.
include/zephyr/drivers/counter.h
Outdated
const struct counter_driver_api *api = | ||
(struct counter_driver_api *)dev->api; | ||
|
||
if (!api->capture_callback_set) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the whole file should be updated. Zephyr coding guideline explicitly requires comparison against NULL here.
include/zephyr/drivers/counter.h
Outdated
* | ||
* @param dev Pointer to the device structure for the driver instance | ||
* @param chan_id Channel ID | ||
* @param flags Configuration flags (COUNTER_CAPTURE_*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is an anchor for flags:
* @param flags Configuration flags (COUNTER_CAPTURE_*) | |
* @param flags Configuration flags (@ref COUNTER_CAPTURE_FLAGS) |
Ditto for counter_capture_callback_set()
inline description comment.
This introduces APIs for input capture on the STM32. This is something agnostic enough that is seen on other vendors such as Atmel's SAM line and others. A use case for this would be time stamping a rising and/or falling edge where precision and lack of jitter is important. The only way to currently do this in zephyr is to set up a GPIO interrupt and then read the counter or kernel time... but this can have limitations of software such as the processor may have a lot going on and can be subject to interrupt latencies. Having a hardware 'latch' done of the rising and/or falling edge with respect to a timer can eliminate these imprecisions. How this is to be used, is that an application is to call `capture_callback_set` with the configuration of the channel number, flags (such as falling, rising, or both edges), and then a callback function. Then the `capture_enable` function is to be called to enable this. The callback has the timestamp of the edge in ticks and the edge that it captured (rising or falling). Signed-off-by: Ryan McClelland <[email protected]>
b6988b5
to
fe57a67
Compare
|
This introduces apis for input capture on the STM32. This is something
agnostic enough that is seen on other vendors such as Atmel's SAM line
and others.
A use case for this would be time stamping a rising and/or falling edge
where precision and lack of jitter is important. The only way to currently
do this in zephyr is to set up a GPIO interrupt and then read the counter
or kernel time... but this can have limitations of software such as the
processor may have a lot going on and can be subject to interrupt latencies.
Having a hardware 'latch' done of the rising and/or falling edge with
respect to a timer can eliminate these imprecisions.
How this is to be used, is that an application is to call
capture_callback_set
with the configuration of the channel number, flags(such as falling, rising, or both edges), and then a callback function. Then
the
capture_enable
function is to be called to enable this. The callbackhas the timestamp of the edge in ticks and the edge that it captured (rising
or falling).
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 whichwill still return a 32bit value. This also adds a
counter_get_freq_64
function to get a 64bit value for the frequency.
Add inline helper functions for converting ticks to nanoseconds.Also implement an inline helper for converting microseconds ornanoseconds with a 64bit tick value.
split in to #94189