ICU, use both channels?

[Giovanni]

It is correct, the interrupt should be disabled if the callback is null, but a null callback could explain your crash. Try putting breakpoints in order to make sure.

About the high level API:
1) Putting more requirements on the driver (extra modes) would require more complex implementations. The driver is already fairly complex.
2) The existing ICU implementations would require similar extensions, assuming that the timers support those modes.
3) The branch 2.6.x will not receive huge modifications, any extension will have to wait for 3.0 anyway.

Giovanni

[Korken]

Thanks you for the tip. I'm currently trying to use ChibiStudio (discussed in another thread) so hopefully I can track it down.
Because I double checked the initialization of all the registers, and it is the same as in the ST driver so it is not a setup problem.

About the high level API:
1) Agreed it is complex, but complex functionality require that.
2) When it comes to support on other platforms, I have been thinking on that. The two defined "channels", eicupwmchannel_t and eicuchannel_t could be used in conjunction to present information about the hardware.
For example eicuchannel_t tells how many channels are available and eicupwmchannel_t tells how many PWM channels is supported. Perhaps this could be used?
3) Understood! But tell me if you are interested to use the expanded code, I'd be glad to help you figuring out the implementation as long as I know the problems.

[Korken]

Okey! I have catched the error! But the thing is, it should not happen! See the timer registers:

If you look at the DIER register, only the CC1IE bit is set, but in the SR register (After an overflow) the UIF bit is set and the CC2IF bit is set.
Why in Cthulhu's name are the interrupts firing when they are disabled?

Any ideas?

[Giovanni]

Nope, you have to step into the initialization code I think.

Giovanni

[Korken]

Okey! I found out what is happening!
The thing is when the counter overflows the interrupt bits are always set because they match the compare values of the disabled channels (CCR = 0, even though they are disabled).
They won't generate any interrupt but if you are unlucky and the channel you are using fires before these are cleared you get this problem.

The fix is simple: use the interrupt enable register as a bit mask, changing:

Code: Select all

static void eicu_lld_serve_interrupt(EICUDriver *eicup)
{
  uint16_t sr;
  sr = eicup->tim->SR;

  /* Clear interrupts */
  eicup->tim->SR = ~sr;

  ...

to:

Code: Select all

static void eicu_lld_serve_interrupt(EICUDriver *eicup)
{
  uint16_t sr;
  sr = eicup->tim->SR;

  /* Clear interrupts */
  eicup->tim->SR = ~sr;

  /* Pick out the interrupts we are interested in by using
     the interrupt enable bits as mask */
  sr &= (eicup->tim->DIER & 0x007F);

  ...

And then it works as expected!

[Giovanni]

Hi,

Back to this topic. I am thinking to keep the ICU driver only handle PWM in 3.0. The new EICU driver would implement other capture modes (TBD, except PWM).

The various timers could be assigned to PWM, ICU or EICU.

My problem is that the current ICU for STM32 is already very complex as-is because all the overhead caused by the handling of the two inputs, the risk is to make it unmanageable.

Giovanni

[Korken]

Okey! The code will be on the link provided if you decide to use it!

However, which overhead are you referring to? Perhaps I can redesign something to make it more streamlined.
Because now it is a direct ISR implementation base of macros, there is practically no overhead in execution time.
Worst case scenario is if it is executing in input edge mode for channel 4, then there are six if-statements to overcome. Is this the problem section?

[Giovanni]

Hi,

It is not about a runtime overhead but increased driver complexity. The modified driver would also enforce me to update implementations for other families (SPC5, AT, ecc) and that would be a problem, some of those families are not even handled by me.

This is why I prefer to introduce the new functionalities as a different new driver, this would limit the impact on a lot of code.

Giovanni

[Korken]

Ah, now I understand!

Can you not use it as I have named it as a new driver: "Extended ICU"?
Then people can implement it at their own pace for the other families.

Worst case I can put it in contributions.

[Giovanni]

Note that a driver does not need to be placed in the HAL necessarily, so making an add-on is a perfectly valid approach.

Giovanni