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

Feature tam control #4

Closed
wants to merge 5 commits into from
Closed

Conversation

coamitch
Copy link
Contributor

@coamitch coamitch commented Mar 1, 2024

Changes Made

Please provide a description of all changes made in this PR and why the changes
are needed.

Associated Issues

Please provide a list of all open issues that this PR will close or contribute
toward closing.

  • Fixes # (issue)

Testing

Please provide a clear and concise description of the testing performed.

@evan-palmer evan-palmer self-requested a review March 1, 2024 04:44
@EverardoG EverardoG force-pushed the feature-tam-control branch from e85c5fa to 3f12844 Compare March 1, 2024 04:50
@coamitch coamitch closed this Mar 1, 2024
@@ -6,7 +6,83 @@
"*.srdf": "xml",
"*.rviz": "yaml",
"*.config": "xml",
"*.sdf": "xml"
"*.sdf": "xml",
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need these

Copy link
Collaborator

Choose a reason for hiding this comment

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

Outdated because of the rebase?

thruster_allocation_matrix_controller/CMakeLists.txt Outdated Show resolved Hide resolved
return controller_interface::CallbackReturn::SUCCESS;
}

void ThrusterAllocationMatrixController::reference_callback(std::shared_ptr<geometry_msgs::msg::Wrench> msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need this. See the relevant comment in the lambda

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure. Same comment as the other relevant comment.


} // namespace

ThrusterAllocationMatrixController::CallbackReturn ThrusterAllocationMatrixController::on_init()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why aren't you using update_parameters here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure what you mean.

command_interfaces_[i].set_value(thruster_forces[i]);
}

// TODO: we still aren't sure if this is the correct type/way of doing this. we may need to come back and do
Copy link
Contributor

Choose a reason for hiding this comment

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

Add this back in before you submit the PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

Noted.


// otherwise, they are all the same length and we can pack them into an Eigen matrix
Eigen::MatrixXd tam_(dof_, num_thrusters_);
tam_ << tam_x, tam_y, tam_z, tam_rx, tam_ry, tam_rz;
Copy link
Contributor

Choose a reason for hiding this comment

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

Check this in your debugging. I think Eigen is column major so things might be inserted weird.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, will do.

@evan-palmer evan-palmer self-requested a review March 1, 2024 05:53
@evan-palmer evan-palmer closed this Mar 7, 2024
@evan-palmer evan-palmer deleted the feature-tam-control branch March 13, 2024 09:26
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.

4 participants