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

Proposal to add deadband to thruster #1927

Merged
merged 16 commits into from
Aug 4, 2023

Conversation

ejalaa12
Copy link
Contributor

@ejalaa12 ejalaa12 commented Mar 13, 2023

🎉 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>.

  • If this tag is not present, the deadband will not be applied.
  • otherwise the value in the tag is the absolute value of thrust below which the output will be set to 0.

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

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

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 🌱 garden Ignition Garden label Mar 13, 2023
@osrf-triage osrf-triage added this to Inbox in Core development Mar 13, 2023
@azeey azeey requested a review from caguero March 13, 2023 18:52
@azeey azeey moved this from Inbox to In review in Core development Mar 13, 2023
Copy link
Contributor

@caguero caguero left a 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.

src/systems/thruster/Thruster.cc Show resolved Hide resolved
src/systems/thruster/Thruster.hh Outdated Show resolved Hide resolved
src/systems/thruster/Thruster.cc Outdated Show resolved Hide resolved
src/systems/thruster/Thruster.cc Show resolved Hide resolved
src/systems/thruster/Thruster.cc Outdated Show resolved Hide resolved
src/systems/thruster/Thruster.cc Show resolved Hide resolved
src/systems/thruster/Thruster.cc Show resolved Hide resolved
@ejalaa12
Copy link
Contributor Author

Hi !
Thanks for the quick review @ahcorde @caguero and sorry for the long delay, I didn't have time to work on this during the past month.
I'll update the PR right now.

@ejalaa12
Copy link
Contributor Author

I just pushed the changes.

@ejalaa12
Copy link
Contributor Author

@caguero friendly ping :)

@arjo129
Copy link
Contributor

arjo129 commented Apr 24, 2023

Any chance we can get a test? Thanks!

@ejalaa12
Copy link
Contributor Author

Sure I'm working on it and I'll update the PR by next week

@ejalaa12
Copy link
Contributor Author

@arjo129 just added the unit test for that feature, thanks for reviewing

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.

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

test/integration/thruster.cc Outdated Show resolved Hide resolved
test/integration/thruster.cc Outdated Show resolved Hide resolved
test/integration/thruster.cc Show resolved Hide resolved
test/integration/thruster.cc Outdated Show resolved Hide resolved
src/systems/thruster/Thruster.cc Show resolved Hide resolved
src/systems/thruster/Thruster.cc Outdated Show resolved Hide resolved
@ejalaa12
Copy link
Contributor Author

@arjo129 I addressed your remarks in the new commit. Btw, should I rebase this PR on gz-sim7 ?

Signed-off-by: Alaa El Jawad <ejalaa12@gmail.com>
Signed-off-by: Alaa El Jawad <ejalaa12@gmail.com>
Signed-off-by: Alaa El Jawad <ejalaa12@gmail.com>
Signed-off-by: Alaa El Jawad <ejalaa12@gmail.com>
Signed-off-by: Alaa El Jawad <ejalaa12@gmail.com>
Signed-off-by: Alaa El Jawad <ejalaa12@gmail.com>
Signed-off-by: Alaa El Jawad <ejalaa12@gmail.com>
Signed-off-by: Alaa El Jawad <ejalaa12@gmail.com>
Signed-off-by: Alaa El Jawad <ejalaa12@gmail.com>
Signed-off-by: Alaa El Jawad <ejalaa12@gmail.com>
Signed-off-by: Alaa El Jawad <ejalaa12@gmail.com>
@ejalaa12
Copy link
Contributor Author

I had to do a force-push to reword my previous commits in order to sign them off

@codecov
Copy link

codecov bot commented May 29, 2023

Codecov Report

Merging #1927 (a06cdc2) into gz-sim7 (3dc8c7a) will increase coverage by 0.33%.
Report is 128 commits behind head on gz-sim7.
The diff coverage is 73.61%.

❗ Current head a06cdc2 differs from pull request most recent head 824f8e5. Consider uploading reports for the commit 824f8e5 to get more accurate results

@@             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     
Files Changed Coverage Δ
include/gz/sim/SdfEntityCreator.hh 100.00% <ø> (ø)
include/gz/sim/ServerConfig.hh 100.00% <ø> (ø)
include/gz/sim/Util.hh 100.00% <ø> (ø)
include/gz/sim/components/Component.hh 100.00% <ø> (ø)
include/gz/sim/detail/EntityComponentManager.hh 93.86% <ø> (ø)
include/gz/sim/rendering/MarkerManager.hh 100.00% <ø> (ø)
include/gz/sim/rendering/RenderUtil.hh 100.00% <ø> (ø)
include/gz/sim/rendering/SceneManager.hh 100.00% <ø> (ø)
src/gui/plugins/modules/EntityContextMenu.cc 19.78% <0.00%> (ø)
src/gui/plugins/spawn/Spawn.cc 9.88% <0.00%> (-0.40%) ⬇️
... and 51 more

... and 1 file with indirect coverage changes

@ejalaa12 ejalaa12 requested a review from arjo129 May 31, 2023 11:38
@ejalaa12
Copy link
Contributor Author

ejalaa12 commented Jun 9, 2023

@arjo129 did you have time to look at my latest commits ?

@ejalaa12
Copy link
Contributor Author

@arjo129 friendly ping :)

@arjo129
Copy link
Contributor

arjo129 commented Jul 25, 2023

Sorry for the (really looong) turn around time. There are still two unresolved threads. Please address those.

@ejalaa12
Copy link
Contributor Author

Hey @arjo129, NP, thanks for your last comment.
I addressed the unresolved threads.

Signed-off-by: Alaa El Jawad <ejalaa12@gmail.com>
Signed-off-by: Alaa El Jawad <ejalaa12@gmail.com>
@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023
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.

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.

src/systems/thruster/Thruster.cc Outdated Show resolved Hide resolved
test/integration/thruster.cc Outdated Show resolved Hide resolved
@ejalaa12
Copy link
Contributor Author

ejalaa12 commented Aug 2, 2023

@arjo129 no worries, thanks for sticking up with this PR.
I protected the mentioned block with a mutex, and adjusted the pid settings in the world file to make the commented section work.

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 <ejalaa12@gmail.com>
Signed-off-by: Alaa El Jawad <ejalaa12@gmail.com>
Signed-off-by: Alaa El Jawad <ejalaa12@gmail.com>
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.

Thanks for iterating on this @ejalaa12. LGTM. We can look into the sleep related issue elsewhere.

@arjo129 arjo129 enabled auto-merge (squash) August 3, 2023 05:31
@mjcarroll mjcarroll disabled auto-merge August 4, 2023 14:12
@mjcarroll mjcarroll merged commit e19f26d into gazebosim:gz-sim7 Aug 4, 2023
5 of 8 checks passed
@ejalaa12 ejalaa12 deleted the thruster-deadband branch August 4, 2023 14:49
@ejalaa12
Copy link
Contributor Author

ejalaa12 commented Aug 4, 2023

Thanks for the merge ! :)
Happy to contribute to this project 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants