Skip to content

ESP32: New hardware PCNT Counter() and Encoder(). #6639

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

Draft
wants to merge 153 commits into
base: master
Choose a base branch
from

Conversation

IhorNehrutsa
Copy link
Contributor

@IhorNehrutsa IhorNehrutsa commented Nov 20, 2020

@IhorNehrutsa IhorNehrutsa changed the title Feature/esp32 pcnt quad ESP32: New features PCNT() and QUAD() Nov 20, 2020
@peterhinch
Copy link
Contributor

To offer a general comment, encoder interfaces are difficult because of the effects of vibration. Accuracy can be lost as the inputs can experience brief pulses of arbitrary duration. These don't constitute systematic rotation but can be wrongly interpreted as such.

I haven't studied the code in detail but the fact that this solution offers detection of rising, falling, or both edges rings an alarm bell: in my experience the state machine must detect both edges to avoid loss of precision in the presence of vibration.

@IhorNehrutsa
Copy link
Contributor Author

IhorNehrutsa commented Nov 24, 2020

To all. Could someone test the pulse counter and quadrature counter in industrial hardware with a big PPR?
image

@peterhinch
Copy link
Contributor

While that is worthwhile, it is hard to replicate the situation where the encoder is positioned precisely at a transition and machine vibration causes jitter around that point. That is best simulated with a pulse generator over a range of pulse widths. There is a risk of creating an interface where small positional errors only become apparent after days of continuous machine running.

@robert-hh
Copy link
Contributor

How does the QUAD API code return the direction of the rotation in addition to the counts?

@IhorNehrutsa IhorNehrutsa changed the title ESP32: New features PCNT() and QUAD() ESP32: New features PCNT() and QUAD(). Sep 21, 2021
Comment on lines +134 to +141
STATIC void register_isr_handler(void) {
if (pcnt_isr_handle == NULL) {
check_esp_err(pcnt_isr_register(pcnt_intr_handler, (void *)0, (int)0, (pcnt_isr_handle_t *)&pcnt_isr_handle));
if (pcnt_isr_handle == NULL) {
mp_raise_msg(&mp_type_Exception, MP_ERROR_TEXT("wrap interrupt failed"));
}
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using pcnt_isr_handler_add() instead would give the unit demultiplexing inside the isr handler for free, in turn simplifying the whole handler. See .e.g https://github.com/espressif/esp-idf/blob/faf0f61cdba67fa5f7e680f2f5b0966c8d69ce9a/examples/peripherals/pcnt/pulse_count_event/main/pcnt_event_example_main.c

@jonathanhogg
Copy link
Contributor

This implements two types of counters on top of the the esp32 PCNT. Why was this solution choosen over a direct implementation of the PCNT API? The Counter and Encoder could then easily be subclassed from machine.PCNT leaving all the options of the underlying hardware available for other type of counters.

The intent is to have a common MicroPython API that works across different ports that implement counting and quadrature decoding in different ways. There has been much discussion of this issue across this PR and #8072, #7911, #7582, #5216 and #5496.

Perhaps someone in the future will have a burning need to use some missing feature of the raw hardware and will refactor this implementation to work on top of a new esp32.PCNT class instead. As the author of a direct implementation of the PCNT API (#7582) that I am using in production equipment, I personally feel that there is little additional functionality in the underlying hardware that is worth the additional complexity.

Right now I feel like just getting anything into mainline would be great.

@harbaum
Copy link

harbaum commented Jan 23, 2022

Right now I feel like just getting anything into mainline would be great.

I don't really agree with the anything.

But anyway, I think that adding a simple esp32 specific module is more likely to work as this would not depend on any yet-to-agree-on API. But a python module subclassing from that could easily be added implementing these APIs once they become available.

additional complexity.

I don't think there's additional complexity, PCNT is rather simple. Both counter modules together are IMHO more complex than a simple wrapper module around PCNT. And the python subclasses don't count as there's no need to make them part of the firmware although you could of course freeze them into it if you wish to.

@IhorNehrutsa
Copy link
Contributor Author

@harbaum

This has changed in IDF 4.3 and there's now pcnt_get_event_status() for this.

The pcnt_get_event_status() may be use later when MicroPython will use IDFv4.3.

MicroPython v1.18 on 2022-01-17; ESP32 module with ESP32
Type "help()" for more information.
>>> import platform
>>> platform.platform()
'MicroPython-1.18.0-xtensa-IDFv4.2.2-with-newlib3.0.0'

@jonathanhogg
Copy link
Contributor

Honestly, I'm not sure why the esp32/pcnt.rst file exists. I don't ever see that file being pulled into mainline.

The documentation for the full common API should be covered by #8072, with any port differences summarised briefly in esp32/quickref.rst. This would be the approach used for all of the other machine documentation.

@IhorNehrutsa
Copy link
Contributor Author

Honestly, I'm not sure why the esp32/pcnt.rst file exists. I don't ever see that file being pulled into mainline.

The pcnt.rst was added more then year ago.
I propose to leave this question to @dpgeorge consideration.

if (PCNT.status_unit[id].ZERO_LAT) {
if (self->counter == 0) {
self->status |= EVT_ZERO;
mp_sched_schedule(self->handler_zero, MP_OBJ_FROM_PTR(self));
Copy link

@harbaum harbaum Feb 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this supposed to be used by the user? This triggers when the hardware counter passes zero but the event is only scheduled if self->counter also is zero.

But the user setting the counter using value() imho can set an arbitrary offsets on self->counter to values != 0. Then the hardware counter and self->counter won't pass zero at the same time. Or am I misunderstanding something?

int aPinNumber;
int bPinNumber;

volatile int64_t counter;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to understand how self->counter is supposed to work. And I am not sure it actually does what it's intended to. This is my understanding:

self->counter serves two purposes: a) it's something like additional upper bits for the 16 bit hardware counter and b) it allows to set the counter to any value which the hardware counter does not allow. self->counter then basically becomes an offset

The problem I see is in the trigger events of the hardware counter, especially EVT_ZERO. The hardware triggers only on the hardware counter. The interrupt routines try to take this into account by additionally comparing self->counter with zero or the match values. This may actually work as long as self->counter is only used as the upper bits. But it will IMHO fail if it's used to adopt to the fact that the hardware counter cannot be set. If self->counter is set instead while the user tries to set the counter then the hardware counter and self->counter imho get out of sync. And thus they'll not pass the trigger values at the same time. Thus they e.g. don't pass zero at the same time and the EVT_ZERO will not trigger and it seems to me that the match triggers also won't work as intended.

@AmirHmZz
Copy link
Contributor

AmirHmZz commented May 24, 2022

Thanks for everyone who is working on this! Any updates on merging this PR? @dpgeorge

IhorNehrutsa added a commit to IhorNehrutsa/micropython that referenced this pull request Jun 16, 2022
Just rebase to Version 1.19.xx from micropython#6639
@IhorNehrutsa IhorNehrutsa marked this pull request as draft June 16, 2022 21:41
@IhorNehrutsa
Copy link
Contributor Author

This pull request is not possible to rebase on master.
Please switch to the
ESP32: Add Quadrature Encoder and Pulse Counter classes. #8766

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Jul 30, 2022
readline: make ctrl-l clear screen & redraw line
IhorNehrutsa added a commit to IhorNehrutsa/micropython that referenced this pull request Sep 6, 2022
Just rebase to Version 1.19.xx from micropython#6639
IhorNehrutsa added a commit to IhorNehrutsa/micropython that referenced this pull request Oct 10, 2022
Just rebase to Version 1.19.xx from micropython#6639
IhorNehrutsa added a commit to IhorNehrutsa/micropython that referenced this pull request Nov 5, 2022
Just rebase to Version 1.19.xx from micropython#6639
IhorNehrutsa added a commit to IhorNehrutsa/micropython that referenced this pull request Feb 20, 2023
Just rebase to Version 1.19.xx from micropython#6639
IhorNehrutsa added a commit to IhorNehrutsa/micropython that referenced this pull request Mar 10, 2023
Just rebase to Version 1.19.xx from micropython#6639
@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.