Skip to content
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

MultibodyPlant should check Context validity #11252

Closed
avalenzu opened this issue Apr 16, 2019 · 14 comments · Fixed by #15129 or #15646
Closed

MultibodyPlant should check Context validity #11252

avalenzu opened this issue Apr 16, 2019 · 14 comments · Fixed by #15129 or #15646

Comments

@avalenzu
Copy link
Contributor

Passing a diagram context into Frame::CalcPose() results in the following assertion:

abort: Failure at bazel-out/k8-dbg/bin/external/drake/systems/framework/_virtual_includes/cache_and_dependency_tracker/drake/systems/framework/cache.h:702 in get_cache_entry_value():
condition 'has_cache_entry_value(index)' failed.

I spent a fair few minutes wondering what had gone wrong in my diagram before I realized that the issue was a context mismatch. If the assertion had come from a MultibodyPlant method this would have been more apparent. We could add

DRAKE_ASSERT_VOID(System<double>::CheckValidContext(context));

in user-facing non-systems-framework methods of MultibodyPlant.

@amcastro-tri
Copy link
Contributor

would you prefer ASSERT (only debug) or DEMAND?

@sherm1
Copy link
Member

sherm1 commented Apr 16, 2019

Context-validating is potentially very expensive.

@avalenzu
Copy link
Contributor Author

I think ASSERT, definitely.

@amcastro-tri
Copy link
Contributor

See related issue #11801.

@amcastro-tri
Copy link
Contributor

Should this be an MBP feature or a System feature? @sherm1?

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Jan 13, 2020

The check proposed by #12564 should never be disarmed (i.e., not only in Debug builds). For the "correctly-mated System vs Context" checking:

  • System methods (like EvalTimeDerivatives) will embed the check, but
  • for MbP-specific functions like GetFreeBodyPose, the MbP must call ValidateContext, because the framework can't do it.

@amcastro-tri amcastro-tri changed the title MultibodyPlant should check Context validity in debug builds MultibodyPlant should check Context validity Jan 13, 2020
@sherm1
Copy link
Member

sherm1 commented Jan 13, 2020

Agreed. Eventually you would probably hit a framework method that would barf on the mismatch, but the delayed detection would be less helpful for users. (Would be better if the ValidateContext method would report the name of the failing API though.)

@sherm1 sherm1 added the component: multibody plant MultibodyPlant and supporting code label May 12, 2020
@jwnimmer-tri
Copy link
Collaborator

I think that #14389 resolved this.

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Mar 3, 2021

While #14389 resolved the issue for MbP's direct API, there are several other public classes on the MbT-related side that still lack calls to ValidateContxt, e.g. Frame::CalcPoseInWorld as noted in #12560 (comment).

@jwnimmer-tri jwnimmer-tri reopened this Mar 3, 2021
@amcastro-tri
Copy link
Contributor

How expensive are those ValidateContext()? can they safely be placed on release builds?
Also, I imagine we want them in MBP's public APIs, not MBT?

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Mar 4, 2021

Per its API Doc, calling ValidateContext is sufficiently fast for performance sensitive code.

PR #14389 already added the checks to all of the MbP methods.

What's still needed here is to add it to any of the multibody/tree/... methods that take a Context, e.g., Frame::CalcPoseInWorld and others.

@rpoyner-tri
Copy link
Contributor

@amcastro-tri do you have any work-in-progress on this? If not, I think i have a patch that should do the job.

@rpoyner-tri rpoyner-tri self-assigned this Jun 3, 2021
rpoyner-tri added a commit to rpoyner-tri/drake that referenced this issue Jun 3, 2021
Closes RobotLocomotion#11252.

Since all of the context-taking methods in multibody/tree lead
eventually to MultiBodyTreeSystem, ensure that all of its public methods
validate the context.
@amcastro-tri
Copy link
Contributor

Your patch is excellent. Thanks @rpoyner-tri!

EricCousineau-TRI pushed a commit that referenced this issue Jun 7, 2021
Closes #11252.

Since all of the context-taking methods in multibody/tree lead
eventually to MultiBodyTreeSystem, ensure that all of its public methods
validate the context.
@jwnimmer-tri
Copy link
Collaborator

In #15489, I found that MultibodyPlant::SetDefaultState and MultibodyPlant::SetRandomState lack validation checks on the State* in/out pointer. They call CheckValidState, but that function only does a basic "continuous xor discrete" sanity check, it doesn't inspect the SystemId tag.

@jwnimmer-tri jwnimmer-tri reopened this Aug 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment