ICU Overflow callback used for scaling

Report here problems in any of ChibiOS components. This forum is NOT for support.
User avatar
FXCoder
Posts: 384
Joined: Sun Jun 12, 2016 4:10 am
Location: Sydney, Australia
Has thanked: 180 times
Been thanked: 130 times

ICU Overflow callback used for scaling

Postby FXCoder » Tue Oct 23, 2018 6:21 am

When an ICU overflow callback is set and enabled the state is changed to ICU_WAITING by the driver after the callback to the user function.
In the case where ICU overflow is used to accumulate timer wrap around (count periods in excess of the timer using the callbacks) then the change of state by the overflow handler prevents the period callback from ever being executed as follows...

The timer CC interrupt is handled but as the state is ICU_WAITING the ICU is still in synchronisation mode and simply changes state to ICU_ACTIVE.
The next overflow (since we are measuring a period beyond the timer capacity) will again set the state to ICU_WAITING.
Thus the ICU never reports anything other than OVERFLOW.

The ICU overflow handler should leave it to the user callback to determine what to do on overflow.
BTW in the case where ICU does not use callbacks then ICU state is not changed by the driver when an overflow occurs.

FYI the use case in the above is a GPS disciplined oscillator where the APB frequency (48MHz in this case) is counted using a 16 bit TIM ICU triggered by GPS TIMEPULSE all wrapped in a state machine.
The state machine manages start of accumulation (after one ICU period has been seen) and then accumulates each ICU overflow into a total count of clocking frequency + final amount taken at closing ICU period callback.

Code: Select all

Index: hal_icu.h
===================================================================
--- hal_icu.h   (revision 12389)
+++ hal_icu.h   (working copy)
@@ -200,8 +200,8 @@
 
 /**
  * @brief   Common ISR code, ICU timer overflow event.
- * @note    An overflow always brings the driver back to the @p ICU_WAITING
- *          state.
+ * @note    An overflow leaves the state unchanged so that the callback can
+ *          determine what action to take.
  *
  * @param[in] icup      pointer to the @p ICUDriver object
  *
@@ -209,7 +209,6 @@
  */
 #define _icu_isr_invoke_overflow_cb(icup) do {                              \
   (icup)->config->overflow_cb(icup);                                        \
-  (icup)->state = ICU_WAITING;                                              \
 } while (0)
 /** @} */
 

User avatar
Giovanni
Site Admin
Posts: 14444
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 1074 times
Been thanked: 921 times
Contact:

Re: ICU Overflow callback used for scaling

Postby Giovanni » Thu Nov 15, 2018 10:58 am

Hi,

I am not sure about this change, what about putting the state change before calling the callback? that would allow you to change it on the fly without affecting others.

Giovanni

User avatar
FXCoder
Posts: 384
Joined: Sun Jun 12, 2016 4:10 am
Location: Sydney, Australia
Has thanked: 180 times
Been thanked: 130 times

Re: ICU Overflow callback used for scaling

Postby FXCoder » Thu Nov 15, 2018 11:30 am

Hi,
I understand the concern about other ICU users expecting the current behavior.
As noted if callbacks are not used the behavior of ICU is to not re-sync at overflow.
So that inconsistency may or may not be of concern.

Two options:

1. As you suggest put the state change before callback.
Also add a new macro to ICU to restore the state during the callback.
This works although it is a bit non-obvious.

2. Add a #define to mcuconf.h to modify the behavior of _icu_isr_invoke_overflow_cb(icup).
This would be obvious albeit less flexible as it would affect all ICUs in use.

I guess #1 is the preferable option given it is configurable per ICU.

--
Bob

User avatar
FXCoder
Posts: 384
Joined: Sun Jun 12, 2016 4:10 am
Location: Sydney, Australia
Has thanked: 180 times
Been thanked: 130 times

Re: ICU Overflow callback used for scaling

Postby FXCoder » Tue Nov 20, 2018 7:01 am

