Skip to content

Robot wrapper#1341

Merged
aclegg3 merged 25 commits intomasterfrom
robot_wrapper
Jun 27, 2021
Merged

Robot wrapper#1341
aclegg3 merged 25 commits intomasterfrom
robot_wrapper

Conversation

@aclegg3
Copy link
Copy Markdown
Contributor

@aclegg3 aclegg3 commented Jun 24, 2021

Motivation and Context

End goal is to provide an extensible convenience interface for common robot functionality. This PR addresses immediate need for wheeled mobile manipulator and specifically Fetch robot interface.

Migrated from @ASzot's implementation.

Some open questions are listed TODO in the code for future consideration.

Secondary:

  • Adds "hab_fetch" option to datasets_download.py
  • Fixes a bug with ArticulatedLink bounding boxes
  • Adds Bullet debug draw toggle to viewer with ,

How Has This Been Tested

Added a pytest.
Output:

test_fetch_robot_wrapper.mp4

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jun 24, 2021
Copy link
Copy Markdown
Contributor

@erikwijmans erikwijmans left a comment

Choose a reason for hiding this comment

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

Just some quick comments

Comment thread habitat_sim/robots/mobile_manipulator.py Outdated
Comment thread habitat_sim/robots/mobile_manipulator.py Outdated
Comment thread habitat_sim/robots/fetch_robot.py Outdated
Comment thread habitat_sim/robots/mobile_manipulator.py
Comment thread habitat_sim/robots/fetch_robot.py Outdated
Comment thread habitat_sim/robots/fetch_robot.py Outdated
@aclegg3 aclegg3 marked this pull request as ready for review June 25, 2021 02:26
@aclegg3
Copy link
Copy Markdown
Contributor Author

aclegg3 commented Jun 25, 2021

Looks like any import habitat_sim will error unless we manually defer the robots import in __init__.py. 🤔

@erikwijmans
Copy link
Copy Markdown
Contributor

erikwijmans commented Jun 25, 2021

I think that is because there is no __init__.py in the robots folder. Without one of those, even when they are just a blank file, python often does the wrong thing.

They are also definitely needed for installation to work as the installer uses __init__.py files to figure out what folders to include.

@aclegg3
Copy link
Copy Markdown
Contributor Author

aclegg3 commented Jun 25, 2021

I think that is because there is no __init__.py in the robots folder.

😆 Apparently I forgot to git add that.

@aclegg3 aclegg3 changed the title [WIP] Robot wrapper Robot wrapper Jun 25, 2021
Copy link
Copy Markdown
Contributor

@mathfac mathfac left a comment

Choose a reason for hiding this comment

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

Looks great. Left some naming and styling comment that can be easily addressed. Some harder to fix comments can be addressed in follow up PRs. Thank you @ASzot and @aclegg3 for contributing the wrapper.

Comment thread tests/test_robot_wrapper.py Outdated
Comment thread tests/test_robot_wrapper.py Outdated
Comment thread habitat_sim/robots/robot_interface.py


@attr.s(auto_attribs=True, slots=True)
class MobileManipulatorParams:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In follow up PRs make sense to use config node instead to hold this parameters.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Completely agree. Config makes sense here.

self._limit_robo_joints = limit_robo_joints

self._arm_sensor_names = [
s for s in self._sim._sensors if s.startswith("robot_arm_")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hardcoded names of the sensors may not play well with several robots. Need follow up PRs.

Comment thread habitat_sim/robots/mobile_manipulator.py Outdated
Comment thread habitat_sim/robots/mobile_manipulator.py Outdated
Comment thread habitat_sim/robots/mobile_manipulator.py Outdated
Comment thread habitat_sim/robots/mobile_manipulator.py Outdated
Comment thread habitat_sim/robots/fetch_robot.py Outdated
def reconfigure(self) -> None:
super().reconfigure()
# remove any default damping motors
for motor_id in self._robot.existing_joint_motor_ids:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this all isn't specific for the Fetch robot only we can move it to Mobile Manipulator reconfigure.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Depends on whether we want to use the default damping motors from URDF or clear them for custom settings. This seemed like a top level decision so we could call super methods. 🤔
Let's see how we feel once we try a couple more robots.

Comment thread habitat_sim/robots/mobile_manipulator.py Outdated
Comment thread habitat_sim/robots/mobile_manipulator.py Outdated
Comment thread habitat_sim/robots/mobile_manipulator.py Outdated
@aclegg3 aclegg3 merged commit 1fd47f3 into master Jun 27, 2021
@aclegg3 aclegg3 deleted the robot_wrapper branch June 27, 2021 22:20
@property
def arm_joint_limits(self) -> Tuple[np.ndarray, np.ndarray]:
"""Get the arm joint limits in radians"""
arm_pos_indices = list(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This probably would have been a bit faster using a dedicated getter and a list comprehension. apologies for not getting to this pr in time*

map(lambda x: self.joint_pos_indices[x], self.params.arm_joints)
)

lower_lims = [self.joint_limits[0][i] for i in arm_pos_indices]
Copy link
Copy Markdown
Contributor

@Skylion007 Skylion007 Jun 27, 2021

Choose a reason for hiding this comment

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

Suggested change
lower_lims = [self.joint_limits[0][i] for i in arm_pos_indices]
lower_lims, upper_lims = self.joint_limits[:, arm_pos_indices]

Will be, much, much faster.

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

Labels

CLA Signed Do not delete this pull request or issue due to inactivity.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants