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

fixed thruster.cc undefined behavior #2316

Closed
wants to merge 22 commits into from
Closed

fixed thruster.cc undefined behavior #2316

wants to merge 22 commits into from

Conversation

GauravKumar9920
Copy link
Contributor

🦟 Bug fix

Fixes #2291

Summary

Note to maintainers: Remember to Merge with commit (not squash-merge or rebase)

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Feb 15, 2024
azeey
azeey requested changes Feb 28, 2024
View reviewed changes
test/integration/thruster.cc Outdated Show resolved Hide resolved
@GauravKumar9920 GauravKumar9920 requested a review from azeey March 2, 2024 15:14
arjo129
arjo129 requested changes Mar 18, 2024
View reviewed changes
Copy link
Contributor

@arjo129 arjo129 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 a number of places where .back() is still being used without being checked.

ctrl+F shows the following instancesFor:

@GauravKumar9920 GauravKumar9920 requested a review from arjo129 March 18, 2024 11:37
arjo129
arjo129 requested changes Mar 19, 2024
View reviewed changes
test/integration/thruster.cc Outdated Show resolved Hide resolved
test/integration/thruster.cc Outdated Show resolved Hide resolved
test/integration/thruster.cc Outdated Show resolved Hide resolved
@GauravKumar9920
Copy link
Contributor Author

I'm sorry, My bad.
I will look into the whole code once again and make changes and make sure I'm not missing out on any bugs or anything.

@GauravKumar9920 GauravKumar9920 requested a review from arjo129 March 19, 2024 10:43
@arjo129
Copy link
Contributor

arjo129 commented Mar 19, 2024

This is looking a lot better!

Do you mind signing off via DCO. Instructions are here:
https://github.com/gazebosim/gz-sim/pull/2316/checks?check_run_id=22827807927

arjo129
arjo129 requested changes Mar 20, 2024
View reviewed changes
test/integration/thruster.cc Outdated Show resolved Hide resolved
@GauravKumar9920 GauravKumar9920 requested a review from arjo129 March 20, 2024 07:40
arjo129
arjo129 requested changes Mar 20, 2024
View reviewed changes
test/integration/thruster.cc Outdated Show resolved Hide resolved
arjo129
arjo129 requested changes Mar 22, 2024
View reviewed changes
test/integration/thruster.cc Outdated Show resolved Hide resolved
test/integration/thruster.cc Outdated Show resolved Hide resolved
GauravKumar9920 and others added 13 commits March 30, 2024 20:32
@GauravKumar9920
fixed thruster.cc undefined behaviour
9d87652 
Signed-off-by: Gaurav Kumar <[email protected]>
@GauravKumar9920
updating the code to check for the empty modelposes
e023e3d 
Signed-off-by: Gaurav Kumar <[email protected]>
@GauravKumar9920
adding a runtime error
5f81ca1 
Signed-off-by: Gaurav Kumar <[email protected]>
@GauravKumar9920
minor fix
2db4d5d 
Signed-off-by: Gaurav Kumar <[email protected]>
@GauravKumar9920
Added assert false function
d3f586c 
Signed-off-by: Gaurav Kumar <[email protected]>
@GauravKumar9920
added ASSERT_FALSE stement that's required elsewhere
1ad599e 
Signed-off-by: Gaurav Kumar <[email protected]>
@GauravKumar9920
minor changes
3239f41 
Signed-off-by: Gaurav Kumar <[email protected]>
@GauravKumar9920 @arjo129
Update test/integration/thruster.cc
59a0f5e 
Co-authored-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Gaurav Kumar <[email protected]>
Signed-off-by: Gaurav Kumar <[email protected]>
@GauravKumar9920
Fixed all the issues, I Hope
be2d152 
Signed-off-by: Gaurav Kumar <[email protected]>
@GauravKumar9920 @arjo129
Update test/integration/thruster.cc
19ce285 
Co-authored-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Gaurav Kumar <[email protected]>
Signed-off-by: Gaurav Kumar <[email protected]>
@GauravKumar9920
Add entity validation to OdometryPublisher (gazebosim#2326)
60861c1 
Until SystemManager has the ability to unload system plugins, plugins require an explicit check of the validity of the entities used in the Update methods. Such a check was missing in OdometryPublisher, which led to non-critical but annoying errors in the console.
---------

Signed-off-by: Anton Bogdanov <[email protected]>
Signed-off-by: Gaurav Kumar <[email protected]>
@g-arjones @GauravKumar9920
Add entity and sdf parameters to Server's AddSystem interface (gazebo…
b918724 
…sim#2324)

Signed-off-by: Gabriel Arjones <[email protected]>
Signed-off-by: Gaurav Kumar <[email protected]>
@caguero @GauravKumar9920
Fix wget in maritime tutorials (gazebosim#2330)
8c63d88 
* Fix wget in maritime tutorials.

Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Gaurav Kumar <[email protected]>
francocipollone and others added 7 commits March 30, 2024 20:32
@francocipollone @GauravKumar9920
Add reference to joint_controller.md tutorial. (gazebosim#2333)
d56ea91 
Signed-off-by: Franco Cipollone <[email protected]>
Signed-off-by: Gaurav Kumar <[email protected]>
@azeey @GauravKumar9920
Prepare for 8.2.0 (gazebosim#2335)
bce4ae2 
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Gaurav Kumar <[email protected]>
@iche033 @GauravKumar9920
Publish step size in world stats topic (gazebosim#2340)
1716fe5 
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Gaurav Kumar <[email protected]>
@iche033 @GauravKumar9920
Check null mesh (gazebosim#2341)
8ca57b4 
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Gaurav Kumar <[email protected]>
@sauk2 @GauravKumar9920
Fixed turning error in ackermann steering (gazebosim#2342)
55cd8a6 
This is a fix to the error seen in Ackermann Steering's <steering_only> mode. The steps to reproduce this error are described in issue gazebosim#2314.

Signed-off-by: Saurabh Kamat <[email protected]>
Signed-off-by: Gaurav Kumar <[email protected]>
@sauk2 @GauravKumar9920
Added mutex to protect stored time variables (gazebosim#2345)
2dcec27 
Signed-off-by: Saurabh Kamat <[email protected]>
Signed-off-by: Gaurav Kumar <[email protected]>
@GauravKumar9920
made the required changes
d580c2d 
Signed-off-by: Gaurav Kumar <[email protected]>
@GauravKumar9920 GauravKumar9920 requested a review from arjo129 March 30, 2024 20:36
arjo129
arjo129 requested changes Apr 1, 2024
View reviewed changes
Copy link
Contributor

@arjo129 arjo129 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Lol. This happens to all of us. If your git-fu is not strong enough I recommend closeing this PR and opening a new one in a new branch of main. Just copy the new test/integraton/thruster.cc over.

test/integration/thruster.cc Outdated Show resolved Hide resolved
@GauravKumar9920 @arjo129
Update test/integration/thruster.cc
0e92da8 
Co-authored-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Gaurav Kumar <[email protected]>
@GauravKumar9920
Copy link
Contributor Author

Lol. This happens to all of us. If your git-fu is not strong enough I recommend closeing this PR and opening a new one in a new branch of main. Just copy the new test/thruster.cc over.

Thanks @arjo129 I was really unsure of what has happened, I wasn't able to find a reliable solution for it online. I will open a new pr : )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arjo129 arjo129 arjo129 requested changes

@mjcarroll mjcarroll Awaiting requested review from mjcarroll mjcarroll is a code owner

@azeey azeey Awaiting requested review from azeey

@mabelzhang mabelzhang Awaiting requested review from mabelzhang

@iche033 iche033 Awaiting requested review from iche033

Assignees
No one assigned
Labels
🎵 harmonic Gazebo Harmonic
Projects
Core development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

INTEGRATION_thruster has undefined behavior potentially causing test flakiness
9 participants
@GauravKumar9920 @arjo129 @azeey @mjcarroll @g-arjones @caguero @francocipollone @iche033 @sauk2