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

Separate RobotBase from MJBotsInterface #31

Merged
merged 15 commits into from
Aug 3, 2022
Merged

Conversation

jdcaporale
Copy link
Contributor

@jdcaporale jdcaporale commented Aug 2, 2022

(Pull Request 2 of 3 in a sequence: builds from PR #30 and originally in PR #22 )

Adds the RobotInterface Class. Its role is to separate MJBots specfic communication and hardware interfacing from robot state for control. The MJBots hardware interface shares ownership of the JointBase/Moteus pointers and uses them for communications and the robot is free to set trques and to be derived to include any future features (limbs, state estimators, etc.).
Also, adds a simple derived robot example.

TODO:

  • Upgrade the example to match our design intention comments (e.g. this or this on previous PR

The goal of the parent class is to seperate the MJBots specific code from the robot code. Presumably this begins the process of creating a inheritable kodlab robot interface for better communication and accessibility between classes and a future where different systems could be used (instead of just MJBots).
Using the so called 'dreaded diamond' to implement a class that is derived from both an MjbotsRobotInterface and SimpleRobot example class which are each in turn virtually derived from kodlab::RobotInterface. This version requires building and passing in the robot to the control loop which upcasts the pointer to an MjbotsRobotInterface. Should not break legacy code
The multiple inheritance makes the construction fraught and requires writing two contructors currently. Partly because of the scope of ControlLoopOptions, likely we should make a RobotOption class.
The dreaded diamond led to many issues for writing new robot classes and control loops because it over complicated the construction of them. By composing seperate classes (RobotInterfaceDerived and MjbotsRobotInterface) in MjbotsControlLoops all of those issues are removed. Using the existing shared pointer vectors both classes have access to the JointMoteuses (using upcasting or copying directly, resp.), which is necessary for setting and sending torque commands. Lastly, some states are moteus specific, e.g. pi3hat::attitude and moteus::Mode and are now also housed in moteus_interface_ and will require conversion funcitons to be fed into the RobotInterfaceDerived class.
@jdcaporale jdcaporale changed the title Add robot base Separate RobotBase from MJBotsInterface Aug 2, 2022
@ethanmusser ethanmusser added the enhancement New feature or request label Aug 2, 2022
Copy link
Contributor

@ShaneRozenLevy ShaneRozenLevy left a comment

Choose a reason for hiding this comment

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

Need to update the readme. Otherwise besides some naming issues it looks good.

include/examples/simple_robot.h Show resolved Hide resolved
include/kodlab_mjbots_sdk/robot_interface.h Show resolved Hide resolved
include/kodlab_mjbots_sdk/robot_interface.h Outdated Show resolved Hide resolved
@ethanmusser
Copy link
Collaborator

README.md updated in 4b9c6b3.

@ethanmusser ethanmusser marked this pull request as ready for review August 3, 2022 18:03
Copy link
Collaborator

@ethanmusser ethanmusser left a comment

Choose a reason for hiding this comment

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

All problems have been remedied or addressed in seperate issues. LGTM.

README.md Outdated Show resolved Hide resolved
@jdcaporale jdcaporale merged commit 5f12c9d into master Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants