adafruit / circuitpython

CircuitPython - a Python implementation for teaching coding with microcontrollers
https://circuitpython.org
Other
4k stars 1.18k forks source link

rotaryio 1/4 of possible resolution? #1677

Closed KacperLa closed 2 years ago

KacperLa commented 5 years ago

It seems to me that the way rotaryio is currently written is limited to 1/4 of its possible resolution. On encoder library's for the Arduino the rising and falling of the a and b pins is used to calculate the position of the encoder rather then their states. Is it possible to implement this proposed method?

dhalbert commented 5 years ago

The current method is meant for use with detented Gray Code rotary encoders: one count per click. This has the advantage of not needing debouncing. We can add modes for other kinds of encoders, x2 and x4, but you would see more bobbling back and forth unless the outputs were debounced with capacitors. What kind of encoders are you thinking of using?

papahabla commented 5 years ago

I noticed that when using rotaryio, my detent encoder was only registering every other detent as a position change. Based on the c code, I wrote the following script to test a slightly different implementation and it seems to work pretty well for me with full detent resolution. Of course it needs to be ported back to C to take advantage of using interrupts.

PEC16-4215F-N0024

import board
import digitalio
import time

a = digitalio.DigitalInOut(board.A2)
a.direction = digitalio.Direction.INPUT
b = digitalio.DigitalInOut(board.A3)
b.direction = digitalio.Direction.INPUT

# For 11 (True, True) at detent
# I.e. c=G, a/b pulled-up
# ccw<-------v(n)-------->cw
# 11<-00<-10<-11->00->10->11
CCW = 0b11100011
CW = 0b11001011
a.pull = digitalio.Pull.UP
b.pull = digitalio.Pull.UP

# For 00 (False, False) at detent
# I.e. c=VDC, a/b pulled-down
# ccw<-------v(n)-------->cw
# 00<-11<-01<-00->11->01->00
#CCW = 0b00011100
#CW = 0b00110100
#a.pull = digitalio.Pull.DOWN
#b.pull = digitalio.Pull.DOWN

print(a.value)
print(b.value)
pv = (a.value << 1) | b.value
idx = 0

while True:
    v = (a.value << 1) | b.value
    if v != pv & 0x3:
        pv = (pv << 2 | v) & 0xFF
        if pv == CCW:
            print('{} LEFT'.format(idx))
        if pv == CW:
            print('{} RIGHT'.format(idx))
        idx += 1
    time.sleep(.01)  # any higher and it will not work very well
dhalbert commented 5 years ago

@KacperLa @papahabla Which boards are you using? The nrf and atmel-samd implementations differ.

papahabla commented 5 years ago

@dhalbert I am using the ItsyBitsy M4 Express.

KacperLa commented 5 years ago

I have been using the following encoder:

https://www.amazon.com/Signswise-Incremental-Encoder-Dc5-24v-Voltage/dp/B00UTIFCVA/ref=sr_1_2?keywords=encoder&qid=1553538276&s=gateway&sr=8-2

These are great for hobby use as they are very accessible and very precise. I believe its a hall effect quadrature encoder, to use it you have to wire it through a logic lever shifter but there are encoders like it that don't require the shifter, like this one that is mounted on the back of a motor:

https://www.robotshop.com/en/andymark-neverest-orbital-20-12v-340rpm-175-oz-in-gearmotor-w--encoder.html

I am currently using these two encoders in a project with the feather M4 and would really appreciate the extra precision of their full resolution.

Thank you so much

nickzoic commented 5 years ago

Yep, it's dividing down by four to be compatible with the other rotaryio implementations (see #1045 / #1521)

It'd be easy enough for it to be modified not to, perhaps by adding a constructor argument 'divider' which can default to 4. Values 1,2 and 4 are probably pretty sensible for different kinds of encoder.

ie = rotaryio.IncrementalEncoder(board.A0, board.A1, divider=1)

@tannewt what do you reckon?

-----Nick

tannewt commented 5 years ago

@nickzoic divider with a default of 4 for backwards compatibility sounds good to me.

KacperLa commented 5 years ago

That would be fantastic!!!

nickzoic commented 5 years ago

There's a potential fix in the patch above, but I haven't tested it yet ... mostly because I can't find the rotary encoder I was testing with last time! I'll get to it in the next couple of days and put a PR in. (it also still needs some things like checking that 1 <= divisor <= 64 and so on ...)

dhalbert commented 5 years ago

