-
Notifications
You must be signed in to change notification settings - Fork 113
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.
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
RSDK-7466 - Change navigation config to include bounding regions #3971
Merged
+230
−7
Merged
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
d848054
update navigation service config to include bounding regions
nfranczak b8baf40
add TestNavSetUpFromFaultyConfig to explicitly test for errGeomWithTr…
nfranczak 076c77d
update description for bounding_regions
nfranczak bb71885
add faulty config
nfranczak 3d58929
fix TestValidateGeometry
nfranczak 705833e
update tests to more rigously check for where we fail - in the valida…
nfranczak 3935292
udpated json cfgs
nfranczak d11b401
wrap error + explcitly test for string contains in returned error
nfranczak 2c75d8a
check for string in error check
nfranczak de41ee3
remove print ln
nfranczak 65a28fd
linted
nfranczak 2fd1b87
give more specific errors
nfranczak 1d0ace6
final version of checking for errors
nfranczak 68a0a39
remove lll
nfranczak 476f078
Trigger Build
nfranczak d0ab5b9
Merge branch 'main' of github.com:viamrobotics/rdk into RSDK-7466
nfranczak 2a721e6
remove Print.ln
nfranczak 5590ade
resolve lll
nfranczak File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,7 @@ var ( | |
errNegativeObstaclePollingFrequencyHz = errors.New("obstacle_polling_frequency_hz must be non-negative if set") | ||
errNegativePlanDeviationM = errors.New("plan_deviation_m must be non-negative if set") | ||
errNegativeReplanCostFactor = errors.New("replan_cost_factor must be non-negative if set") | ||
errGeomWithTranslation = errors.New("geometries specified through navigation are not allowed to have a translation") | ||
) | ||
|
||
const ( | ||
|
@@ -99,6 +100,7 @@ type Config struct { | |
MetersPerSec float64 `json:"meters_per_sec,omitempty"` | ||
|
||
Obstacles []*spatialmath.GeoGeometryConfig `json:"obstacles,omitempty"` | ||
BoundingRegions []*spatialmath.GeoGeometryConfig `json:"bounding_regions,omitempty"` | ||
PositionPollingFrequencyHz float64 `json:"position_polling_frequency_hz,omitempty"` | ||
ObstaclePollingFrequencyHz float64 `json:"obstacle_polling_frequency_hz,omitempty"` | ||
PlanDeviationM float64 `json:"plan_deviation_m,omitempty"` | ||
|
@@ -181,7 +183,16 @@ func (conf *Config) Validate(path string) ([]string, error) { | |
for _, obs := range conf.Obstacles { | ||
for _, geoms := range obs.Geometries { | ||
if !geoms.TranslationOffset.ApproxEqual(r3.Vector{}) { | ||
return nil, errors.New("geometries specified through the navigation are not allowed to have a translation") | ||
return nil, errGeomWithTranslation | ||
} | ||
} | ||
} | ||
|
||
// Ensure bounding regions have no translation | ||
for _, region := range conf.BoundingRegions { | ||
for _, geoms := range region.Geometries { | ||
if !geoms.TranslationOffset.ApproxEqual(r3.Vector{}) { | ||
return nil, errGeomWithTranslation | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a test for this new validation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will add this as a unit test. |
||
} | ||
} | ||
} | ||
|
@@ -225,6 +236,7 @@ type builtIn struct { | |
// exploreMotionService will be removed once the motion explore model is integrated into motion builtin | ||
exploreMotionService motion.Service | ||
obstacles []*spatialmath.GeoGeometry | ||
boundingRegions []*spatialmath.GeoGeometry | ||
|
||
motionCfg *motion.MotionConfiguration | ||
replanCostFactor float64 | ||
|
@@ -368,6 +380,12 @@ func (svc *builtIn) Reconfigure(ctx context.Context, deps resource.Dependencies, | |
return err | ||
} | ||
|
||
// Parse bounding regions from the configuration | ||
newBoundingRegions, err := spatialmath.GeoGeometriesFromConfigs(svcConfig.Obstacles) | ||
if err != nil { | ||
return err | ||
} | ||
nicksanford marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Create explore motion service | ||
// Note: this service will disappear after the explore motion model is integrated into builtIn | ||
exploreMotionConf := resource.Config{ConvertedAttributes: &explore.Config{}} | ||
|
@@ -381,6 +399,7 @@ func (svc *builtIn) Reconfigure(ctx context.Context, deps resource.Dependencies, | |
svc.mapType = mapType | ||
svc.motionService = motionSvc | ||
svc.obstacles = newObstacles | ||
svc.boundingRegions = newBoundingRegions | ||
svc.replanCostFactor = replanCostFactor | ||
svc.visionServicesByName = visionServicesByName | ||
svc.motionCfg = &motion.MotionConfiguration{ | ||
|
@@ -524,6 +543,7 @@ func (svc *builtIn) moveToWaypoint(ctx context.Context, wp navigation.Waypoint, | |
MovementSensorName: svc.movementSensor.Name(), | ||
Obstacles: svc.obstacles, | ||
MotionCfg: svc.motionCfg, | ||
BoundingRegions: svc.boundingRegions, | ||
Extra: extra, | ||
} | ||
cancelCtx, cancelFn := context.WithCancel(ctx) | ||
|
This file contains 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
This file contains 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this to be with the obstacles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bounding region is not an obstacle...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicksanford definition taken from API
https://github.com/viamrobotics/api/blob/main/proto/viam/service/motion/v1/motion.proto#L166
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nick is right, we should remove the word "obstacle" when referring to these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, what do we think of:
Is it worth updating the API to match the new definition we are putting forward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets change
bounds
above togeometries
. And yeah lets make the 1 line change to the API too. We probably need to clean up the comments inmotion.proto
more but this would be a good start