miketeachman / micropython-rotary

MicroPython module to read a rotary encoder.
MIT License
269 stars 56 forks source link

Add Raspberry Pico support #12

Closed epmoyer closed 3 years ago

epmoyer commented 3 years ago

Mike, fantastic job on this project. It was a pleasure to work with.

I have added Pico support; which was remarkably easy due to the excellent structure and documentation; kudos! I am developing/testing on a Raspberry Pico with a Keyes KY-040 and it is running rock solid for me.

epmoyer commented 3 years ago

Ah, of course!

Cheers!

epmoyer commented 3 years ago

Apologies for not updating/validating all the examples the first time around! Here is the test output from running on the Pico:

Screen Shot 2021-04-05 at 9 32 37 PM

Screen Shot 2021-04-05 at 9 32 54 PM

Screen Shot 2021-04-05 at 9 33 21 PM

And my test fixture for the 2 encoder case: image

Cheers!

miketeachman commented 3 years ago

Hi Eric, @epmoyer Thanks a ton for making this PR and doing all the testing! I tried all the examples on a rPi Pico board and they work well. I reviewed the changes to the code and docs - it looks solid. There are just a couple of minor changes that I recommend. I looked at the default pins in the examples and would like to change them to clk=13, and dt=14 (single encoder examples). For the two encoder example, change them to clk=13, dt=14, and clk=18, dt=19. Why? this uses the same pins for ESP32 and rPi Pico boards. And, it avoids using any ESP strapping pins. (I used pin 15 in the original examples, which was a poor choice as it goes against the recommendations outlined in the rotary documentation). It was always the intent that users will need to customize the pin selections, but this will at least be an improvement on the original pin choices. Note that the pin naming conventions is completely different for the PyBoards (e.g. "X7", "Y5", "X21", etc).

Please take look and see if these proposed changes make sense to you. If so, change your PR code and I'll merge the PR. If not, let's continue the discussion and figure out a good path forwards.

Review comments follow ...

epmoyer commented 3 years ago

All changes incorporated. Thanks for taking the time to review/comment! I know giving someone a PR is like giving them a puppy :) Hopefully this little guy won't make trouble. Now run to Mike little Pico, and be a good boy... 🐕

I re-ran the tests just to make sure the pin changes caused no problems. test_example_simple_13_14_18_19

test_example_asyncio_13_14_18_19

test_example_asyncio_class_13_14_18_19

miketeachman commented 3 years ago

Looks great ! Thanks again. Mike