Skip to content

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

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

Merged
merged 4 commits into from
Apr 13, 2021

Conversation

ScottMonaghan
Copy link
Contributor

…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.

…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
Copy link
Contributor

kattni commented Aug 19, 2020

@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
Copy link
Contributor Author

@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
Copy link
Contributor

@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
Copy link
Contributor Author

@FoamyGuy I based the comment on @ladyada’s edit here which still calls out mode 1: adafruit/Adafruit-PWM-Servo-Driver-Library@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.

@tannewt tannewt requested a review from FoamyGuy August 25, 2020 01:07
Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

I took a look through the data sheet and while I don't understand everything in there I do think I can understand enough to see that Mode 1 is referring to the register itself not really the value that is being set into the register.

This PR looks good to me now. But have not tested it, don't have this device.

@ScottMonaghan
Copy link
Contributor Author

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

@kattni
Copy link
Contributor

kattni commented Aug 27, 2020

@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
Copy link
Contributor Author

@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.

Copy link
Contributor

@kattni kattni left a comment

Choose a reason for hiding this comment

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

I'm ok with merging this. I haven't tested it, but I verified that the same change was made in the Arduino library. Thanks again for the submission, and thank you for your patience!

@kattni kattni merged commit 52217ce into adafruit:master Apr 13, 2021
@kattni
Copy link
Contributor

kattni commented Apr 13, 2021

@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
Copy link
Contributor Author

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

adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Apr 14, 2021
Updating https://github.com/adafruit/Adafruit_CircuitPython_PCA9685 to 3.3.6 from 3.3.5:
  > Merge pull request adafruit/Adafruit_CircuitPython_PCA9685#32 from ScottMonaghan/smonaghan_pca9685_fix_mode1_reg
  > "Increase duplicate code check threshold "

Updating https://github.com/adafruit/Adafruit_CircuitPython_FunHouse to 1.1.1 from 1.1.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_FunHouse#7 from makermelissa/main
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.

PCA9685 configured to take i2c commands from all addresses.
3 participants