-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
ESP32: Add Quadrature Encoder and Pulse Counter classes. #8766
base: master
Are you sure you want to change the base?
ESP32: Add Quadrature Encoder and Pulse Counter classes. #8766
Conversation
Please consider #7582 as a replacement for this one. The other one is more flexible as it exposes the PCNT API and implements the machine.Counter and machine.Encoder in Pyrhon which is IMHO a much nicer approach as it allows to use PCNT for further counter types like on that uses two dedicated pins to count up and down. |
I would still like to understand why this PR is preferable over e.g. #7582 This one is IMHO significantly more limited than #7582 as it hides the underlying PCNT API of the ESP32. #7582 exposes this and implements Encoder and Counter as one of many possibilities on top of that. Furthermore this PR IMHO does not handle setting the counter correctly as it implements a separate offset to the non-settable hardware counter which in turn won't work with the IRQs (counter passes zero, counter reaches a certain value) as the offset happens on the software side and is not taken into account by the hardware side. If I am wrong with my assumptions, then please correct me. |
Hello - new here and just built the branch and tested it with a 100 PPR CNC rotary wheel and initial test works great! I do wonder if it might make sense for the encoder at least to have a default non-zero filter_ns value. I needed it due to bounce on my encoder and suspect a lot of users will be confused by behavior caused by not setting filter_ns for rotary encoders. Those who have use-cases where no filter is needed will likely be more advanced users who will know to override the default. For reference, this was tested on an Unexpected Maker ProS3. If there are any specific tests the team needs that I can help with, let me know - otherwise I'll report in if I see any issues during my development. |
@msamblanet Can you check the maximum rotation speed of the encoder shaft when measuring the exact pulse count without pulse losses? |
@IhorNehrutsa - I don’t have a mechanical drive other than my hand. I was stress testing it by spinning around 6 full rotations per second (600 steps or 2400 counts per second) with no loss (way faster than my real world use case). I was using a 10000ns filter (arbitrary choice but a filter was needed with my setup). Given the frequency of the esp32 pcnt unit (25mhz I think) I’m not too worried here :) I think once during my testing I lost a couple pulses but I am testing in a breadboard with a couple iffy connections. As it was a single occurrence, I’m ignoring it unless it happens repeatable. Edit - corrected pulses to counts in rate |
I just tried to rebase this PR so I could test it combined with another PR which is based on a new MP base. This PR fails to build when rebased at 2 points in machine_encoder.c. Full error output below. The branch without a rebase builds fine for me.
|
0a42fe9
to
d99a22a
Compare
be3911f
to
d7fa619
Compare
dcc3c9d
to
87e4b65
Compare
@msamblanet, @AmirHmZz |
Tested with industrial 6250ppr encoder.
|
Add _src inverse pulse input.
|
477a068
to
1483eca
Compare
002923f
to
cec1abf
Compare
47db009
to
5ef41fa
Compare
5ef41fa
to
a6d25ca
Compare
462994a
to
7b82d99
Compare
7b82d99
to
4d752d2
Compare
4d752d2
to
ab46ae5
Compare
a58be0b
to
3b44fa5
Compare
d0a3849
to
4bd7b04
Compare
Code size report:
|
5bbba6d
to
4da4454
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8766 +/- ##
=======================================
Coverage 98.36% 98.36%
=======================================
Files 159 159
Lines 21088 21093 +5
=======================================
+ Hits 20743 20748 +5
Misses 345 345 ☔ View full report in Codecov by Sentry. |
76ffec5
to
bcd081d
Compare
Signed-off-by: IhorNehrutsa <Ihor.Nehrutsa@gmail.com>
bcd081d
to
c5bfb0d
Compare
This is an automated heads-up that we've just merged a Pull Request See #13763 A search suggests this PR might apply the STATIC macro to some C code. If it Although this is an automated message, feel free to @-reply to me directly if |
Applied to industrial encoders. Has a 32-bit wide counter!!! Has a 32-bit wide matching value !!! (64 bit wide is possible).
![image](https://private-user-images.githubusercontent.com/70886343/262034896-29f55b3b-37e9-44e1-b840-6da36f457d74.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjE4NTEwMzksIm5iZiI6MTcyMTg1MDczOSwicGF0aCI6Ii83MDg4NjM0My8yNjIwMzQ4OTYtMjlmNTViM2ItMzdlOS00NGUxLWI4NDAtNmRhMzZmNDU3ZDc0LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA3MjQlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNzI0VDE5NTIxOVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWEzZTdjMDUxNWY5ZDU2MGJjNzMyMzA2MjgzMjZiOTU0MTdhNDI1YzllMTQwMDUyMjA2NDNkZDUyZmY3Mzc2MWYmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.fibAcXajfx6pqT2JRDIhmdWpMfR585zm07A6lKsIXzc)
Count pulses on Pin(17) with direction on Pin(16) an irq handler on zero value.
Please note that this pull request
ESP32: Add Quadrature Encoder and Pulse Counter classes. #8766
and
docs\machine: Add Counter and Encoder classes. #8072
and
mimxrt: Add Quadrature Encoder and Pulse Counter classes. #7911
are mutually agreed upon.
Note: MicroPython-lib PR Add Stepper Motor PWM-Counter driver. requires this PR.