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

Add a first shell of the agimus_controller API #97

Conversation

MaximilienNaveau
Copy link
Collaborator

This is a first trial and discussion room about the agimus_controller new API.
We want to have a generic implementation that can cover most usage of the OCP/MPC/WarmStart combinations.
The design pattern is this one.
1000024250

agimus_controller/agimus_controller/mpc.py Outdated Show resolved Hide resolved
agimus_controller/agimus_controller/mpc.py Outdated Show resolved Hide resolved
agimus_controller/agimus_controller/ocp_factory.py Outdated Show resolved Hide resolved
agimus_controller/agimus_controller/trajectory.py Outdated Show resolved Hide resolved
agimus_controller/agimus_controller/trajectory.py Outdated Show resolved Hide resolved
agimus_controller/agimus_controller/warm_start_factory.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@ArthurH91 ArthurH91 left a comment

Choose a reason for hiding this comment

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

great work @MaximilienNaveau thank you for this! I put my comments, that are mostly questions (sorry for this) and some suggestions concerning collision avoidance

ArthurH91
ArthurH91 previously approved these changes Dec 2, 2024
@ArthurH91 ArthurH91 dismissed their stale review December 2, 2024 16:25

missclick

Copy link
Contributor

@Kotochleb Kotochleb left a comment

Choose a reason for hiding this comment

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

Could you please add deprecated folder to .gitignore and remove the unrelated changes from this PR to make it cleaner?

Other than that the design seems ok

agimus_controller/setup.py Outdated Show resolved Hide resolved
agimus_controller/setup.py Show resolved Hide resolved
agimus_controller/agimus_controller/mpc_output.py Outdated Show resolved Hide resolved
agimus_controller/agimus_controller/mpc.py Show resolved Hide resolved
agimus_controller/agimus_controller/mpc_output.py Outdated Show resolved Hide resolved
agimus_controller/agimus_controller/trajectory.py Outdated Show resolved Hide resolved
agimus_controller/agimus_controller/ocp_base.py Outdated Show resolved Hide resolved
agimus_controller/agimus_controller/ocp_base.py Outdated Show resolved Hide resolved
agimus_controller/agimus_controller/mpc_output.py Outdated Show resolved Hide resolved
agimus_controller/agimus_controller/ocp_base.py Outdated Show resolved Hide resolved
agimus_controller/agimus_controller/mpc.py Outdated Show resolved Hide resolved
agimus_controller/agimus_controller/mpc.py Outdated Show resolved Hide resolved
agimus_controller/agimus_controller/mpc_output.py Outdated Show resolved Hide resolved
agimus_controller/agimus_controller/mpc.py Outdated Show resolved Hide resolved
agimus_controller/agimus_controller/mpc.py Outdated Show resolved Hide resolved
agimus_controller/agimus_controller/mpc.py Show resolved Hide resolved
@MaximilienNaveau MaximilienNaveau force-pushed the topic/humble-devel/refactor branch from 7dc2196 to c4ef2dc Compare December 6, 2024 16:24
@MaximilienNaveau
Copy link
Collaborator Author

MaximilienNaveau commented Dec 6, 2024

I believe we just need to get clear on the warm start API.
I try to follow all your comments.
Sometimes it's hard.
I greatly appreciate your patience!!!

Just to be clear I do not run the code so when we will add unit-test this code might simply break due to typos in the python standard.

Copy link
Contributor

@Kotochleb Kotochleb left a comment

Choose a reason for hiding this comment

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

Imports are still not in alphabetical order

agimus_controller/agimus_controller/mpc.py Outdated Show resolved Hide resolved
@MaximilienNaveau
Copy link
Collaborator Author

Imports are still not in alphabetical order

Now it's done in principle 👍

Copy link
Contributor

@kzorina kzorina left a comment

Choose a reason for hiding this comment

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

There are still some minor disagreements, but I guess we can fix in the next pull requests.
The skeleton looks good to me!

@MaximilienNaveau MaximilienNaveau force-pushed the topic/humble-devel/refactor branch from 1825eda to 637222c Compare December 9, 2024 13:50
@MaximilienNaveau MaximilienNaveau merged commit fe1ef5c into agimus-project:topic/humble-devel/refactor Dec 9, 2024
1 check passed
@MaximilienNaveau MaximilienNaveau deleted the topic/humble-devel/refactor branch December 9, 2024 14:22
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.

7 participants