From 0a078d5fed7ecc8816b1acdc83ef6f28440ff2ad Mon Sep 17 00:00:00 2001
From: Alan Davidson <alan@viam.com>
Date: Wed, 21 Aug 2024 13:34:46 -0400
Subject: [PATCH] [RSDK-8494] Validate PWM-setting requests (#4309)

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.
---
 components/board/genericlinux/gpio.go   | 10 +++++++++
 components/board/gpio_pin.go            | 23 +++++++++++++++++++-
 components/board/gpio_pin_test.go       | 29 +++++++++++++++++++++++++
 components/board/hat/pca9685/pca9685.go |  9 ++++++++
 components/board/pi/impl/board.go       |  6 +++++
 5 files changed, 76 insertions(+), 1 deletion(-)
 create mode 100644 components/board/gpio_pin_test.go

diff --git a/components/board/genericlinux/gpio.go b/components/board/genericlinux/gpio.go
index 179914dc5de..f8aa5f09644 100644
--- a/components/board/genericlinux/gpio.go
+++ b/components/board/genericlinux/gpio.go
@@ -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"
 )
@@ -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()
 }
@@ -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()
 }
diff --git a/components/board/gpio_pin.go b/components/board/gpio_pin.go
index ccabfb6ea4b..9d53b58298c 100644
--- a/components/board/gpio_pin.go
+++ b/components/board/gpio_pin.go
@@ -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.
 //
@@ -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
+}
diff --git a/components/board/gpio_pin_test.go b/components/board/gpio_pin_test.go
new file mode 100644
index 00000000000..7a598a0a67e
--- /dev/null
+++ b/components/board/gpio_pin_test.go
@@ -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)
+}
diff --git a/components/board/hat/pca9685/pca9685.go b/components/board/hat/pca9685/pca9685.go
index bf0348baf52..7c5224b334e 100644
--- a/components/board/hat/pca9685/pca9685.go
+++ b/components/board/hat/pca9685/pca9685.go
@@ -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()
@@ -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))
 }
diff --git a/components/board/pi/impl/board.go b/components/board/pi/impl/board.go
index df9d3dfe009..2551ff2ab12 100644
--- a/components/board/pi/impl/board.go
+++ b/components/board/pi/impl/board.go
@@ -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 {