Skip to content
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.

Sign up for GitHub

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

piglow: various readability improvements #27

Merged
merged 1 commit into from
Jun 24, 2016
Merged

piglow: various readability improvements #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions piglow/examples_test.go → piglow/example_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,14 @@ import (
)

func Example() {
p, _ := piglow.Open(&i2c.Devfs{Dev: "/dev/i2c-1", Addr: piglow.Address})
p, err := piglow.Open(&i2c.Devfs{Dev: "/dev/i2c-1", Addr: piglow.Address})
if err != nil {
panic(err)
}

p.Setup()
if err := p.Setup(); err != nil {
panic(err)
}

brightness := 0
for i := 0; i < 10; i++ {
Expand Down
51 changes: 15 additions & 36 deletions piglow/examples/strobe.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
package main

import (
"os"
"os/signal"
"syscall"
"time"

"github.com/goiot/devices/piglow"
Expand All @@ -16,42 +13,24 @@ func main() {
panic(err)
}

// catch signals and terminate the app
sigc := make(chan os.Signal, 1)
signal.Notify(sigc,
syscall.SIGHUP,
syscall.SIGINT,
syscall.SIGTERM,
syscall.SIGQUIT)
defer func() {
p.Shutdown()
p.Close()
}()
Copy link
Contributor

Choose a reason for hiding this comment

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

I have to admit that it is a better example. It's easier to read, it exits cleanly and doesn't have to use a signal. Note that the signal handling can be documented in the overall README or wiki (I do think it's something real users will want to want to learn about, but I now see that it doesn't have to be in the default examples).


if err := p.Setup(); err != nil {
panic(err)
time.Sleep(50 * time.Millisecond)
for i := 1; i <= 18; i++ {
if err := p.SetLEDBrightness(i, 1); err != nil {
panic(err)
}
time.Sleep(10 * time.Millisecond)
}

for {
select {
case <-sigc:
p.Shutdown()
p.Close()
return
default:
time.Sleep(50 * time.Millisecond)

for i := 1; i <= 18; i++ {
if err := p.SetLEDBrightness(i, 1); err != nil {
panic(err)
}
time.Sleep(10 * time.Millisecond)
}

time.Sleep(50 * time.Millisecond)

for i := 18; i > 0; i-- {
if err := p.SetLEDBrightness(i, 0); err != nil {
panic(err)
}
time.Sleep(10 * time.Millisecond)
}
time.Sleep(50 * time.Millisecond)
for i := 18; i > 0; i-- {
if err := p.SetLEDBrightness(i, 0); err != nil {
panic(err)
}
time.Sleep(10 * time.Millisecond)
}
}
65 changes: 7 additions & 58 deletions piglow/piglow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,7 @@ type PiGlow struct {

// Reset resets the internal registers
func (p *PiGlow) Reset() error {
if err := p.conn.Write([]byte{0x17, 0xFF}); err != nil {
return err
}

return nil
return p.conn.Write([]byte{0x17, 0xFF})
}

// Shutdown sets the software shutdown mode of the PiGlow
Expand All @@ -40,23 +36,18 @@ func (p *PiGlow) Setup() error {
if err := p.Reset(); err != nil {
return err
}

if err := p.Enable(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

if Setup stays exported (see #26) I would suggest to rename it. To me this is almost the real Reset. I don't know what the 3 registers do (doc would have been nice) but it sounds like Setup would be called the first time around or every time we want a clean slate, it does sound like a reset.

Copy link
Member

Choose a reason for hiding this comment

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

How would you feel about removing this part from the PR while we discuss the proper way it should be called/named.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

return err
}

if err := p.SetLEDControlRegister(1, 0xFF); err != nil {
return err
}

if err := p.SetLEDControlRegister(2, 0xFF); err != nil {
return err
}

if err := p.SetLEDControlRegister(3, 0xFF); err != nil {
return err
}

return nil
}

Expand All @@ -68,8 +59,8 @@ func Open(o driver.Opener) (*PiGlow, error) {
if err != nil {
return nil, err
}

return &PiGlow{conn: conn}, nil
g := &PiGlow{conn: conn}
return g, nil
}

// Close frees the underlying resources. It must be called once
Expand All @@ -83,19 +74,15 @@ func (p *PiGlow) Green(level int) error {
if err := p.conn.Write([]byte{0x04, byte(level)}); err != nil {
return err
}

if err := p.conn.Write([]byte{0x06, byte(level)}); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am probably nitpicking but I would love to see these bytes being defined somewhere so their names can indicate what they are used for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding comments nearby what they are doing would be good to have. @zankich?

Copy link
Member

Choose a reason for hiding this comment

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

I can add constants for the registers which are being written to.

return err
}

if err := p.conn.Write([]byte{0x0E, byte(level)}); err != nil {
return err
}

if err := p.conn.Write([]byte{0x16, 0xFF}); err != nil {
return err
}

return nil
}

Expand All @@ -104,19 +91,15 @@ func (p *PiGlow) Blue(level int) error {
if err := p.conn.Write([]byte{0x05, byte(level)}); err != nil {
return err
}

if err := p.conn.Write([]byte{0x0C, byte(level)}); err != nil {
return err
}

if err := p.conn.Write([]byte{0x0F, byte(level)}); err != nil {
return err
}

if err := p.conn.Write([]byte{0x16, 0xFF}); err != nil {
return err
}

return nil
}

Expand All @@ -125,19 +108,15 @@ func (p *PiGlow) Yellow(level int) error {
if err := p.conn.Write([]byte{0x03, byte(level)}); err != nil {
return err
}

if err := p.conn.Write([]byte{0x09, byte(level)}); err != nil {
return err
}

if err := p.conn.Write([]byte{0x10, byte(level)}); err != nil {
return err
}

if err := p.conn.Write([]byte{0x16, 0xFF}); err != nil {
return err
}

return nil
}

Expand All @@ -146,19 +125,15 @@ func (p *PiGlow) Orange(level int) error {
if err := p.conn.Write([]byte{0x02, byte(level)}); err != nil {
return err
}

if err := p.conn.Write([]byte{0x08, byte(level)}); err != nil {
return err
}

if err := p.conn.Write([]byte{0x11, byte(level)}); err != nil {
return err
}

if err := p.conn.Write([]byte{0x16, 0xFF}); err != nil {
return err
}

return nil
}

Expand All @@ -167,19 +142,15 @@ func (p *PiGlow) White(level int) error {
if err := p.conn.Write([]byte{0x0A, byte(level)}); err != nil {
return err
}

if err := p.conn.Write([]byte{0x0B, byte(level)}); err != nil {
return err
}

if err := p.conn.Write([]byte{0x0D, byte(level)}); err != nil {
return err
}

if err := p.conn.Write([]byte{0x16, 0xFF}); err != nil {
return err
}

return nil
}

Expand All @@ -188,20 +159,13 @@ func (p *PiGlow) Red(level int) error {
if err := p.conn.Write([]byte{0x01, byte(level)}); err != nil {
return err
}

if err := p.conn.Write([]byte{0x07, byte(level)}); err != nil {
return err
}

if err := p.conn.Write([]byte{0x12, byte(level)}); err != nil {
return err
}

if err := p.conn.Write([]byte{0x16, 0xFF}); err != nil {
return err
}

return nil
return p.conn.Write([]byte{0x16, 0xFF})
}

// SetLEDControlRegister sets the control register 1-3 to the bitmask enables.
Expand All @@ -228,25 +192,15 @@ func (p *PiGlow) SetLEDControlRegister(register, enables int) error {
if err := p.conn.Write([]byte{address, byte(enables)}); err != nil {
return err
}

if err := p.conn.Write([]byte{0x16, 0xFF}); err != nil {
return err
}

return nil
return p.conn.Write([]byte{0x16, 0xFF})
}

// SetLEDBrightness sets the led 1-18 to the level 0-255.
func (p *PiGlow) SetLEDBrightness(led, level int) error {
if err := p.conn.Write([]byte{byte(led), byte(level)}); err != nil {
return err
}

if err := p.conn.Write([]byte{0x16, 0xFF}); err != nil {
return err
}

return nil
return p.conn.Write([]byte{0x16, 0xFF})
}

// SetBrightness sets all the LEDs to the level 0-255.
Expand All @@ -256,10 +210,5 @@ func (p *PiGlow) SetBrightness(level int) error {
return err
}
}

if err := p.conn.Write([]byte{0x16, 0xFF}); err != nil {
return err
}

return nil
return p.conn.Write([]byte{0x16, 0xFF})
}
Loading