adafruit / Adafruit_CircuitPython_PCA9685

Adafruit CircuitPython driver for PCA9685 16-channel, 12-bit PWM LED & servo driver chip.
MIT License
120 stars 64 forks source link

Fixed issue 31 PCA9685 configured to take i2c commands from all addre… #32

Closed ScottMonaghan closed 3 years ago

ScottMonaghan commented 3 years ago

…sses

Issue description: I'm combining my PCA9685 with HT16K33 8x8 matrix. As soon as I send a command to the HT1633 the PCA stops responding and all my connected servos go limp.

The solution for this issue was identified here: rwaldron/johnny-five#1591 (comment)

It appears the issue was fixed in the arduino library but not corrected in circuit python (see adafruit/Adafruit-PWM-Servo-Driver-Library@9f8e1dd#diff-5d1e3a5213c0ef6dedb2baf5cc323727L106-R110)

I confirmed the fix below works for me: Change line 163 of adafruit_pca9685 in frequency(self, freq) from self.mode1_reg = old_mode | 0xa1 # Mode 1, autoincrement on to self.mode1_reg = old_mode | 0xa0 # Mode 1, autoincrement on, fix to stop pca9685 from accepting commands at all addresses

I will submit a pull request with this change.

kattni commented 3 years ago

@ScottMonaghan Hello! Thank you for the contribution! The checks on this PR have failed. Please check out this guide to find out how to handle the formatting and Pylint checks. If you need assistance, please let us know. Tagging @FoamyGuy who can also assist you.

ScottMonaghan commented 3 years ago

@kattni and @FoamyGuy. I've updated my code to comply with pylint and Black. Also, I had to update one of the Adafruit-provided examples to comply with pylint (had to change import order of libraries).

Let me know if you need anything else!

FoamyGuy commented 3 years ago

@ScottMonaghan Thank you for submitting this! Also thanks for working through the CI checks, including the unrelated pylint error that popped up on you during the process.

I took a look through the the thread you linked. It does venture a bit deeper than my current understanding goes, but I do see the conclusion at the end and how you've applied the same fix here and it does make sense to me.

I do have one remaining minor question about the comment near this updated code: It specifies Mode 1. I am unsure of the context it's being used in though. If the change that we've made here results in using a different mode (in whatever context it's used) we should probably update the comment as well.

ScottMonaghan commented 3 years ago

@foamyguy I based the comment on @LadyAda’s edit here which still calls out mode 1: https://github.com/adafruit/Adafruit-PWM-Servo-Driver-Library/commit/9f8e1dd.

I assumed the original 0xA1 was a typo and 0xA0 was the correct mode 1 based on Lady Ada’s edit.

For confirmation you can refer to the PCA9685 data sheet.

ScottMonaghan commented 3 years ago

@FoamyGuy and @kattni Great! Is there anything else you need from me?

kattni commented 3 years ago

@ScottMonaghan Not at the moment! We should probably have someone else test this PR before merging, but that's to sort out on our end, not yours. It'll be a few days before I can do it. We'll figure it out though! Simply wanted to give you a heads up.

ScottMonaghan commented 3 years ago

@FoamyGuy and @kattni , I hope you both are well. I just wanted to follow up on this pull request.

I'm about to embark on a new open-source project with this library on the raspberry pi and it would be great if we can just pull down dependent libraries from pip without worrying about having to modify.

kattni commented 3 years ago

@ScottMonaghan This should be available on PyPI as soon as the release assets are deployed. It will be available in the Adafruit CircuitPython Library Bundle tomorrow.

ScottMonaghan commented 3 years ago

@kattni thank you! I'm honored to be able to contribute!