fix!: waypoint rates are optional, and survive a transmission - #226
Merged
Conversation
Two things, found together because the first is what exposed the second. `Command.Trajectory` required a velocity and an acceleration on every waypoint. Positions and times is the ordinary way to describe a trajectory — it is what ROS-style planners emit — and a caller with only that had to invent rates to satisfy the schema, which is worse than saying nothing: a driver cannot tell an invented zero from a deliberate standstill. Both are now optional, and an absent one means "you decide", which a driver can act on sensibly by working out the speed needed to arrive by `time_from_start`. Separately, and already broken: `Transmission.apply_to_waypoint/2` pattern matched a waypoint as a map, while the schema has always declared keyword lists. Every trajectory sent to a joint with a transmission raised `FunctionClauseError` and took the actuator down with it. On an SO-101 that is five joints of six; only the gripper works, because it has no transmission, which is why this survived. It now handles keyword lists, and leaves an omitted rate omitted rather than turning it into a zero. `follow_trajectory/4` no longer multiplies a nil rate by 1.0. BREAKING CHANGE: `velocity` and `acceleration` on a trajectory waypoint may now be `nil`. Anything reading them straight out of a waypoint has to cope with that; drivers that treated them as always-present need a fallback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two fixes, found together because the first exposed the second.
Waypoint rates are optional
Command.Trajectoryrequiredvelocityandaccelerationon every waypoint. Positions and times is the ordinary way to describe a trajectory — it's what ROS-style planners emit — and a caller with only that had to invent rates to satisfy the schema.Inventing is worse than omitting: a driver can't tell a made-up
0.0from a deliberate standstill. Both fields are now optional, and absent means you decide — which a driver can act on by working out the speed needed to arrive bytime_from_start.follow_trajectory/4also stopped multiplying a nil rate by1.0.Trajectories were broken on any joint with a transmission
Found while testing the above.
Transmission.apply_to_waypoint/2pattern-matched a waypoint as a map:but the schema has always declared them as keyword lists (
{:list, {:keyword_list, @waypoint_schema}}). So every trajectory sent to a joint with a transmission raisedFunctionClauseErrorand took the actuator down:On an SO-101 that's five joints of six. Only the gripper works, because it has no transmission — which is exactly why this survived unnoticed, and why I only caught it when I went looking for something else.
It now handles keyword lists, and leaves an omitted rate omitted rather than turning it into a zero. There was already a
maybe_apply_rate/2for precisely that; it just wasn't being used here.Breaking
velocityandaccelerationon a waypoint may now benil. Anything reading them straight out of a waypoint needs to cope — drivers treating them as always-present want a fallback.bb_servo_feetechalready has one: an absent velocity falls back to the leg's time budget, then to the joint's velocity limit.Testing
mix check --no-retrypasses; 1317 tests. Four new ones cover the keyword-list shape, an omitted rate surviving the transmission asnilrather than0.0,time_from_startbeing untouched, and the nil-transmission path.Came out of hardware work on an SO-101 — bb_servo_feetech#94 unified its position and trajectory code paths, at which point the fallback for an absent velocity turned out to be unreachable through the public API.