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

IncrementalEncoder: factor out the quadrature state machine #4580

Merged
merged 5 commits into from
Apr 9, 2021

Conversation

jepler
Copy link
Member

@jepler jepler commented Apr 8, 2021

.. and use the same one in samd, nrf, and raspberrypi.

nrf used a different one which may be the cause of #3875 (but I didn't actually test so I don't know if it's fixed)

If we want to substitute a smaller implementation (like one without a table, as discussed in #4559) now we can do it just once instead of 3+ times.

esp32s2 wasn't touched, as it is using a hardware quadrature decoder module.

@ladyada
Copy link
Member

ladyada commented Apr 8, 2021

i can test this on samd later cause im going to make a rotary trinkey

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

I tested this on a Feather Sense (nrf) and a QT Py M0. On both I got no position changes: .position was always 0.

I did not test with an RP2040 board.

Reloading both boards with 6.2.0 showed both could work. However, I noticed that the nrf and the atmel-samd implementations run in opposite directions! One goes up, and one goes down, when turned in the same direction. We'll need to pick a direction and note in the release notes that 7.0.0 has reversed one or the other. I presume the nrf post will change because the code is copied from atmel-samd, but we should double-check.

Test program:

import board,rotaryio,time
r = rotaryio.IncrementalEncoder(board.A0, board.A1)
while True:
    print(r.position)
    time.sleep(0.5)

@jepler jepler marked this pull request as draft April 9, 2021 12:22
@jepler
Copy link
Member Author

jepler commented Apr 9, 2021

I'll convert this to a draft and do further testing before taking up anybody else's time. I stand by the idea that it's a good idea to unify these, especially when discussing alternate implementations of the quadrature state machine, but clearly the implementation is flawed.

This lost line caused incremental encoders to stay stuck at 0 forever.

I seem to have lost it while trying to create tidy commits :frown:
@jepler
Copy link
Member Author

jepler commented Apr 9, 2021

Ah there was an obvious problem, yay. Setting back to ready-for-review!

@jepler jepler marked this pull request as ready for review April 9, 2021 13:58
@dhalbert
Copy link
Collaborator

dhalbert commented Apr 9, 2021

OK, I pushed several changes:

  • Changed 4-bit struct fields to whole bytes. The amount of RAM saved is minimal, at the cost of larger code size, which is an issue on some boards.
  • Fixed nrf ISR for rotaryio: it was not setting the values properly.
  • Made the direction of rotation be the same across the three ports. Used the atmel-samd direction as the default, since there are more boards out there. This necessitated remembering the pin swap on raspberrypi, and it needs to start out "swapped" due to the direction. We will need to document this incompatible change in the release notes.

I am still wondering about the requirement for consecutive pins on the RP2040. Yes, then you can use PIO, but it makes rotaryio code less portable across boards. I lucked out because I was using A0 and A1, which happened to be consecutive on the Feather RP2040, but it was just luck.

Tested on Feather Sense (nrf), QT Py M0, and Feather RP2040.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

All set now after trading reviews :) . Thanks @jepler for doing the refactoring.

@dhalbert dhalbert merged commit 1b60c9d into adafruit:main Apr 9, 2021
@jepler jepler deleted the incrementalencoder-refactor branch November 3, 2021 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants