[feat] Add support for pca9685 pwm module by alvarowolfx · Pull Request #296 · google/periph

alvarowolfx · 2018-10-28T01:22:36Z

Add support for the PCA9685 16 channel PWM module. I create this for a demo on Gophercon Brazil talking about embedded development with Golang and one of the demos was a robot arm (MeArm) controlled through Google Assistant, Cloud IoT Core and Golang running on an Orange Pi Zero. So I extracted the code for controlling the pca9685 module and I'm sending this PR.

Code for the project: https://github.com/alvarowolfx/golang-voice-iot
Demo of it working: https://youtu.be/Xep6dZ3xOYA

Based on https://github.com/wintersandroid/Android-Things-PCA9685
Datasheets: https://cdn-shop.adafruit.com/datasheets/PCA9685.pdf
Product page: https://www.adafruit.com/product/815

maruel

Thanks a lot for the change!

maruel · 2018-10-28T02:21:12Z

+//
+// Based on https://github.com/wintersandroid/Android-Things-PCA9685
+//
+// Datasheets:


For godoc to process this correctly, you need to keep an empty line between and no semi colon, e.g.:

// Datasheet // // https://www.nxp.com/docs/en/data-sheet/PCA9685.pdf

Use the original datasheet, not the copy from Adafruit.

maruel · 2018-10-28T02:22:13Z

+// https://cdn-shop.adafruit.com/datasheets/PCA9685.pdf
+//
+// Product page:
+// https://www.adafruit.com/product/815


The product page is https://www.nxp.com/products/analog/interfaces/ic-bus/ic-led-controllers/16-channel-12-bit-pwm-fm-plus-ic-bus-led-controller:PCA9685

I do link to adafruit on the website in the "where to buy" section but not in the documentation.

maruel · 2018-10-28T02:24:19Z

+	dev *i2c.Dev
+}
+
+// NewI2CAddress returns a Dev object that communicates over I2C on a alternate address


Merge these two functions, have the caller specify pca9685.I2CAddr every time and comment that it's what should be normally passed.

maruel · 2018-10-28T02:25:58Z

+	"periph.io/x/periph/conn/i2c"
+)
+
+// PCA9685Address i2c default address


There's a bit of inconsistency in the drivers for default I2C addresses, I think I'd prefer I2CAddr here, since PCA9685Address stutters.

https://blog.golang.org/package-names talks a bit about this.

maruel · 2018-10-28T02:28:04Z

+}
+
+// SetPwmFreq set the pwm frequency
+func (d *Dev) SetPwmFreq(freqHz float32) error {


Do not use floating point, use physic.Frequency instead.

maruel · 2018-10-28T02:33:54Z

+		dev: &i2c.Dev{Bus: bus, Addr: address},
+	}
+	err := dev.init()
+	return dev, err


Return nil for *Dev if err != nil

maruel · 2018-10-28T02:34:04Z

+}
+
+func (d *Dev) init() error {
+	d.SetAllPwm(0, 0)


Please check errors thoroughly.

alvarowolfx · 2018-10-28T21:55:22Z

Thanks for the comments, already pushed some updates and need to work more on some ideas.

maruel

Looks pretty good, next cycle should be good.

maruel · 2018-10-29T00:48:59Z

+	"time"
+
+	"periph.io/x/periph/conn/physic"
+


Please keep the imports grouped, as goimports would normally do.

maruel · 2018-10-29T00:50:09Z

+		log.Fatal(err)
+	}
+
+	pca.SetPwmFreq(50)


If you don't want to include error checking to keep the example easier to read, then comment to tell the user that they should. But in practice I'd expect the users to copy-paste, so I recommend to keep the error checking.

maruel · 2018-10-29T00:53:45Z

+
+	prescaleToSend := int(math.Floor(float64(prescaleVal + 0.5)))
+
+	var modeRead []byte


Same reduction as above.

maruel · 2018-10-29T00:54:08Z

+
+	oldmode := modeRead[0]
+	newmode := (byte)((oldmode & 0x7F) | 0x10)                     // sleep
+	if _, err := d.dev.Write([]byte{mode1, newmode}); err != nil { // go to sleep;


These 3 can probably be merged (?) I haven't looked at the datasheet to confirm.

I tried sending commands altogether instead of doing 3 call to dev.Write and unfortunately didn't work on the device.

maruel · 2018-10-29T00:54:30Z

+
+	time.Sleep(100 * time.Millisecond)
+
+	if _, err := d.dev.Write([]byte{mode1, (byte)(oldmode | 0x80)}); err != nil {


This is fine to do this:

_, err := d.dev.Write([]byte{mode1, (byte)(oldmode | 0x80)}) return err

maruel · 2018-10-29T00:57:31Z

+
+// SetPwmFreq set the pwm frequency
+func (d *Dev) SetPwmFreq(freqHz physic.Frequency) error {
+	prescaleVal := float32(25 * physic.MegaHertz)


No need for floating point. The rounding can done by adding half of the value you are dividing, all in integers:

p := (25 * physic.MegaHertz / 4096 + freqHz/2) / freqHz

maruel · 2018-10-29T01:00:49Z

+}
+
+// SetPwm set a pwm value for given pca9685 channel
+func (d *Dev) SetPwm(channel int, on uint16, off uint16) error {


Ultimately, I'll ask you to use gpio.Duty for PWM duty cycle values. This can be done in a follow up.
https://periph.io/x/periph/conn/gpio#Duty

maruel · 2018-10-29T01:01:19Z

gohci

maruel · 2018-10-29T01:05:51Z

Please fix the test, thanks.

maruel · 2018-10-30T12:15:26Z

gohci

maruel

Ok so I have a few comments but nothing that is worth doing more back and forth on this PR. Please address the comments in a follow up at your leisure.

One important point is to use the datasheet as the reference. Do not blindly copy paste constants from sources other than the datasheet.

maruel · 2018-10-30T12:08:53Z

+	allLedOffL byte = 0xFC
+	allLedOffH byte = 0xFD
+
+	// Bits


In a follow up, I think it's worth describing what they are for. This is mixing bitmasks for mode2 (ourDrv, invrt), mode1 (allCall, sleep) and restart is unused and I couldn't find it in the datasheet.

So remove restart for now, we'll handle the rest later.

maruel · 2018-10-30T12:10:30Z

+		return err
+	}
+
+	if _, err := d.dev.Write([]byte{mode2, outDrv}); err != nil {


I looked at the datasheet and the way it works is via a memory mapped system where the pointer increments at each byte. That's fairly standard with I²C and https://periph.io/x/periph/conn/mmr exists exactly for this.

That said we should do that in a follow up, let's get something working first.

maruel · 2018-10-30T12:10:49Z

+	return d.SetPwmFreq(50 * physic.Hertz)
+}
+
+// SetPwmFreq set the pwm frequency


In a follow up, please terminate all documentation with a period.

maruel · 2018-10-30T12:14:47Z

+	sleep   byte = 0x10
+	allCall byte = 0x01
+	invrt   byte = 0x10
+	outDrv  byte = 0x04


This comment is only FYI.

This device can sink 25mA and in totem-pole can source 10mA. You always put it in totem-pole at the moment.
I started adding Drive() to some GPIOs (like https://periph.io/x/periph/host/bcm283x#Pin.Drive) but I haven't defined an interface to gpio yet. I realize I should probably have both Sink and Source instead, so maybe I'll change that.

maruel · 2018-10-30T12:16:09Z

+)
+
+// Dev is a handler to pca9685 controller
+type Dev struct {


In a follow up, please add Halt() that sets sleep bit, so that all PWMs stop. That's the essence of Halt. :)

maruel · 2018-10-30T12:17:25Z

+const (
+	mode1      byte = 0x00
+	mode2      byte = 0x01
+	subAdr1    byte = 0x02


These are bitmasks on mode1. I'd vote to remove them since they are not used and you are mixing register addresses and bitmasks here, which is super confusing when looking up the datasheet.

Well, there are subadr1~3 registers, but they are 0x19 to 01A.

alvarowolfx · 2018-10-30T16:01:42Z

Thanks for the awesome help @maruel. I'm still fairly new to Golang and I'm learning a lot with your reviews. I'll be fixing the other drivers that I sent and will be probably sending another one later today (bh1750 light sensor driver). Them I can revisit those drivers. You are completely right, I should stop following blindly and porting from other languages and start looking more into the datasheet and understanding more how it works.

maruel · 2018-10-30T16:36:34Z

No worries, you did well. Reading datasheets is an art in itself so there's a ramp up curve there too.

Some nits from #296. The auto-increment capability of the device can simplify contiguous writes.

Some nits from google/periph#296. The auto-increment capability of the device can simplify contiguous writes.

[feat] Add support for pca9685 pwm module

4e7f3b0

maruel reviewed Oct 28, 2018

View reviewed changes

[feat] Check errors, unexport symbols and improve docs

327205e

maruel reviewed Oct 29, 2018

View reviewed changes

alvarowolfx added 3 commits October 29, 2018 18:23

[fix] Add default i2c address as param and check error

f104b2a

[feat] Improve code and reduce tyep casting

bce2241

[feat] Use gpio.Duty, physic.Angle and physic.Frequency

7bf2896

maruel approved these changes Oct 30, 2018

View reviewed changes

maruel merged commit edb8c30 into google:master Oct 30, 2018

kevinboulain mentioned this pull request Jul 6, 2019

pca9685: cleanup #414

Merged

maruel pushed a commit that referenced this pull request Jul 10, 2019

pca9685: cleanup (#414)

62f92ed

Some nits from #296. The auto-increment capability of the device can simplify contiguous writes.

maruel pushed a commit to periph/devices that referenced this pull request Dec 24, 2020

pca9685: cleanup (#414)

4439ce0

Some nits from google/periph#296. The auto-increment capability of the device can simplify contiguous writes.

maruel pushed a commit to periph/host that referenced this pull request Dec 24, 2020

pca9685: cleanup (#414)

b147c6c

Some nits from google/periph#296. The auto-increment capability of the device can simplify contiguous writes.


		prescaleToSend := int(math.Floor(float64(prescaleVal + 0.5)))

		var modeRead []byte


		time.Sleep(100 * time.Millisecond)

		if _, err := d.dev.Write([]byte{mode1, (byte)(oldmode \| 0x80)}); err != nil {

Conversation

alvarowolfx commented Oct 28, 2018

Uh oh!

maruel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alvarowolfx commented Oct 28, 2018

Uh oh!

maruel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maruel commented Oct 29, 2018

Uh oh!

maruel commented Oct 29, 2018

Uh oh!

maruel commented Oct 30, 2018

Uh oh!

maruel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alvarowolfx commented Oct 30, 2018

Uh oh!

maruel commented Oct 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants