RSDK-628 - allow configurable kinematics for fake arm models #1432
Conversation
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
|
|
spieswl
approved these changes
Oct 5, 2022
| @@ -145,19 +146,19 @@ func TestFrameSystemFromConfig(t *testing.T) { | |||
|
|
|||
| // There is a point at (1500, 500, 1300) in the world referenceframe. See if it transforms correctly in each referenceframe. | |||
| worldPose := referenceframe.NewPoseInFrame(referenceframe.World, spatialmath.NewPoseFromPoint(r3.Vector{1500, 500, 1300})) | |||
| armPt := r3.Vector{0, 0, 500} | |||
| armPt := r3.Vector{500, 0, 0} | |||
There was a problem hiding this comment.
Is there a place we can check where these values are coming from?
There was a problem hiding this comment.
Unfortunately not really, this was all hand calculated. But you can see the kinematic information in fake_model.json. Does this answer your question?
| return fakearm.NewArmIK(ctx, config.Component{Name: "arm"}, logger) | ||
| return fakearm.NewArm(ctx, config.Component{ | ||
| Name: arm.Subtype.String(), | ||
| ConvertedAttributes: &fakearm.AttrConfig{ArmModel: xarm.ModelName(6)}, |
There was a problem hiding this comment.
I assume this change is so that this test can use the kinematic model of the xArm6?
There was a problem hiding this comment.
Yes, before it was using another fake kinematic model, which no longer exists
biotinker
requested changes
Oct 5, 2022
| func NewArm(ctx context.Context, cfg config.Component, logger golog.Logger) (arm.LocalArm, error) { | ||
| var model referenceframe.Model | ||
| var err error | ||
| if cfg.ConvertedAttributes != nil { |
There was a problem hiding this comment.
Probably we should also allow the use of arbitrary files similar to wrapper
There was a problem hiding this comment.
yes probably, but this is work that I don't think is necessary now, so I would like to push it to another PR
biotinker
reviewed
Oct 10, 2022
biotinker
requested changes
Oct 10, 2022
|
|
||
| // AttrConfig is used for converting config attributes. | ||
| type AttrConfig struct { | ||
| FailNew bool `json:"fail_new"` |
There was a problem hiding this comment.
I think this should also be removed, no?
There was a problem hiding this comment.
This is still being used in some tests, so I wasn't inclined to take it out. Thoughts?
| // Validate ensures all parts of the config are valid. | ||
| func (config *AttrConfig) Validate(path string) error { | ||
| if config.FailNew { | ||
| return errors.New("whoops") |
|
Closed
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.
Currently there is no way to specify what kinematic model a fake arm which makes it difficult to test motion planning for arms that we are not connected to. Additionally, the fake and fakeIK models both exist in the codebase which is confusing. This PR addresses both these concerns by wrapping fakeIK model functionality into the fake model, and further makes the kinematic model for the fake model configurable.