Skip to content

Commit

Permalink
[RSDK-8494] Validate PWM-setting requests (viamrobotics#4309)
Browse files Browse the repository at this point in the history
The Jira ticket was specifically for the pinctrl module, but I think it should be generalized to all boards. So, here we go.

Thanks to John for the suggestion of calling "slightly over 1.0" a non-error value! If you want a different threshold for "slightly over," I'm happy to change it.

The rpi4 doesn't validate that the frequency is positive because apparently setting it to 0 already sets it to the default of 800 Hz, and it's a uint so can't be negative.

I haven't tested on actual hardware, but the new unit tests pass. I'm willing to test on hardware if you think it's important.
  • Loading branch information
penguinland committed Aug 21, 2024
1 parent 69e97dc commit 0a078d5
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 1 deletion.
10 changes: 10 additions & 0 deletions components/board/genericlinux/gpio.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/pkg/errors"
"go.viam.com/utils"

"go.viam.com/rdk/components/board"
"go.viam.com/rdk/logging"
rdkutils "go.viam.com/rdk/utils"
)
Expand Down Expand Up @@ -358,6 +359,11 @@ func (pin *gpioPin) SetPWM(ctx context.Context, dutyCyclePct float64, extra map[
pin.mu.Lock()
defer pin.mu.Unlock()

dutyCyclePct, err := board.ValidatePWMDutyCycle(dutyCyclePct)
if err != nil {
return err
}

pin.pwmDutyCyclePct = dutyCyclePct
return pin.startSoftwarePWM()
}
Expand All @@ -375,6 +381,10 @@ func (pin *gpioPin) SetPWMFreq(ctx context.Context, freqHz uint, extra map[strin
pin.mu.Lock()
defer pin.mu.Unlock()

if freqHz < 1 {
return errors.New("must set PWM frequency to a positive value")
}

pin.pwmFreqHz = freqHz
return pin.startSoftwarePWM()
}
Expand Down
23 changes: 22 additions & 1 deletion components/board/gpio_pin.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
package board

import "context"
import (
"context"
"fmt"

"github.com/pkg/errors"
)

// A GPIOPin represents an individual GPIO pin on a board.
//
Expand Down Expand Up @@ -83,3 +88,19 @@ type GPIOPin interface {
// 0 will use a default PWM frequency of 800.
SetPWMFreq(ctx context.Context, freqHz uint, extra map[string]interface{}) error
}

// ValidatePWMDutyCycle makes sure the value passed in is a believable duty cycle value. It returns
// the preferred duty cycle value if it is, and an error if it is not.
func ValidatePWMDutyCycle(dutyCyclePct float64) (float64, error) {
if dutyCyclePct < 0.0 {
return 0.0, errors.New("cannot set negative duty cycle")
}
if dutyCyclePct > 1.0 {
if dutyCyclePct < 1.01 {
// Someone was probably setting it to 1 and had a roundoff error.
return 1.0, nil
}
return 0.0, fmt.Errorf("cannot set duty cycle to %f: range is 0.0 to 1.0", dutyCyclePct)
}
return dutyCyclePct, nil
}
29 changes: 29 additions & 0 deletions components/board/gpio_pin_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package board_test

import (
"testing"

"go.viam.com/test"

"go.viam.com/rdk/components/board"
)

func TestValidatePWMDutyCycle(t *testing.T) {
// Normal values are unchanged
val, err := board.ValidatePWMDutyCycle(0.5)
test.That(t, err, test.ShouldBeNil)
test.That(t, val, test.ShouldEqual, 0.5)

// No negative values
_, err = board.ValidatePWMDutyCycle(-1.0)
test.That(t, err, test.ShouldNotBeNil)

// Values slightly over 100% get rounded down
val, err = board.ValidatePWMDutyCycle(1.005)
test.That(t, err, test.ShouldBeNil)
test.That(t, val, test.ShouldEqual, 1.0)

// No values well over 100%
_, err = board.ValidatePWMDutyCycle(2.0)
test.That(t, err, test.ShouldNotBeNil)
}
9 changes: 9 additions & 0 deletions components/board/hat/pca9685/pca9685.go
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,11 @@ func (gp *gpioPin) SetPWM(ctx context.Context, dutyCyclePct float64, extra map[s
gp.pca.mu.RLock()
defer gp.pca.mu.RUnlock()

dutyCyclePct, err := board.ValidatePWMDutyCycle(dutyCyclePct)
if err != nil {
return err
}

dutyCycle := uint16(dutyCyclePct * float64(0xffff))

handle, err := gp.pca.openHandle()
Expand Down Expand Up @@ -408,5 +413,9 @@ func (gp *gpioPin) SetPWMFreq(ctx context.Context, freqHz uint, extra map[string
gp.pca.mu.RLock()
defer gp.pca.mu.RUnlock()

if freqHz < 1 {
return errors.New("must set PWM frequency to a positive value")
}

return gp.pca.SetFrequency(ctx, float64(freqHz))
}
6 changes: 6 additions & 0 deletions components/board/pi/impl/board.go
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,12 @@ func (pi *piPigpio) pwmBcom(bcom int) (float64, error) {
func (pi *piPigpio) SetPWMBcom(bcom int, dutyCyclePct float64) error {
pi.mu.Lock()
defer pi.mu.Unlock()

dutyCyclePct, err := board.ValidatePWMDutyCycle(dutyCyclePct)
if err != nil {
return err
}

dutyCycle := rdkutils.ScaleByPct(255, dutyCyclePct)
pi.duty = int(C.gpioPWM(C.uint(bcom), C.uint(dutyCycle)))
if pi.duty != 0 {
Expand Down

0 comments on commit 0a078d5

Please sign in to comment.