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

Add IsBatteryLow condition node #1974

Merged
merged 8 commits into from
Sep 5, 2020
Merged

Add IsBatteryLow condition node #1974

merged 8 commits into from
Sep 5, 2020

Conversation

naiveHobo
Copy link
Contributor

Basic Info

Info Please fill out this column
Ticket(s) this addresses #1973
Primary OS tested on Ubuntu 18.04
Robotic platform tested on Gazebo TurtleBot3 simulation

Description of contribution in a few bullet points

  • Added IsBatteryLow BT condition node

Description of documentation updates required from your changes

SteveMacenski
SteveMacenski reviewed Sep 3, 2020
View reviewed changes
Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

Wow, that was quick, thanks for taking this on 👍

nav2_behavior_tree/CMakeLists.txt Show resolved Hide resolved
nav2_behavior_tree/include/nav2_behavior_tree/plugins/condition/is_battery_low_condition.hpp Outdated Show resolved Hide resolved
nav2_behavior_tree/include/nav2_behavior_tree/plugins/condition/is_battery_low_condition.hpp Outdated Show resolved Hide resolved
SteveMacenski
SteveMacenski approved these changes Sep 3, 2020
View reviewed changes
Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

Add to MD and website and we can merge

@naiveHobo naiveHobo mentioned this pull request Sep 3, 2020
Merged
@naiveHobo
Copy link
Contributor Author

It looks like voltage is the only mandatory field in sensor_msgs::BatteryState:

https://github.com/ros2/common_interfaces/blob/3ee382893da4c34b43cf836d4c235d25ab4befab/sensor_msgs/msg/BatteryState.msg#L35

Should we still continue with percentage or is voltage a better option?

@mikeferguson
Copy link
Contributor

Should we still continue with percentage or is voltage a better option?

Perhaps both? could be parameterized.

@naiveHobo
Copy link
Contributor Author

@mikeferguson looks okay now?

mikeferguson
mikeferguson suggested changes Sep 3, 2020
View reviewed changes
Copy link
Contributor

@mikeferguson mikeferguson left a comment

Choose a reason for hiding this comment

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

We still need to find_package(std_msgs) - other than that, looks good.

nav2_behavior_tree/CMakeLists.txt Outdated Show resolved Hide resolved
@SteveMacenski
Copy link
Member

SteveMacenski commented Sep 3, 2020 •
edited
Loading

@mikeferguson don’t you think both is a little unclear? I’d prefer to work on just the % unless you think its atypical for implementations to make proper use of that field. % is a more linear than voltage and easier for a typical person to reason about when making robot navigation decisions (“I should probably redock at 20%” vs “I need to take a bunch of data and model what voltage I to threshold at”)

Or “both” in meaning it checks for both a voltage and % threshold, not either or. By default then we can just make the voltage outrageously high so that it doesnt affect users that dont override it.

@mikeferguson
Copy link
Contributor

I would have initially said "use percentage" - but it was pointed out that only the voltage is "required" in that message. For higher end robots, they should have a good estimate of battery state, but some lower end robots may not. I would suggest that maybe we invert the logic, so by default it uses percentage (since that is the better estimate), but there is a way to get a voltage-based mode?

@SteveMacenski
Copy link
Member

but there is a way to get a voltage-based mode?

I don't understand the question. Can you rephrase?

I agree that if we do either/or that percentage should be default. I could live with that.

@mikeferguson
Copy link
Contributor

I don't understand the question. Can you rephrase?

Not really a question. I was basically suggesting that we have two modes:

  • Default: uses percentage
  • There is some way to instead use voltage, for those robots that don't have a good percentage estimate.

SteveMacenski
SteveMacenski reviewed Sep 4, 2020
View reviewed changes
Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

Needs markdown file in doc/parameters updated with new BT

@SteveMacenski SteveMacenski merged commit 91d0a83 into ros-navigation:main Sep 5, 2020
@naiveHobo naiveHobo deleted the battery-low branch September 5, 2020 14:47
SteveMacenski pushed a commit that referenced this pull request Nov 4, 2020
@naiveHobo @SteveMacenski
Add IsBatteryLow condition node (#1974)
1ce77b8 
* Add IsBatteryLow condition node

* Update default battery topic and switch to battery %

* Fix test

* Switch to sensor_msgs/BatteryState

* Add option to use voltage by default or switch to percentage

* Add sensor_msgs dependency in package.xml

* Make percentage default over voltage

* Update parameter list
@SteveMacenski SteveMacenski mentioned this pull request Nov 4, 2020
Merged
SteveMacenski added a commit that referenced this pull request Nov 4, 2020
@SteveMacenski @mikeferguson
@wilcobonestroo @naiveHobo @mbuijs @justinIRBT
Foxy sync 4 - SMAC planner, amongst others (#2072)
96b4713 
* initialize variables in inflation layer (#1970)

* Fix zero waypoints crash (#1978)

* return if the number of waypoints is zero.

* terminate the action.

* Succeed action instead of terminating.

* Add IsBatteryLow condition node (#1974)

* Add IsBatteryLow condition node

* Update default battery topic and switch to battery %

* Fix test

* Switch to sensor_msgs/BatteryState

* Add option to use voltage by default or switch to percentage

* Add sensor_msgs dependency in package.xml

* Make percentage default over voltage

* Update parameter list

* Initialize inflate_cone_ variable. (#1988)

* Initialize inflate_cone_ variable.

* initialize inflate_cone_ based on parameter.

* Increase the sleep time in the tests makes the costmap test always succeed on my machine.

* Add timeouts to all spin_until_future_complete calls (#1998)

* Add timeouts to all spin_until_future_complete calls

Signed-off-by: Sarthak Mittal <[email protected]>

* Update default timeout value

Signed-off-by: Sarthak Mittal <[email protected]>

* Controllers should not be influenced by time jumps or slew (#2012)

* Controllers should not be influenced by time jumps

Therefore use rclcpp::GenericRate<std::chrono::steady_clock> instead of
rclcpp::Rate

Signed-off-by: Martijn Buijs <[email protected]>

* Change to using `rclcpp::WallRate` for better readability

Signed-off-by: Martijn Buijs <[email protected]>

* Fix max path cycles for case where map has larger Y dimension than X dimension (#2017)

* Fix max path cycles for case where map has larger Y dimension than X dimension

* Improve readability

* fix ament_cpplint and ament_uncrustify issues

* fix minor cherry pick conflict mistake

* bump version to 0.4.4

Co-authored-by: Michael Ferguson <[email protected]>
Co-authored-by: Wilco Bonestroo <[email protected]>
Co-authored-by: Sarthak Mittal <[email protected]>
Co-authored-by: Martijn Buijs <[email protected]>
Co-authored-by: justinIRBT <[email protected]>
ruffsl pushed a commit to ruffsl/navigation2 that referenced this pull request Jul 2, 2021
@naiveHobo @ruffsl
Add IsBatteryLow condition node (ros-navigation#1974)
0b9e866 
* Add IsBatteryLow condition node

* Update default battery topic and switch to battery %

* Fix test

* Switch to sensor_msgs/BatteryState

* Add option to use voltage by default or switch to percentage

* Add sensor_msgs dependency in package.xml

* Make percentage default over voltage

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

@SteveMacenski SteveMacenski SteveMacenski approved these changes

@mikeferguson mikeferguson mikeferguson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@naiveHobo @mikeferguson @SteveMacenski