Is your code equivalent to the 16-byte transitions table I used in the atmel-samd code? https://github.com/adafruit/circuitpython/blob/master/ports/atmel-samd/common-hal/rotaryio/IncrementalEncoder.c#L121.

Could we unify the two implementations? It seems like they do the equivalent thing. Common routines could be put in shared-modules/rotaryio/ -- that's the style for lower-level code that's port-independent.

I looked at a lot of encoder code when I was writing that. There's a lot of bad code out there that doesn't really work right. The transitions table was a more robust variant on the best code I found.

nickzoic commented 5 years ago

The previous one (in the previous PR) definitely was: this one works correctly on the whiteboard but I haven't tried it in hardware! I'll find a rotary encoder and give it a go (all it's really doing is code-golfing your state transition table down to logical operations: note that in some of the 'bad transitions' the counter is both incremented and decremented) I need to write an explanatory comment in there before it's ready to PR

Yep, we could have the common-hal code call back into the shared-bindings shared-modules code at the state-transition level, with a call something like:

void rotaryio_incrementalencoder_state_update(bool state_a, bool state_b)

That'd move the responsibility for turning the state transitions into counts back into shared-bindings. I agree it's a good idea: I should have thought of that while I was adding the divider to both of them ...

dhalbert commented 5 years ago

Remote physical debugging: I built the latest push on your fork branch, and it works with my rotary encoder (while you're looking for yours).

nickzoic commented 5 years ago

On Fri, 5 Apr 2019, at 08:05, Dan Halbert wrote:

Remote physical debugging: I built the latest push on your fork branch, and it works with my rotary encoder (while you're looking for yours).

Thanks! I'll pick one up from somewhere, and in the meantime do some code cleanup to get that ready for PR!

KacperLa commented 5 years ago

This is great thank you for your work. A friend of mine is also using the feather m4 express with several encoders, we are both looking forward to being able to get the full resolution of the quadrature encoders we are using.

I was trying to compile the .u2f file with your code for the feather m4 express, but I get an error when I try to import rotaryio. I am sure I am doing something wrong. I understand that this is still in development but would it be possible to get a copy of the current stable versions of the firmware with this modification for the feather m4 express?

nickzoic commented 5 years ago

Found it! Should be able to get this finished off and PRed tomorrow (GMT+10). (actually, probably not: doing the shared-modules thing is proving slower than I thought)

mikelynders commented 5 years ago

Hi! I am also trying to implement full quadrature resolution with rotaryio for a university project. I tried copying the rotaryio files from nickzoic's repo into the current release of circuit python and compiling but that didn't seem to work for me (using feather M4). Can I second the request for the built .u2f that you guys used for testing? That would help me out alot until the change can be added into the official release.

krittick commented 3 years ago

I'm running into this issue in 6.2.0-beta.2 with several different encoders and can't reliably use them for my projects due to this. Is it still simply just awaiting a PR at this point or does more work need to be done first?

dmikhalsky commented 2 years ago

I was quite surprised that the same encoder increments once per "dent" in Arduino, but once per 3-4 "dents" if connected to Pico running CircuitPython Is there a progress on correcting this? Can I just alter some C code to get Arduino-like behaviour ie higher resolution?

dhalbert commented 2 years ago

@dmikhalsky We don't see skipped increments on RP2040 in general. The MacroPad has an on-board encoder, and the "MacroKeys" program, which uses rotaryio, is changing things once per detent. How fast are you spinning the encoder, and is it a regular encoder like the one in our store?

dmikhalsky commented 2 years ago

@dhalbert Yes, the encoder is vanilla "aliexpress-type" one. I am just starting with CircuitPython and Pico and tried examples from https://learn.adafruit.com/rotary-encoder/circuitpython In both examples the encoder behaves the same. In the HID one, it took me 4-5 full rotations to go from zero volume to full volume. Serial debug shows the same. I rotate the encoder very slowly and it increments every 3-4 dents. I am pretty sure the decoder is OK as I just recently used it in another Arduino project and it was "one dent, one increment"

dhalbert commented 2 years ago

@dmikhalsky I would not expect those results, so it might be a wiring issue. Could you take this to https://forums.adafruit.com/viewforum.php?f=60 ? Post your code and a picture of your wiring. We try not to do support in GitHub: it's for bug and feature tracking, and I don't think this is a pervasive bug.

dhalbert commented 2 years ago

Fixed by #5468, I believe.