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

Add UFACTORY 850 as one of the robots supported, introduce various HRI utilities for teleop and next episode hinting while reseting #493

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

vmayoral
Copy link

@vmayoral vmayoral commented Oct 28, 2024

This PR comes as a result of the Oct 2024 Hackathon wherein I participated remotely. Results were successful within the weekend of hacking, so I thought I'd give back and contribute. Results first:

resulting dataset resulting policy
https://huggingface.co/datasets/vmayoral/u850_test https://huggingface.co/vmayoral/act_u850_test

The contributions of this PR are summarized below:

  • Add UFACTORY 850 as one of the supported robots, validing principles and introducing support for a commercial-grade lowcost cobot solution
  • Enabled teleop while reseting episode
  • Various minor updates for usability including next episode dislosure while reseting
  • Ensures datatypes are consistent across realsense camera instantiations
  • Proposes a generic xArmWrapper of the UFACTORY Python SDK, usable with other arms from the same vendor
  • extends manipulator.py with minimal changes (attempted respecting the already existing structure) to support the new arm tested
  • Configs and related minor changes

Some notes and considerations:

  • Current teleop mechanism in UFACTORY is jerky when compared to simpler implementations. There's plenty of margin for improvement here. Let me know if this of interest and I may find some room for contributing here as well.

- Added extensions to manipulator to support 850
- Create Motors' UFactory abstraction
- Proposed a new example demonstrating local setup

Signed-off-by: Víctor Mayoral Vilches <[email protected]>
Signed-off-by: Víctor Mayoral Vilches <[email protected]>
Signed-off-by: vmayoral <[email protected]>
This is useful while recording many episodes, since
you just loose track of them and the verbose outputs
from the teleop don't allow for clarity on this regard

Signed-off-by: vmayoral <[email protected]>
Also, it's a derivative of the previous ones

Signed-off-by: vmayoral <[email protected]>
@Cadene
Copy link
Collaborator

Cadene commented Oct 31, 2024

Wow really cool! We are in the middle of a refactor. We will come back to this PR soon. In the meantime, I am curious if other people working on UFACTORY 850 are able to use this arm with LeRobot :)

@vmayoral
Copy link
Author

vmayoral commented Nov 4, 2024

Let me know @Cadene if you'd like other flavours of xArm robots tested. I'm rather certain it will, as the UFACTORY API is rather well structured and known reproducible. I've got an xArm Lite 6 also lying around, happy to test it if that gets this PR moving forward.

@Cadene
Copy link
Collaborator

Cadene commented Nov 4, 2024

@vmayoral Sounds good ;) Any chance you could run pre-commit run --all-files?
See: https://github.com/huggingface/lerobot/blob/main/CONTRIBUTING.md#submitting-a-pull-request-pr
Thanks 🙏

@@ -497,15 +515,21 @@ def teleop_step(
# Cap goal position when too far away from present position.
# Slower fps expected due to reading from the follower.
if self.config.max_relative_target is not None:
present_pos = self.follower_arms[name].read("Present_Position")
if self.robot_type == "u850":
Copy link

@apockill apockill Nov 7, 2024

Choose a reason for hiding this comment

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

This approach of adding if/elif throughout the codebase doesn't seem scalable.

What do folks think about creating an abstract base class that all robots can implement their wrapper API's within, so we don't have to edit so much code in a brittle way to add new robot support?

I can take this on- I'm currently working on adding support for the elephant robotics MyArmM&C series.

The following abstract class would describe the current robot supported by manipulator.py, but personally I don't like the API and I think it's wayyy too specific to one robot:

from abc import ABC, abstractmethod

class BaseRobotWrapper(ABC):

   @abstractmethod
   def read(self, cmd: Literal["Present_Position]) -> npt.NDArray[np.float64]:
       pass
       
   @abstractmethod
   def write(self, cmd: Literal["Operating_Mode", "Torque_Enable", "Goal_Position", "Lock", "Mode", "P_Coefficient", "I_Coefficient", "D_Coefficient", "Acceleration", "Maximum_Acceleration"], val: int, motors: str | list[str] | None=None) -> None:
       pass
       

Something more generic could be made, leaving the API high level:

class BaseRobotWrapper(ABC):

   @abstractmethod
   def read_angles(self) -> npt.NDArray[np.float64]:
       """Read arbitrary number of joints to the robot. Mirrors write_angles"""

   @abstractmethod    
   def write_angles(self, angles: npt.NDArray[np.float64]) -> None:
       """Write arbitrary number of joints to the robot. Mirrors read_angles"""

   @abstractmethod
   def set_follower_presets() -> none:
      """This would replace:
      
      - set_koch_robot_preset
      - set_aloha_robot_preset
      - set_so100_robot_preset
      - ...
      """

   @abstractmethod
   def set_leader_presets() -> None:
      pass    

   @abstractmethod
   def reset() -> None:
      """Used when 'reset_environment' is called. Optional to implement."""

Copy link
Author

Choose a reason for hiding this comment

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

@apockill, I've started reviewing my code following @Cadene's requests above, but I also like your suggestion and would be happy to work together on this. Frankly, an abstraction layer such as the one you propose would facilitate future ports, but I'm unsure if this is something desired at all, so I just took the most straightforward approach for my weekend of hacking.

Nevertheless, I think your suggestion is needed and a code rebase is probably the best way to do it. Happy to participate on that as well. For now, here, I'll focus on my initial intent and contribution.

Choose a reason for hiding this comment

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

Okay, sounds good! Maybe let's keep things as is, once xarm / myarm support is merged in I can jump in and write wrappers for all the currently supported robots.

The only hard thing will just be to have everyone test their bots out for regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants