Complaint if Joint doesn't exists before adding joint controller

[cjds]

🎉 New feature

Closes : # Sorry haven't written an issue yet

Summary

• Changed the JointController class to inform user when the joint trying to be controlled doesn't exist on the model
• Changed it similarly to JointStatePublisher to show similar error message1

Test it

Tested this locally by trying to control joints that didn't exist in a model

Checklist

• Signed all commits for DCO
• Added tests
• Added example and/or tutorial
• Updated documentation (as needed)
• Updated migration guide (as needed)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

[chapulina]

There's some logic on PreUpdate to continuously look for the joint if it doesn't exist yet. This suggests to me that in some use cases, the joint may not be present on the model yet when Cofigure is called, but it may show up later:

https://github.com/ignitionrobotics/ign-gazebo/blob/8804138a7637150786e9618c851066d8cce6ab05/src/systems/joint_controller/JointController.cc#L191-L199

So I think that we should do one of the 2:

1. If there's no way the joint can show up later, we should remove the checks in PreUpdate, get this->jointEntity in Configure and return on failure.
2. If there's a way a joint can show up later, we shouldn't print an error at Configure.

I believe that 1. covers the usual use case. Do you remember why those checks were added in the first place, @azeey ?

[cjds]

@chapulina I agree with 1. but not necessarily 2. Maybe we can print a warning instead? Whole reason I made the PR was I spent the day wondering why my joint wouldn't actuate only to find out it was a typo

[chapulina]

Whole reason I made the PR was I spent the day wondering why my joint wouldn't actuate only to find out it was a typo

Ouch yeah trying infinitely silently is bad. Let's see what @azeey says, I think we could keep your error message and return right after it.

[azeey]

There's some logic on PreUpdate to continuously look for the joint if it doesn't exist yet. This suggests to me that in some use cases, the joint may not be present on the model yet when Cofigure is called, but it may show up later:

hmm, I don't remember exactly why I added that check. Maybe I misunderstood how plugins are loaded. My current understanding is that for a given model, all the entities in the model are created before the plugin for that model is loaded. Waiting for entities is required only if that entity belongs to another model. So, option 1 would work, with the modification that we set a flag (eg. validConfig = false), so that when PreUpdate is subsequently called, we can return early by checking that flag.

[cjds]

Okay I think I understand that change. I'm going to go ahead and try to change my PR to be that instead.

Thanks for the input

[ahcorde]

friendly ping @cjds

[ahcorde]

another friendly ping @cjds

[cjds]

Sorry @ahcorde completely forgot about making changes. Just made the required changes.

[cjds]

@ahcorde I don't think I quite understand the code well enough to understand why the tests are failing.

I think it might be related to this line but maybe you could explain it to me
https://github.com/ignitionrobotics/ign-gazebo/blob/ign-gazebo5/test/integration/joint_controller_system.cc#L99

[chapulina]

I pushed some updates in e128aab:

• Removed validConfig variable because we can check if the system is valid by checking the jointEntity
• Demoted jointName to a local variable now that it's only used in one function
• Moved kNullEntity check to the top of PreUpdate
• Added a simple test to make sure nothing crashes

LGTM with happy CI