[RSDK-802] Add generic servo driver #1585
Conversation
npmenard
added
the
safe to test
viambot
added
safe to test
viambot
added
safe to test
My instinct is that anything not actively in use or tested should be removed. If you don't plan to use that feature, it's dead code and might have bugs creep in over time, so get rid of it for now and we can add it back in if it becomes important in the future. |
viambot
added
safe to test
Agreed |
| I2CAddress *int `json:"i2c_address"` | ||
| BoardName string `json:"board_name"` | ||
| I2CName string `json:"i2c_name"` | ||
| I2CAddress *int `json:"i2c_address"` |
There was a problem hiding this comment.
Mild curiosity: why is this a pointer-to-int instead of a mere int? Doesn't that open us up to shenanigans if someone modifies the value it points at? (The correct response is probably "Alan, that's off-topic for this PR and should be discussed at a later date instead of now.")
components/servo/generic/servo.go
Outdated
| const ( | ||
| minDeg float64 = 0.0 | ||
| maxDeg float64 = 180.0 | ||
| minWidthUs uint = 500 // absolute minimum width pwm width |
There was a problem hiding this comment.
Comment feels like it has too many "widths" in it. Would "absolute minimum PWM width" still make sense?
| // MinDeg minimum angle the servo can reach, note this doesn't affect PWM calculation | ||
| MinDeg *float64 `json:"min_angle_deg,omitempty"` | ||
| // MaxDeg maximum angle the servo can reach, note this doesn't affect PWM calculation | ||
| MaxDeg *float64 `json:"max_angle_deg,omitempty"` |
There was a problem hiding this comment.
Same question here re: storing references to our data instead of our data itself. Why have a *float64 instead of a float64 itself? What happens if someone else changes our underlying data?
There was a problem hiding this comment.
It's a neat trick to avoid defining default value, for example if it wasn't a pointer and a user didn't reference this field it would be initialized to 0.0 by default. Now for minDeg it's fine but for max we have an issue :) Also we consume the config struct and never reference it again, and we can reasonably assume that nothing is going to change anything while we read it
components/servo/generic/servo.go
Outdated
| period := 1.0 / float64(frequency) // dutyCycle in s | ||
| pwmWidthus := pct * period * 1000 * 1000 | ||
| rSpan := maxDeg - minDeg // servo moves from 0 to 180 deg | ||
| lSpan := float64(maxUs - minUs) // pulse width between 1ms to 2ms to s |
There was a problem hiding this comment.
Same suggestions here re: comments not matching code and variable names being confusing. Maybe degRange and usRange instead?
components/servo/generic/servo.go
Outdated
| pwmWidthus = float64(minUs) | ||
| } | ||
| if pwmWidthus > float64(maxUs) { | ||
| pwmWidthus = float64(maxUs) |
There was a problem hiding this comment.
Mild preference for
pwmWidthus = math.max(pwmWidthus, minUs)
pwmWidthus = math.min(pwmWidthus, maxUs)
which I suspect is more idiomatic.
components/servo/generic/servo.go
Outdated
|
|
||
| angle := math.Round(minDeg + (pwmWidthus-float64(minUs))*scale) | ||
|
|
||
| return angle |
There was a problem hiding this comment.
Slight preference to just return on line 217, rather than define a variable there that just gets returned here. but I'm open to disagreement.
components/servo/generic/servo.go
Outdated
| return angle | ||
| } | ||
|
|
||
| // Attempt to find the PWM resolution assuming an hardware PWM |
There was a problem hiding this comment.
Typo: "assuming a hardware PWM"
components/servo/generic/servo.go
Outdated
|
|
||
| // Attempt to find the PWM resolution assuming an hardware PWM | ||
| // Assumption : | ||
| // 1. 16,15,14,12,or 8 bit timer |
There was a problem hiding this comment.
I have no idea what this means. Was this line supposed to be removed before the PR went out? If not, put in more words to explain it better.
viambot
added
safe to test
viambot
added
safe to test
There was a problem hiding this comment.
A few minor nitpicks here and there.
The 'pi' servo holdPos was added for hackweek where the servos were burning themselves out because they were not zeroed before being assembled with the yahbooms. It dictates a timeout to stop the servo from continuously trying to get to a position, it is not needed in generic servo at all. We can remove it at a later date, I don't think it's important now and it is kinder to those $1 servos. I'd just keep it in there until we need to break it.
viambot
added
safe to test
|
This is an implementation of a generic servo driver. This is supposed to work with any GPIO pins implementing PWM, to properly scale the movement of the servo we need to have an understanding of the underlying hardware resolution to do so we have a search function that estimate the underlying pwm timer resolution.
I have an holdPos attribute in the config (to mimick the pi implementation) in the pi implementation if set to false it would stop the pwm signal (servo torque to 0) after a certain time. I chose not to implement it since I can't think of a reasonable case where we would want to the servo torque to be 0. I am tempted to remove it altogether but would like other people's input !