Skip to content
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

Proposal: Esp32 machine.PCNT through Encoder.py and Counter.py #8253

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

harbaum
Copy link

@harbaum harbaum commented Feb 2, 2022

This is an attempt to implement the Counter classes as described in #8072 in Python rather than C. It's thus meant as a replacement of #6639. It converts machine into a Python class similar to #8244

This is using a rather direct implementation of the ESP32 PCNT API in C with the Encoder and Counter classes derived from this.

One advantage is that the C part is way simpler. Also the full PCNT API is available allowing other type of counters (e.g. an up/down counter using dedicated pins for up and down) to be implemented besides Counter and Encoder using PCNT directly.

There are also downsides. In the current approach Counter and Encoder.py share quite some code and functionality so a common CounterBase class may be nice in-between PCNT and Counter/Encoder.

Also the interrupt handling in Python introduces a significant delay. This is a problem since the hardware counter automatically falls back to zero on overflow and it's up to the interrupt handler to adopt to these overflows correctly. This is less of a problem with C as there's only few delay between the counter being reset and the handler being called. In Python this time is longer and a Python program reading the counter value between the hardware counter rolling to 0 and the interrupt handler adjusting for that will read wrong counter values. One solution would be to implement the overflow interrupt in the PCNT.c.

@dpgeorge
Copy link
Member

dpgeorge commented Feb 2, 2022

Also the interrupt handling in Python introduces a significant delay. This is a problem since the hardware counter automatically falls back to zero on overflow and it's up to the interrupt handler to adopt to these overflows correctly

This needs to be improved. Three ways to address this are:

  1. try to implement hard IRQs for this case so the Python callback is called immediately in the ISR handler
  2. make schedule'd calls (via mp_sched_schedule) run with less latency
  3. implement the IRQ in C

I would probably suggest option 3, because it's always going to be beneficial to have as minimal latency as possible for this kind of class.

@harbaum
Copy link
Author

harbaum commented Feb 3, 2022

I would probably suggest option 3

Yes, but only partly. In my actual use case these counters are not simply be used as such. I do run motors which are connected to the counters and one use case is to stop the motors in the handler once they have run a given distance. So I still like to have the ability to run python handlers. So doing the counter handling purely in C is not a full solution.

Doing this IRQ in C will of course mean that the counter that is increased inside that IRQ handler is also on the C side and some control over the IRQ sources also has to be on the C side. I'd still like to keep this as simple as possible ...

I actually see a fourth solution which would be making the python side cope with these delays. I could e.g. try to check for pending interrupts when the user wants to read the counter value thus making sure that the handler fixing the counters is explicitly called in that case. But I am afraid that I cannot easily tell whether there's a handler call pending as the IRQ itself already has been serviced and it's the handler call that's pending. Anyway, I see that this needs work. That's why I did this as a proposal. But the original solution does not support my use cases at all ...

@harbaum
Copy link
Author

harbaum commented Jun 27, 2022

I would like to revoke this PR in favor of #7582 which basically does the same in a very same way but overall a little bit nicer ...

tannewt added a commit to tannewt/circuitpython that referenced this pull request Aug 3, 2023
[Fix] Wrong fix of cache use for STM32
@projectgus
Copy link
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

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

Successfully merging this pull request may close these issues.

None yet

3 participants