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

Battery updates #183

Merged
merged 3 commits into from
Jun 9, 2020
Merged

Battery updates #183

merged 3 commits into from
Jun 9, 2020

Conversation

caguero
Copy link
Contributor

@caguero caguero commented Jun 5, 2020

This patch introduces the following battery updates:

  • The recharging is no longer triggered by the parameter socThreshold. The old behavior implies that a battery automatically starts charging when the battery capacity falls below a certain threshold. I think it's more realistic that the recharging can be started or stopped on demand. I took the liberty of removing the socThreshold parameter directly because it wasn't documented in the header file.

  • If <enable_recharging> is present, two new services are available for starting or stopping the battery recharge.

  • If there's no joint velocity/force command, the battery doesn't drain anymore. Before, once the battery started draining, it never stopped even if the joints weren't moving.

  • I updated the battery status published. It can now be in CHARGING, DISCHARGING, FULL or NOT_CHARGING state.

  • I documented the SDF charging parameters and added a few minor tweaks.

caguero added 2 commits June 5, 2020 17:59
Signed-off-by: Carlos Aguero <caguero@openrobotics.org>
Signed-off-by: Carlos Aguero <caguero@openrobotics.org>
@caguero caguero requested a review from claireyywang June 5, 2020 16:34
@caguero caguero requested a review from chapulina as a code owner June 5, 2020 16:34
@github-actions github-actions bot added the 📜 blueprint Ignition Blueprint label Jun 5, 2020
@codecov
Copy link

codecov bot commented Jun 5, 2020

Codecov Report

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

Impacted file tree graph

@@             Coverage Diff              @@
##           ign-gazebo2     #183   +/-   ##
============================================
  Coverage        62.35%   62.35%           
============================================
  Files              123      123           
  Lines             6101     6101           
============================================
  Hits              3804     3804           
  Misses            2297     2297           

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 ffde437...d988d5f. Read the comment docs.

Copy link
Contributor

@claireyywang claireyywang 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 improving the battery feature! I wasn't sure how to test the start/stop recharge function. Could we add some documentation on what service to send in the sdf file?

@caguero
Copy link
Contributor Author

caguero commented Jun 8, 2020

Thanks for improving the battery feature! I wasn't sure how to test the start/stop recharge function. Could we add some documentation on what service to send in the sdf file?

It's a bit harder to test because we're using a one way service, as we don't expect any response. One way services are not supported in ign command line tool for now. You can create a publisher similar to the one in https://github.com/ignitionrobotics/ign-transport/blob/master/example/requester_oneway.cc and publish it to /vehicle_blue/linear_battery/recharge/start or /vehicle_blue/linear_battery/recharge/stop.

How about I update ign tools to support one way services, and then, I update this example in a separate PR?

@claireyywang
Copy link
Contributor

How about I update ign tools to support one way services, and then, I update this example in a separate PR?

If that's not too much work then I think that'd be very helpful for demo purposes. And yes, separate PR sounds like a good plan.

Copy link
Contributor

@claireyywang claireyywang left a comment

Choose a reason for hiding this comment

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

LGTM, just waiting for pending CI to approve

Copy link
Contributor

@claireyywang claireyywang left a comment

Choose a reason for hiding this comment

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

Approve since the homebrew test failure doesn't seem to be introduced by this PR. @chapulina I saw a similar test failure in this CI https://build.osrfoundation.org/job/ignition_gazebo-ci-pr_any-homebrew-amd64/3142/

@caguero caguero merged commit 668c01e into ign-gazebo2 Jun 9, 2020
@caguero caguero deleted the battery_recharge branch June 9, 2020 07:07
@caguero caguero mentioned this pull request Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants