ina219 i2c driver: high side current and voltage sensor #292

NeuralSpaz · 2018-10-13T12:57:59Z

adds initial experimental driver for Texas Instruments i2c INA219 high side current sensor IC.

basic driver operation verified on raspberry pi zero w. ina219/cmd
depends on physic.Power in add more SI units #294

maruel

Hi! Thanks a lot for this extensive change! It's very appreciated that you fix TODOs!

I only nitpicked a few things in the driver itself, it's fine to commit WIP there so beside the copyright, the rest could be done as follow up. For the physic package I'm more stringent on the quality of the docs and coherency, hence my comments are focusing on this package.

I'd like to ask you a favor; would you mind to split this PR in 3?

One for the rounding improvement in physic, and only thing
One adding the new physic types
The rest

The reason is that if someone wants to figure out when rounding was changed, or when new types were added, it's simple if they are standalone comments.

maruel · 2018-10-13T21:21:05Z

conn/physic/example_test.go

@@ -67,7 +67,7 @@ func ExampleForce() {
 	fmt.Println(physic.PoundForce)
 	// Output:
 	// 10mN
-	// 990.569kN
+	// 990.570kN


I really wonder why I did the multiplication. I think you can replace line 66 with fmt.Println(physic.EarthGravity)

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

conn/physic/units.go

+}
+
+const (
+	//Watt is unit of power J/s, kg⋅m²⋅s⁻³


Space between // and Watt.

maruel · 2018-10-13T21:22:53Z

conn/physic/units.go

+}
+
+const (
+	//Farad is a unit of capacitance. kg⁻¹⋅m⁻²⋅s⁴A²


maruel · 2018-10-13T21:26:54Z

conn/physic/units.go

+	GigaJoule  Energy = 1000 * MegaJoule
+)
+
+// Capacitance is a measurement of capacitance stored as a pico farad.


I used ElectricResistance because I felt that Resistance was a bit ambiguous. Resistance can be measured also as Drag, Hardness and Thermal resistance.

I used ElectricCurrent and ElectricPotential to be coherent; so I feel that Capacitance may be worth having the same treatment, even if it's not ambiguous. Maybe I made an error, I'm still undecided.

Just thinking out loud here.

I think having it as ElectricalCapacitance is more consistence with the current units even though there is no ambiguity and would cause less confusion than the other way; dropping the Electrical prefix.
Also thinking about it Flux probably should be LuminousFlux so as not to confuse lumens with webers or teslas in the future.

maruel · 2018-10-13T21:27:18Z

conn/physic/units.go

+}
+
+const (
+	//Joule is a unit of work. kg⋅m2⋅s⁻²


maruel · 2018-10-13T21:39:36Z

experimental/devices/ina219/ina219.go

+		return PowerMonitor{}, errReadShunt
+	}
+	var pm PowerMonitor
+	pm.Shunt = physic.ElectricPotential(int16(binary.BigEndian.Uint16(rx)))


You'd benefit from mmr here too if you want to use it.

maruel · 2018-10-13T21:40:19Z

experimental/devices/ina219/ina219_test.go

@@ -0,0 +1,730 @@
+// Copyright 2016 The Periph Authors. All rights reserved.


Update year.

maruel · 2018-10-13T21:40:52Z

experimental/devices/ina219/ina219doc.go

@@ -0,0 +1,18 @@
+// Copyright 2016 The Periph Authors. All rights reserved.


Update year. I'd recommend doc.go since this is what is generally used in other packages.

maruel · 2018-10-13T21:41:01Z

experimental/devices/ina219/ina219doc.go

+// Slave Address:
+// Depending which pins the A1, A0 pins are connected to will change the slave
+// address as bellow. Default configuration is address 0x40
+//


Remove two trailing //

experimental/devices/ina219/ina219smoketest/ina219smoketest.go

maruel · 2018-10-13T21:47:00Z

gohci

NeuralSpaz · 2018-10-14T12:21:50Z

Thank you for your review, I have split the PR, first time doing that :S

maruel

I added a few optional comments, I'll merge as soon as #294 is merged in, as these can be done in follow ups, I don't mind here as experimental is meant to improve drivers via iteration.

experimental/devices/ina219/cmd/cmd.go

experimental/devices/ina219/doc.go

maruel · 2018-10-14T13:58:16Z

experimental/devices/ina219/doc.go

+// Datasheet
+// http://www.ti.com/lit/ds/symlink/ina219.pdf
+//
+// Slave Address:


Same. You can test locally with godoc -http=:6060 then visit http://localhost:6060/pkg/periph.io/x/periph/experimental/devices/ina219.

I think you'll have to remove the ':' for this to work.

That said, I think this belongs to Opts, not to package level documentation.

I have kept note about calibration in the package docs as I think it is important and easy to overlook with defaults.

experimental/devices/ina219/example_test.go

experimental/devices/ina219/ina219.go

maruel · 2018-10-14T17:04:58Z

experimental/devices/ina219/ina219.go

+// overflow int64 or loose resolution.
+const calibratescale int64 = ((int64(physic.Ampere) * int64(physic.Ohm)) / 100000) << 12
+
+// Calibrate sets the scaling factor of the current and power registers for the


lower case calibrate

maruel · 2018-10-14T17:05:06Z

gohci

maruel · 2018-10-14T17:12:02Z

You'll have to rebase as the tests currently fail.

git remote add periph https://github.com/google/periph
go fetch periph
git rebase periph/master
git push -f origin ina219

initial commit

split of PR

Change Configuration to Opts. Change default constants to public Opts variable. Move cmd to ../cmd/ina219. Fix comment formatting. Fix godoc formatting. Return error from smoketest. Change calibrate to private since it is called in New and does not need to be called again the sensor has be calibrated.

maruel · 2018-10-14T17:18:58Z

gohci

maruel · 2018-10-14T17:21:01Z

experimental/devices/ina219/ina219smoketest/ina219smoketest.go:66:27: cannot use config (type ina219.Opts) as type *ina219.Opts in argument to ina219.New

I'm going outside, will merge when I'm back in a few hours and you pushed a fix. My only nit is the Calibrate doc that isn't correctly cased.

change Opts to pointer to *Opts in smoketest

NeuralSpaz · 2018-10-14T17:31:20Z

Thank you for your code review/s it has been extremely helpful and edifying. As I have about dozen or so more IC drivers I would like to contribute, it is all great to know.

maruel · 2018-10-14T19:43:52Z

gohci

maruel

Here's some comments for follow ups.

maruel · 2018-10-14T19:40:21Z

experimental/devices/ina219/ina219.go

+func New(bus i2c.Bus, opts *Opts) (*Dev, error) {
+
+	i2cAddress := DefaultOpts.Address
+	if opts.Address != 0 {


It's fine to force users to pass valid value and not 0, as there's DefaultOpts. Same for line 50, 58, which would simplify this code a bit.

maruel · 2018-10-14T19:43:11Z

experimental/devices/ina219/ina219.go

+	d.mu.Lock()
+	defer d.mu.Unlock()
+
+	var pm PowerMonitor


I'd recommend to change line 139 with:

return PowerMonitor{ Shunt: physic.ElectricPotential(shunt) * 10 * physic.MicroVolt, Voltage: physic.ElectricPotential(bus>>3) * 4 * physic.MilliVolt, Current: physic.ElectricCurrent(current) * d.currentLSB, }, nil

That would make the function a bit more dense.

maruel · 2018-10-14T19:48:46Z

Thank you for your code review/s it has been extremely helpful and edifying. As I have about dozen or so more IC drivers I would like to contribute, it is all great to know.

!!

That's amazing.

Thanks for the thoughtful code, it was really easy to review. To migrate to non-experimental, I'll ask:

implement conn.Resource; https://periph.io/x/periph/conn#Resource; I forgot to ask earlier.
Add a page to periph.io; https://github.com/periph/website

That's pretty much it, the code is pretty good already. Thanks again.

maruel · 2018-10-14T19:49:03Z

Also, you may want to add yourself to AUTHORS and CONTRIBUTORS.

maruel reviewed Oct 13, 2018

View reviewed changes

NeuralSpaz changed the title ~~INA219 Driver TI i2c current and voltage sensor~~ ina219 i2c driver: high side current and voltage sensor Oct 14, 2018

maruel reviewed Oct 14, 2018

View reviewed changes

NeuralSpaz added 5 commits October 15, 2018 04:16

adds ina219 driver : i2c current and voltage sensor

34fa3fd

initial commit

"adds ina219 driver : i2c current and voltage sensor"

4b6ac96

code review edits

ac883e8

split of PR

edit/delete comments

d07f152

fix ina219 smoketest bug

cb7f1bf

change Opts to pointer to *Opts in smoketest

NeuralSpaz mentioned this pull request Oct 14, 2018

unit strings rounding todo #293

Merged

maruel reviewed Oct 14, 2018

View reviewed changes

maruel merged commit afa1663 into google:master Oct 14, 2018

NeuralSpaz deleted the ina219 branch October 14, 2018 23:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ina219 i2c driver: high side current and voltage sensor #292

ina219 i2c driver: high side current and voltage sensor #292

NeuralSpaz commented Oct 13, 2018 •

edited

Loading

maruel left a comment

maruel Oct 13, 2018

maruel Oct 13, 2018

maruel Oct 13, 2018

maruel Oct 13, 2018

NeuralSpaz Oct 13, 2018

maruel Oct 13, 2018

maruel Oct 13, 2018

maruel Oct 13, 2018

maruel Oct 13, 2018

maruel Oct 13, 2018

maruel commented Oct 13, 2018

NeuralSpaz commented Oct 14, 2018

maruel left a comment

maruel Oct 14, 2018

NeuralSpaz Oct 14, 2018 •

edited

Loading

maruel Oct 14, 2018

maruel commented Oct 14, 2018

maruel commented Oct 14, 2018

maruel commented Oct 14, 2018

maruel commented Oct 14, 2018

NeuralSpaz commented Oct 14, 2018

maruel commented Oct 14, 2018

maruel left a comment

maruel Oct 14, 2018

maruel Oct 14, 2018

maruel commented Oct 14, 2018

maruel commented Oct 14, 2018

		@@ -0,0 +1,730 @@
		// Copyright 2016 The Periph Authors. All rights reserved.

		@@ -0,0 +1,18 @@
		// Copyright 2016 The Periph Authors. All rights reserved.

ina219 i2c driver: high side current and voltage sensor #292

ina219 i2c driver: high side current and voltage sensor #292

Conversation

NeuralSpaz commented Oct 13, 2018 • edited Loading

maruel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maruel commented Oct 13, 2018

NeuralSpaz commented Oct 14, 2018

maruel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NeuralSpaz Oct 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maruel commented Oct 14, 2018

maruel commented Oct 14, 2018

maruel commented Oct 14, 2018

maruel commented Oct 14, 2018

NeuralSpaz commented Oct 14, 2018

maruel commented Oct 14, 2018

maruel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maruel commented Oct 14, 2018

maruel commented Oct 14, 2018

NeuralSpaz commented Oct 13, 2018 •

edited

Loading

NeuralSpaz Oct 14, 2018 •

edited

Loading