Skip to content

Fix position limits oink constraint for models with continuous joints#147

Merged
sjahr merged 11 commits intomainfrom
fix-position-limit
Mar 3, 2026
Merged

Fix position limits oink constraint for models with continuous joints#147
sjahr merged 11 commits intomainfrom
fix-position-limit

Conversation

@sea-bass
Copy link
Collaborator

@sea-bass sea-bass commented Feb 21, 2026

Oink was going unstable with the Kinova model because using the Pinocchio model's joint limits fields actually considers continuous joints as [cos(theta), sin(theta)], meaning there are 2 elements both with limits +/- 1.0.

This properly uses the RoboPlan Scene's joint limits info, as well as the collapseContinuousJointPositions() utility, to handle continuous joints. Kinova example is now numerically stable by using correct position limits.

Screencast.from.2026-02-21.15-25-17.webm

@sea-bass sea-bass requested a review from sjahr February 21, 2026 20:21
}
const auto& q_collapsed = maybe_q_collapsed.value();

// Get joint limits from the model (only do this once)
Copy link
Collaborator Author

@sea-bass sea-bass Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This highlights a need to possibly have tasks accept the scene in their constructor so that information can be pre-allocated at construction time.

The same thing happens with FrameTask re: getting the frame_id value for the joint.

Although, I do like the solver doing its own thing so that the scene usage is consistent. Maybe then the best approach is to have a setup() method in these tasks/constraints that is automatically called by the solver? Or at least we can just call solver.setup() and that invokes it for all the tasks/constraints/barriers?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I opened an issue to have to track this as additional work. #152

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already captured that in #148 I think!

@sea-bass sea-bass changed the title Fix position limits oink task for models with continuous joints Fix position limits oink constraint for models with continuous joints Feb 21, 2026
Base automatically changed from oink-pass-again to main February 26, 2026 19:23
Copy link
Collaborator

@sjahr sjahr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it and it works! One bigger comment & independent of that, do you mind adding 1-2 tests for the new functionality?

}
const auto& q_collapsed = maybe_q_collapsed.value();

// Get joint limits from the model (only do this once)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I opened an issue to have to track this as additional work. #152

@sjahr
Copy link
Collaborator

sjahr commented Mar 1, 2026

@sea-bass Looks like the last commit broke something

[OsqpEigen::Settings::setTimeLimit] OSPQ has been set without PROFILING, hence this setting is disabled.
ERROR in validate_data: Lower bound at index 21 is greater than upper bound: 5.0000e-03 > -5.0000e-03
ERROR in osqp_setup: Problem data validation.
[OsqpEigen::Solver::initSolver] Unable to setup the workspace.
Warning: IK solver failed: IK solve failed: Failed to initialize solver, using zero delta_q

I just quickly tested it and got this error. Does it happen to you as well? The commit before seems to work. Do you mind taking a look into this? Not sure why the test isn't catching it.

@sea-bass
Copy link
Collaborator Author

sea-bass commented Mar 1, 2026

Ah I think I know what this is. The max velocity/accel/jerk has to be multiplied by the absolute value of the scaling, so in the negative case it gets messed up.

Will fix and test when home.

@sea-bass
Copy link
Collaborator Author

sea-bass commented Mar 1, 2026

Ah I think I know what this is. The max velocity/accel/jerk has to be multiplied by the absolute value of the scaling, so in the negative case it gets messed up.

Will fix and test when home.

This was indeed it. Fixed, thank you!

Copy link
Collaborator

@sjahr sjahr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jap, that fixed it! Nice work 🎉

@sjahr sjahr merged commit 38eea96 into main Mar 3, 2026
9 of 10 checks passed
@sjahr sjahr deleted the fix-position-limit branch March 3, 2026 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants