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

Battery updates - part3 #230

Merged
merged 10 commits into from
Jul 13, 2020
Merged

Battery updates - part3 #230

merged 10 commits into from
Jul 13, 2020

Conversation

caguero
Copy link
Contributor

@caguero caguero commented Jun 30, 2020

Fixes issue #225 .

@caguero
More battery updates.
1bc7890 
Signed-off-by: Carlos Aguero <[email protected]>
@caguero
Tweaks
6955594 
Signed-off-by: Carlos Aguero <[email protected]>
@caguero caguero requested a review from azeey June 30, 2020 21:58
@caguero caguero requested a review from chapulina as a code owner June 30, 2020 21:58
@github-actions github-actions bot added the 📜 blueprint Ignition Blueprint label Jun 30, 2020
@caguero caguero changed the title Battery updates p3 Battery updates - part3 Jun 30, 2020
@codecov
Copy link

codecov bot commented Jun 30, 2020 •
edited
Loading

Codecov Report

Merging #230 into ign-gazebo2 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           ign-gazebo2     #230   +/-   ##
============================================
  Coverage        62.34%   62.34%           
============================================
  Files              123      123           
  Lines             6102     6102           
============================================
  Hits              3804     3804           
  Misses            2298     2298

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cdb8aa5...e0642d1. Read the comment docs.

@caguero
Updated tutorial and tweaks
4442b0d 
Signed-off-by: Carlos Aguero <[email protected]>
@caguero caguero requested a review from maryaB-osr as a code owner July 6, 2020 15:23
@caguero
Enable recharging
f8d2916 
Signed-off-by: Carlos Aguero <[email protected]>
maryaB-osr
maryaB-osr approved these changes Jul 6, 2020
View reviewed changes
Copy link
Contributor

@maryaB-osr maryaB-osr left a comment

Choose a reason for hiding this comment

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

Tutorial changes lgtm, with a couple typo fixes

tutorials/battery.md Outdated Show resolved Hide resolved
tutorials/battery.md Outdated Show resolved Hide resolved
tutorials/battery.md Outdated Show resolved Hide resolved
azeey
azeey requested changes Jul 8, 2020
View reviewed changes
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

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

Looks good. Just a few comments about parameter handling.

src/systems/battery_plugin/LinearBatteryPlugin.cc Show resolved Hide resolved
src/systems/battery_plugin/LinearBatteryPlugin.cc Show resolved Hide resolved
src/systems/battery_plugin/LinearBatteryPlugin.cc Show resolved Hide resolved
src/systems/battery_plugin/LinearBatteryPlugin.cc Show resolved Hide resolved
tutorials/battery.md Outdated Show resolved Hide resolved
examples/worlds/linear_battery_demo.sdf Outdated Show resolved Hide resolved
@azeey
Copy link
Contributor

azeey commented Jul 13, 2020

LGTM. Just missing a changelog

@caguero
Changelog
e0642d1 
Signed-off-by: Carlos Aguero <[email protected]>
@caguero
Copy link
Contributor Author

caguero commented Jul 13, 2020

LGTM. Just missing a changelog

Added e0642d1.

@caguero caguero merged commit 3539321 into ign-gazebo2 Jul 13, 2020
@caguero caguero deleted the battery_updates_p3 branch July 13, 2020 16:13
@caguero caguero mentioned this pull request Jul 13, 2020
Closed
@peci1
Copy link
Contributor

peci1 commented Nov 16, 2021

This PR removed the <start_on_motion> configuration option, while there are still examples using it. Generally, I think it's useful to have the option to start draining the battery as soon as the model is inserted in simulation.

@chapulina
Copy link
Contributor

This PR removed the <start_on_motion> configuration option

I suspect that was probably unintentional, can you confirm, @caguero ?

Would you be able to open a PR bringing it back, @peci1 ?

@peci1
Copy link
Contributor

peci1 commented Nov 16, 2021

Well, I'd rather not - as I went through the code, it seems to me the internal logic became quite complicated over time and I'm not sure I understand all the modes in which this plugin should work.

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

@azeey azeey azeey approved these changes

@maryaB-osr maryaB-osr maryaB-osr approved these changes

@chapulina chapulina Awaiting requested review from chapulina

Assignees
No one assigned
Labels
📜 blueprint Ignition Blueprint
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@caguero @azeey @peci1 @chapulina @maryaB-osr