Counter Init() parameters, cycles() and IRQ()

[NaveenChengappa-IFX]

Summary

• Extended the Counter module implementation Counter module Basics #150.
• Added more Counter.Init() parameters to the scope :
  • src - source pin with PERI_TR_IO_INPUT routing.
  • edge - RISING/FALLING edge detection.
  • direction - UP/DOWN count direction.
  • max/min - value range (supports negative values as well).
  • index - optional index pin for cycle increment.
  • reset - optional reset pin to restart counting.
  • match - optional compare value for IRQ trigger (supports negative values as well).
• Implemented Counter.cycles()
  • Automatic increment/decrement on ROLL_OVER/ROLL_UNDER events.
• Implemented Counter.irq()
  • Trigger flags: IRQ_RESET, IRQ_INDEX, IRQ_MATCH, IRQ_ROLL_OVER, IRQ_ROLL_UNDER.
  • Hard IRQ requests supported.

Testing

HIL Testing carried out to test all features.

Trade-offs and Alternatives

• In init(), the following have not been implemented :
  • filter_ns - Requires CY_IP_MXTCPWM_VERSION needs to be >3
  • match_pin - Requires PERI_TR_IO_OUTPUT to be configured, which adds complexity
• Tested only on pin P11_1

Generative AI

I did not use generative AI tools when creating this PR.

[IFX-Anusha]

A lot of work here 🔥 — the overall structure is good. I have tried my best to review the code (with some AI assistance (of course) ) and found a few things.

[jaenrig-ifx]

Can we use any tcpwm instance? or it has to be the one associated to that the used pin?

[NaveenChengappa-IFX]

We can use any tcpwm instance from 0-31 as mentioned.
The pin associated to the Counter is only for the input trigger (denoted by the src arg in Init()). This is pin P11_1 in our PSOC-Edge board. It is not a TCPWM capable pin.

[jaenrig-ifx]

Do we actually want to expose that TCPWM information to the user?
I guess it would be only valuable if he has to decide 16 or 32 resolution. But that can be also be resolved based on the max/min.
I would say this id should match more the unit peri input channel. So that you could actually avoid passing the Pin value.

And this is always the problem with the id... deciding what is represents.
In the SCB classes it matches the SCBx index. But here we even have PERI0 a PERI1 ... So actually I am not fully sure.

[NaveenChengappa-IFX]

The ID information is shared with the user to make them aware of the 16-bit or 32-bit counter.
The pin input is a requirement from the API as the trigger input only, which in our port's case is only P11_1.

ID has different meanings in different classes. So its better to document it and let the user know and decide what to use.

[jaenrig-ifx]

That is exactly my question, the 'id' meaning for this class:
a) 'id' = counter input channel
b) 'id' = tcpwm associated counter

I don't see the b.
That can be resolved based on the min, max values. And 'id' = channel = hardware pin.

So you can do:

c = Counter(1) 
c = Counter(Pin.cpu.P11_1)
c = Counter(1, Pin.cpu.P11_1)

and these are all the same.
Assuming that P11_1 is in channel 1.

[NaveenChengappa-IFX]

In the Counter class :
a) 'id' = tcpwm associated counter
b) 'src' = counter input channel

Both parameters are very different. We can have one src (P11_1) and multiple ids.
So this becomes a necessity that we support both :
c = Counter(1, Pin.cpu.P11_1)
OR
we keep src as a default P11_1 and this works :
c = Counter(1)

[jaenrig-ifx]

I guess I am not expressing myself very well...

The alternate function PERIx_TR_IO_INPUTx has up to 8 possible inputs (even if for this board only one is available).

I would say this should be the id instead of the tcpwm counter.
Given how other ports use this id, and how we have implemented that in the UART.

The user does not need to know to which tcpwm counter we are associating the counter.
That can be automatically derived from the the min, max value, based on required resolution.

Then, the user can create a counter either based on the pin, or the PERIx_TR_IO_INPUTx.
He could pass both also, then they need to match.

[jaenrig-ifx]

I know right now we have only one baord, but we should not add board specific information in this section to keep it generic.
We should show which pins are counter capable in the pinout diagram. Check with @ederjc about how to add the counter info.

[jaenrig-ifx]

Please do not use the MTB HAL. This is uncertain to be supported in the future.
The port implementation should be based on PDL only.
Once the clk.h class is merged (soon), we should handle the clock from there.

[jaenrig-ifx]

