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.

Sign up for GitHub

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

Jump to bottom

[STEERING] Add missing tan call for ackermann #1117

Merged
merged 2 commits into from
Jun 19, 2024
Merged

[STEERING] Add missing tan call for ackermann #1117

merged 2 commits into from
Jun 19, 2024

Conversation

qinqon
Copy link
Contributor

@qinqon qinqon commented May 6, 2024

The steering library is missing a call to calculate tangent from steering pos at ackermann code.

@qinqon
Copy link
Contributor Author

qinqon commented May 6, 2024

@saikishor FYI

saikishor
saikishor requested changes May 6, 2024
View reviewed changes
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

@qinqon It would be great if you can also update the docs with some references to the actual math, so we can avoid this in future.

Will you be able to add some tests?

@qinqon
Copy link
Contributor Author

qinqon commented May 6, 2024 •
edited
Loading

@qinqon It would be great if you can also update the docs with some references to the actual math, so we can avoid this in future.

Do you know if the original author added some reference somewhere ?

Will you be able to add some tests?

Yep, will do that too.

@christophfroehlich
Copy link
Contributor

Regarding docs: No, the original author did not include any. Please have a look at #954 and review what I've written there.

@qinqon
Copy link
Contributor Author

qinqon commented May 7, 2024

Regarding docs: No, the original author did not include any. Please have a look at #954 and review what I've written there.

Ahh that's much needed, thanks.

@christophfroehlich someting a little off topic, I am preparing a PR to implement swerve drive re-using/refactoring steering library, does it make sense, or is better to create a new controller ?

@christophfroehlich
Copy link
Contributor

@christophfroehlich someting a little off topic, I am preparing a PR to implement swerve drive re-using/refactoring steering library, does it make sense, or is better to create a new controller ?

Please open a separate issue and propose your changes. I also had a rewrite of the steering library in mind, maybe we can find a proper way going forward. We can also discuss this on slack, I can add ellorent@redhat.com if you want?

@qinqon
Copy link
Contributor Author

qinqon commented May 7, 2024

@christophfroehlich someting a little off topic, I am preparing a PR to implement swerve drive re-using/refactoring steering library, does it make sense, or is better to create a new controller ?

Please open a separate issue and propose your changes. I also had a rewrite of the steering library in mind, maybe we can find a proper way going forward. We can also discuss this on slack, I can add ellorent@redhat.com if you want?

Can you add me with my personal email ? quique.llorente@gmail.com

@qinqon qinqon closed this Jun 8, 2024
@christophfroehlich
Copy link
Contributor

christophfroehlich commented Jun 8, 2024 •
edited
Loading

@qinqon this is still missing?

@christophfroehlich christophfroehlich added backport-humble This label should be used by maintaines only! Label triggers PR backport to ROS2 humble. backport-iron This label should be used by maintaines only! Label triggers PR backport to ROS2 Iron. labels Jun 8, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Jun 8, 2024

This pull request is in conflict. Could you fix it @qinqon?

@qinqon qinqon force-pushed the add-missing-tan-at-ackerman branch from 24b033f to b505bc6 Compare June 10, 2024 19:45
@qinqon
[STEERING] Add missing tan call for ackermann
b3a1d85 
Signed-off-by: Quique Llorente <ellorent@redhat.com>
@qinqon
Use c++ std::tan instead of tan fn
daba669 
Signed-off-by: Quique Llorente <ellorent@redhat.com>
@qinqon qinqon force-pushed the add-missing-tan-at-ackerman branch from 8b5497a to daba669 Compare June 18, 2024 12:29
@bmagyar bmagyar merged commit b81be48 into ros-controls:master Jun 19, 2024
5 of 18 checks passed
mergify bot pushed a commit that referenced this pull request Jun 19, 2024
@qinqon @mergify
[STEERING] Add missing tan call for ackermann (#1117)
e3dc53e 
(cherry picked from commit b81be48)
@mergify mergify bot mentioned this pull request Jun 19, 2024
Merged
mergify bot pushed a commit that referenced this pull request Jun 19, 2024
@qinqon @mergify
[STEERING] Add missing tan call for ackermann (#1117)
54f9ba1 
(cherry picked from commit b81be48)
@mergify mergify bot mentioned this pull request Jun 19, 2024
Merged
christophfroehlich pushed a commit that referenced this pull request Jun 19, 2024
@mergify @qinqon
[STEERING] Add missing tan call for ackermann (#1117) (#1177)
3005fea 
(cherry picked from commit b81be48)

Co-authored-by: Enrique Llorente Pastora <ellorent@redhat.com>
christophfroehlich pushed a commit that referenced this pull request Jun 19, 2024
@mergify @qinqon
[STEERING] Add missing tan call for ackermann (#1117) (#1176)
8c99191 
(cherry picked from commit b81be48)

Co-authored-by: Enrique Llorente Pastora <ellorent@redhat.com>
henrygerardmoore pushed a commit to henrygerardmoore/ros2_controllers that referenced this pull request Jul 19, 2024
@dependabot
Bump docker/build-push-action from 4 to 5 (ros-controls#1117)
49ea423
@destogl destogl mentioned this pull request Aug 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmagyar bmagyar bmagyar approved these changes

@christophfroehlich christophfroehlich christophfroehlich approved these changes

@aprotyas aprotyas Awaiting requested review from aprotyas

@destogl destogl Awaiting requested review from destogl

@erickisos erickisos Awaiting requested review from erickisos

@mcbed mcbed Awaiting requested review from mcbed

@VanshGehlot VanshGehlot Awaiting requested review from VanshGehlot

@VX792 VX792 Awaiting requested review from VX792

@bijoua29 bijoua29 Awaiting requested review from bijoua29

@progtologist progtologist Awaiting requested review from progtologist

@saikishor saikishor Awaiting requested review from saikishor

Assignees
No one assigned
Labels
backport-humble This label should be used by maintaines only! Label triggers PR backport to ROS2 humble. backport-iron This label should be used by maintaines only! Label triggers PR backport to ROS2 Iron.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@qinqon @christophfroehlich @bmagyar @saikishor