Adding battery consumers and extra fixes #1811

marcoag · 2022-11-24T13:11:35Z

🎉 New feature

Closes: osrf/lrauv#259

Summary

This PR includes the following changes to the battery plugin:

Improvement so entities can become battery consumers: With the changes included in this PR any Entity that contains a BatteryPowerLoad component can become a consumer. They would have to indicate the name of the battery they want to use and the value of their consumption. This value can be updated at any point during the simulation and the battery will update the total consumption.
Fix to battery tests: For some reason battery tests were testing against the BatterySoC component like it holds voltage values while it seems to save the state of charge that goes from 0 to 1. This PR updates the tests appropriately so they are performed according to this.
Battery plugin weird behavior alternative: Even though startDraining was initialized to true the battery would never start draining at startup due to startDrainingFromTopics starting at false and the fact that this value would take over the other here. So the only way to currently make the battery start draining is to setup a topic and send a message to it. Also there's no way to stop the battery draining. To provide an alternative and maintain behavior this PR introduces a <start_draining> parameter that can be set through the SDFormat in order to make the battery start draining right away or not, this defaults to false to preserve behavior. Also a topic can be set now through <stop_power_draining_topic> to stop the battery draining.
Fix to battery inital state of charge: Battery initial state of charge was being set to 1.0 no matter what the initial values of the parameters were. Given that the battery does not starts draining by default and before the only way to set it to drain was through a topic most of the times the battery state of charge would always stay at 1.0. Now the battery state is initially set to whatever the initial parameters indicate.
Thruster battery consuming option: This also adds the option to make the thruster consume power load from a certain battery through the sdf config parameters. This is meant to provide an example on how to make any plugin set battery consumers. A test is added that includes an example sdf.

Test it

Examples on how to test it can be found on the test that was modified in this PR and the world file.

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.

Signed-off-by: Marco A. Gutierrez <[email protected]>

codecov · 2022-11-28T04:06:49Z

Codecov Report

Merging #1811 (c3dc705) into ign-gazebo6 (3448d05) will increase coverage by 0.03%.
The diff coverage is 80.28%.

@@               Coverage Diff               @@
##           ign-gazebo6    #1811      +/-   ##
===============================================
+ Coverage        64.73%   64.77%   +0.03%     
===============================================
  Files              321      322       +1     
  Lines            26313    26374      +61     
===============================================
+ Hits             17034    17083      +49     
- Misses            9279     9291      +12

Impacted Files	Coverage Δ
src/systems/thruster/Thruster.cc	`85.12% <63.63%> (-4.38%)`	⬇️
src/systems/battery_plugin/LinearBatteryPlugin.cc	`75.60% <94.59%> (+2.49%)`	⬆️
...ude/ignition/gazebo/components/BatteryPowerLoad.hh	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

arjo129

First pass. Just caught some minor things.

Would be interesting to see this integrated with some actual system.

include/ignition/gazebo/components/BatteryPowerLoad.hh

src/systems/battery_plugin/LinearBatteryPlugin.cc

Signed-off-by: Marco A. Gutierrez <[email protected]>

arjo129 · 2022-12-01T06:57:34Z

It might be worth having an example system (could be in this PR or a separate PR) that actually consumes the battery. Maybe perhaps the thruster or joint controller system? Alternatively an entirely new system would work too. If we merge this without a demo system there is no guarantee that later on we won't have to break ABI to actually use the battery component.

Signed-off-by: Marco A. Gutierrez <[email protected]>

marcoag · 2022-12-02T12:23:37Z

Added battery consuming capabilities to the thruster plugin and included a test for it.

caguero · 2022-12-08T18:12:24Z

include/ignition/gazebo/components/BatteryPowerLoad.hh

@@ -0,0 +1,54 @@
+/*
+ * Copyright (C) 2018 Open Source Robotics Foundation


caguero · 2022-12-08T18:13:18Z

include/ignition/gazebo/components/BatteryPowerLoad.hh

+ * limitations under the License.
+ *
+*/
+#ifndef IGNITION_GAZEBO_COMPONENTS_BATTERYCONSUMPTION_HH_