Hi Giovanni,
Before doing anything on a revised patch I walked through the ICU scenario where:
- ICU uses a 16 bit TIM
- Callback is specified for ICU Period and/or Width
- Callback is not specified for ICU overflow

With the above scenario the current ICU implementation will return erroneous period and width values from icuGetWidthX(...) and or icuGetPeriod(...) when a TIM overflow(s) has occurred.
i.e. PWM input period/width exceeds the TIM counter limit.
TIM wraps around and since an overflow CB is not set the overflow is not acted upon.
At the next CC edge the width/period CB is called.
Reading the TIM CC register at that time gives only the residual (erroneous) TIM CNT accumulated after the most recent overflow.

Of course as per the original bug report, if an overflow CB is set then only the overflow CB is called. Period and width CBs are silent.

So I'm thinking that:
1. overflow scaling for 16 bit TIMs should be implemented inside ICU (optionally enabled by a halconf setting).
2. That ICU should be revised so even if scaling is not enabled, it won't report erroneous counts where overflow CB isn't provided.

Thoughts?

--
Bob

User avatar
Giovanni
Site Admin
Posts: 14444
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 1074 times
Been thanked: 921 times
Contact:

Re: ICU Overflow callback used for scaling

Postby Giovanni » Wed Nov 21, 2018 5:31 am

I need to analyze a possible workaround, keeping the overflow interrupt enabled is something I would like to avoid, there are a lot of ICU use cases where interrupts are not needed at all.

Giovanni

User avatar
FXCoder
Posts: 384
Joined: Sun Jun 12, 2016 4:10 am
Location: Sydney, Australia
Has thanked: 180 times
Been thanked: 130 times

Re: ICU Overflow callback used for scaling

Postby FXCoder » Wed Nov 21, 2018 1:28 pm

In polled mode where icu_lld_wait_edge(...) is used UIF is already checked and returns true for overflow.
If scaling were to be supported and enabled then not much of a change to the polled cases to support scaling through overflow.

Same in the interrupt based callback mode.
Not much of a change.
However, UIF interrupt would be enabled and handle accumulation of overflow count when ICU is active even if CB isn't set to be called.

Possible implementation for 16 bit TIMs...
Timer size is known from STM32_TIMx_IS_32BITS define flag.
Add a uint8_t to ICUDriver struct and set a bit in it in icu_lld_init(...) to reflect timer size for an ICU.
Then the common code knows readily what TIM size it is dealing with.
Then add 2 x accumulator pairs to ICUDriver struct to keep overflow count for width and period.
Another bit in the uint8_t indicates which accumulator pair is active for overflow accumulation.
Overflow accumulates to both until width edge when width accumulation is frozen and the respective CC value is added to it.
Another flag in uint8_t is set to indicate width has been captured.
When period edge happens add the respective CC value to the period accumulator.
The active accumulator pair bit is flipped and the new accumulator pair cleared on period edge.
Width and period reads during the new period will return values from the appropriate accumulators.

All of the overflow support could be optionally enabled by a halconf flag.
However, something like the above would fix the current erroneous count bug.
Of course accumulators could be made 64 bit to count really long PWM with 16 or 32 bit TIMs :D

User avatar
Giovanni
Site Admin
Posts: 14444
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 1074 times
Been thanked: 921 times
Contact:

Re: ICU Overflow callback used for scaling

Postby Giovanni » Sun Nov 10, 2019 10:35 am

bump

User avatar
FXCoder
Posts: 384
Joined: Sun Jun 12, 2016 4:10 am
Location: Sydney, Australia
Has thanked: 180 times
Been thanked: 130 times

Re: ICU Overflow callback used for scaling

Postby FXCoder » Wed Nov 13, 2019 11:29 am

On my TODO: list... not immediately though

Bob


Return to “Bug Reports”

Who is online

Users browsing this forum: No registered users and 16 guests