-
Notifications
You must be signed in to change notification settings - Fork 277
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
Proposal to add deadband to thruster #1927
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! I did a first pass and left a few minor comments.
I just pushed the changes. |
@caguero friendly ping :) |
Any chance we can get a test? Thanks! |
Sure I'm working on it and I'll update the PR by next week |
@arjo129 just added the unit test for that feature, thanks for reviewing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea for the feature. However, this does not compile :( (see my comments and CI). After fixing the syntax errors, I am able to run the tests. There are still some clean ups needed. A bit of documentation on the expectations would help me understand the test better.
Also please sign-off your commits. See: https://github.com/gazebosim/gz-sim/pull/1927/checks?check_run_id=13411768621
@arjo129 I addressed your remarks in the new commit. Btw, should I rebase this PR on gz-sim7 ? |
Signed-off-by: Alaa El Jawad <[email protected]>
Signed-off-by: Alaa El Jawad <[email protected]>
Signed-off-by: Alaa El Jawad <[email protected]>
Signed-off-by: Alaa El Jawad <[email protected]>
Signed-off-by: Alaa El Jawad <[email protected]>
Signed-off-by: Alaa El Jawad <[email protected]>
Signed-off-by: Alaa El Jawad <[email protected]>
Signed-off-by: Alaa El Jawad <[email protected]>
Signed-off-by: Alaa El Jawad <[email protected]>
Signed-off-by: Alaa El Jawad <[email protected]>
Signed-off-by: Alaa El Jawad <[email protected]>
I had to do a force-push to reword my previous commits in order to sign them off |
cb5a79b
to
b4af0ed
Compare
Codecov Report
@@ Coverage Diff @@
## gz-sim7 #1927 +/- ##
===========================================
+ Coverage 64.68% 65.01% +0.33%
===========================================
Files 343 354 +11
Lines 27430 28699 +1269
===========================================
+ Hits 17743 18660 +917
- Misses 9687 10039 +352
|
@arjo129 did you have time to look at my latest commits ? |
@arjo129 friendly ping :) |
Sorry for the (really looong) turn around time. There are still two unresolved threads. Please address those. |
Hey @arjo129, NP, thanks for your last comment. |
Signed-off-by: Alaa El Jawad <[email protected]>
Signed-off-by: Alaa El Jawad <[email protected]>
01b9346
to
4289a27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for being obsessive about this, but spotted a few more minor issues. If you're observing flakiness with the test you need only check the last item in the buffer.
@arjo129 no worries, thanks for sticking up with this PR. However I had to rollback the removal of the std::sleep_for, otherwise it breaks the other tests, i'm not entirely sure why. We can probably open a separate issue for this. |
Signed-off-by: Alaa El Jawad <[email protected]>
Signed-off-by: Alaa El Jawad <[email protected]>
Signed-off-by: Alaa El Jawad <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for iterating on this @ejalaa12. LGTM. We can look into the sleep related issue elsewhere.
Thanks for the merge ! :) |
🎉 New feature
This PR is a proposal to add a deadband to the thruster system.
Real thrusters have a physical deadband that are not currently simulated.
Summary
This implements a limit below which the thrusters will not spin nor generate thrust.
It adds an additional parameter in the sdf called
<deadband>
.An additional topic called
/enable_deadband
is also created to enable or disable the deadband effect during runtime.Test it
Unit test are coming...
Checklist
codecheck
passed (See contributing)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.