Is there a situation in which we could use a counter object which has not been initialized?
I would say once we do obj.value([]) that should not the case.

[NaveenChengappa-IFX]

The user can do something like this :
Start off with a bare minimum constructor like c = Counter(0) , and then without initializing it with c.init() , if there is a request for c.value() then this error is thrown.
It is acting like an information to the User stating they have forgotten to c.init() before requesting for the value.
And I dont see any usecase for a bare constructor without init().

[jaenrig-ifx]

As far I understand, for example for UART, if you do UART(0), this would already enable the UART for 115200, 8-N-1 and return a usable object.
I would say that c = Counter(0) should be initialized to certain values with default and/or mandatory args?
Is there any other machine.xxx() with this behavior?

[NaveenChengappa-IFX]

It will work. The only missing mandatory parameter (without a default value) is the src. This is what throws the exception of not initialised when we ask for Counter.value().
So if we can have pin P11_1 set as default src , then this error message will not come up.

We can still do it in our case because we have only P11_1 that can be set as src .

[jaenrig-ifx]

We have to think further than this board, we enabling the MCU.
So potentially with some future board could also use these pins. P7_0, P7_1, P7_5 - P8_0, P11_0 - P11_3.
We should not set any as default.

The src parameter is set already as MP_ARG_REQUIRED -> https://github.com/Infineon/micropython-psoc-edge/blob/Counter_cyclic_and_irq/ports/psoc-edge/machine_counter.c#L518

But machine_counter_init_helper() is only called if there is more than 1 positional argument or more than 0 keyword arguments:
https://github.com/Infineon/micropython-psoc-edge/blob/Counter_cyclic_and_irq/ports/psoc-edge/machine_counter.c#L810-L814

if (n_args > 1 || n_kw > 0) {
    mp_map_t kw_map;
    mp_map_init_fixed_table(&kw_map, n_kw, args + n_args);
    machine_counter_init_helper(self, n_args - 1, args + 1, &kw_map);
}

If that condition is removed. Just:

  mp_map_t kw_map;
  mp_map_init_fixed_table(&kw_map, n_kw, args + n_args);
  machine_counter_init_helper(self, n_args - 1, args + 1, &kw_map);

Then if no src is passed, it will throw exception:

>>> from machine import Counter
>>> c = Counter(0)
Traceback (most recent call last):                                                                                                              
  File "<stdin>", line 1, in <module>                                                                                                           
TypeError: 'src' argument required                                                                                                                                                                                                                           
>>> from machine import Pin                                                                                                                     
>>> c = Counter(0, Pin.cpu.P11_1)  

Then that initialization check in value() would not be required.
Unless that behavior of uninitialized instance Counter(id) object is somehow required. I would say that calling the constructor should also initialize the object.

[jaenrig-ifx]

To be refactored based on shared clock. Maybe via tcpwm class when we refactor it.

[jaenrig-ifx]

Is this required? I expect you should be able to read the counter while active without any race conditions or such.
We can miss some edges here or?

[jaenrig-ifx]

Okay, if we are in the setter, that would make sense, but shouldn't this be moved to the second n_args ==2 section, and guard with if n_args == 1, the getter part?

[jaenrig-ifx]

Or, value() is always a getter, and optionally a settter (if an arg is passed)?

[IFX-Anusha]

This is for the reset condition, right?
value(x) read+reset happens, and therefore edges are missed in reset boundary?

[NaveenChengappa-IFX]

There is a requirement from the micropython API that the setter function should also return the current counter value.
In this case here, the flow is as follows:

To Set: counter.value()

• Disable the counter
• Read the counter value
• Process the value
• Reset the Counter
• Clear IRQs
• Enable the Counter
• Return the read counter value

To Get: counter .value()

• Read the counter value
• Process the value
• Return the read counter value

[jaenrig-ifx]

I get it, it was a bit confusing that setter() also should get.
Still, does the counter needs to be disabled and renabled() ? Not sufficient to set the new counter value after reading (all that atomically)?

[NaveenChengappa-IFX]

There is a risk for race condition at higher frequency of the input trigger pulses. Safer to disable, read and enable.
We have a atomic lock only for the read Cy_TCPWM_Counter_GetCounter().

[IFX-Anusha]

🚀

[jaenrig-ifx]

Very good job!
I leave up to you, if you want to rework some of these different behavior for this PR. Otherwise, you can can create tickets for future enablement/consideration. 👍