For consistency, this name should match the file name.

true, left over from previous naming...

caguero · 2022-12-08T18:14:36Z

include/ignition/gazebo/components/BatteryPowerLoad.hh

+#ifndef IGNITION_GAZEBO_COMPONENTS_BATTERYCONSUMPTION_HH_
+#define IGNITION_GAZEBO_COMPONENTS_BATTERYCONSUMPTION_HH_
+
+#include <string>


I don't think you need this <include>.

Nope, leftover from previous commit.

caguero · 2022-12-08T18:16:10Z

include/ignition/gazebo/components/BatteryPowerLoad.hh

+  /// and the name of the battery it uses.
+  struct BatteryPowerLoadInfo
+  {
+    /// \brief Name of the battery to use.


Entity of the battery to use instead?

Yup, thanks.

caguero · 2022-12-08T18:16:32Z

include/ignition/gazebo/components/BatteryPowerLoad.hh

+  {
+    /// \brief Name of the battery to use.
+    Entity batteryId;
+    /// \brief Battery power load to add to the battery.


Could you specify the units here?

Watts, added.

caguero · 2022-12-08T18:17:41Z

include/ignition/gazebo/components/BatteryPowerLoad.hh

+    double batteryPowerLoad;
+  };
+
+  /// A component that indicates the total consumption of a battery.


Add \brief.

caguero · 2022-12-08T18:30:58Z

src/systems/battery_plugin/LinearBatteryPlugin.cc

-  // after it has been started by a topic. See this comment:
-  // https://github.com/ignitionrobotics/ign-gazebo/pull/1255#discussion_r770223092
-  this->dataPtr->startDraining = this->dataPtr->startDrainingFromTopics;
+  // Recaculate the toal power load among consumers


Recalculate the total

caguero · 2022-12-08T18:35:18Z

src/systems/battery_plugin/LinearBatteryPlugin.cc

+      this->dataPtr->consumerId, total_power_load);
+  if (!success)
+      ignerr << "Failed to set consumer power load." << std::endl;
+
  // Start draining the battery if the robot has started moving


Does this block make sense with the new architecture? I mean, it seems that we're checking if the robot has some velocity in order to start draining the battery. Should that now be taken care by each battery consumer?

I didn't wanna remove this because it might be tricky without changing behavior. Maybe this could still be an option as I could see people wanting this behavior opposed to just start draining right away. Maybe the <start_draining> parameter should inhibit this behavior but keep it when <start_draining> was not set?

I think it's OK to leave it as it is for now for not changing behavior.

caguero · 2022-12-08T18:36:23Z

src/systems/thruster/Thruster.cc

+  /// \brief Battery power load of the thruster.
+  public: double powerLoad = 0.0;
+
+  ///


Add documentation.

Missed this one. Added.

Signed-off-by: Marco A. Gutierrez <[email protected]>

Changes have been made.

azeey · 2022-12-22T00:01:07Z

This has introduced a new warning on macOS (https://build.osrfoundation.org/job/ignition_gazebo-ci-pr_any-homebrew-amd64/9636/clang/). @marcoag mind fixing it?

marcoag · 2022-12-22T00:39:04Z

Thanks for the catch, opened a PR with the fix here.

adding battery consumers and extra improvments

772f951

Signed-off-by: Marco A. Gutierrez <[email protected]>

marcoag requested a review from mjcarroll as a code owner November 24, 2022 13:11

github-actions bot added the 🏯 fortress Ignition Fortress label Nov 24, 2022

Merge branch 'ign-gazebo6' into marcoag/battery_plugin_improvements

d2c961a

azeey added the MBARI-LRAUV Sponsored by MBARI-LRAUV project: https://github.com/osrf/lrauv label Nov 28, 2022

azeey requested a review from caguero November 28, 2022 19:47

arjo129 previously requested changes Nov 29, 2022

View reviewed changes

include/ignition/gazebo/components/BatteryPowerLoad.hh Outdated Show resolved Hide resolved

include/ignition/gazebo/components/BatteryPowerLoad.hh Outdated Show resolved Hide resolved

src/systems/battery_plugin/LinearBatteryPlugin.cc Outdated Show resolved Hide resolved

Marco A. Gutierrez added 2 commits November 30, 2022 17:04

fixing docstrings

b4c52e1

Signed-off-by: Marco A. Gutierrez <[email protected]>

using entities instead of names

f283333

Signed-off-by: Marco A. Gutierrez <[email protected]>

fix to initial state of charge

f29f1f4

Signed-off-by: Marco A. Gutierrez <[email protected]>

marcoag changed the title ~~adding battery consumers and extra improvments~~ Adding battery consumers and extra fixes Dec 1, 2022

Marco A. Gutierrez added 2 commits December 2, 2022 14:48

Merge branch 'ign-gazebo6' into marcoag/battery_plugin_improvements

d331dba

adding battery consumption capabilities to thruster

e6b51a8

Signed-off-by: Marco A. Gutierrez <[email protected]>

marcoag requested a review from arjo129 December 2, 2022 12:26

caguero reviewed Dec 8, 2022

View reviewed changes

Marco A. Gutierrez added 2 commits December 15, 2022 20:58

typos, documentation and extra fixes

5887e7c

Signed-off-by: Marco A. Gutierrez <[email protected]>

Updating battery tutorial, adding consumers info

c3dc705

Signed-off-by: Marco A. Gutierrez <[email protected]>

marcoag requested a review from caguero December 18, 2022 15:50

caguero approved these changes Dec 19, 2022

View reviewed changes

marcoag merged commit 3830bbb into ign-gazebo6 Dec 20, 2022

marcoag deleted the marcoag/battery_plugin_improvements branch December 20, 2022 08:30

marcoag mentioned this pull request Dec 22, 2022

Header guard fix for battery power load component #1846

Merged

8 tasks

caguero mentioned this pull request Dec 26, 2022

[2022 M4] Improve battery model (non-constant discharge rates) osrf/lrauv#259

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding battery consumers and extra fixes #1811

Adding battery consumers and extra fixes #1811

marcoag commented Nov 24, 2022 •

edited

Loading

codecov bot commented Nov 28, 2022 •

edited

Loading

arjo129 left a comment

arjo129 commented Dec 1, 2022

marcoag commented Dec 2, 2022

caguero Dec 8, 2022

marcoag Dec 15, 2022

caguero Dec 8, 2022

marcoag Dec 15, 2022

caguero Dec 8, 2022

marcoag Dec 15, 2022

caguero Dec 8, 2022

marcoag Dec 15, 2022

caguero Dec 8, 2022

marcoag Dec 15, 2022

caguero Dec 8, 2022

marcoag Dec 15, 2022

caguero Dec 8, 2022

marcoag Dec 15, 2022

caguero Dec 8, 2022

marcoag Dec 15, 2022

caguero Dec 15, 2022

caguero Dec 8, 2022

marcoag Dec 15, 2022

azeey commented Dec 22, 2022

marcoag commented Dec 22, 2022

		@@ -0,0 +1,54 @@
		/*
		* Copyright (C) 2018 Open Source Robotics Foundation

Adding battery consumers and extra fixes #1811

Adding battery consumers and extra fixes #1811

Conversation

marcoag commented Nov 24, 2022 • edited Loading

🎉 New feature

Summary

Test it

Checklist

codecov bot commented Nov 28, 2022 • edited Loading

Codecov Report

arjo129 left a comment

Choose a reason for hiding this comment

arjo129 commented Dec 1, 2022

marcoag commented Dec 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azeey commented Dec 22, 2022

marcoag commented Dec 22, 2022

marcoag commented Nov 24, 2022 •

edited

Loading

codecov bot commented Nov 28, 2022 •

edited

Loading