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

RSDK-8460 - Modularize the xarm driver and remove it from RDK #4578

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

Conversation

nfranczak
Copy link
Member

@nfranczak nfranczak commented Nov 21, 2024

This PR removes the xArm driver within the RDK.
Once this is merged 1/2 of the work needed for this ticket will be done.
The other half is the migration script which will convert relevant configs to use the module.
Timing is an important consideration here. If this PR is merged before this upcoming Tuesday 11.26.24 then when the new RDK is cut on Tuesday, app.viam will no longer give the user the option of choosing between the module and RDK object.
The migration will be run on Tuesday, shortly after we have a new release.

Testing that has been done on the xArm module:

  1. the module is used by the wine-pouring-demo. In the demo's script I use CurrentInputs and the DoCommand to open/close the gripper as well as set the speed and acceleration. The script also uses the motion service's DoCommand to execute a plan so MoveThroughJointPositions is tested. I also have used the control tab on app.viam to move the robot and have confirmed that functionality remains for JointPositions and MoveToJointPositions.
  2. Yesterday (11.20.24) I ran a golang script which used EndPosition and MoveToPosition and confirmed that functionality remains.
  3. on app.viam in miko's office/xarm-module-testing I have also setup a xArm6 robot which uses the module. With this robot I have run both python and golang scripts to ensure that arm methods continue to function, however this was done back in September.

Testing I have not done:

  1. I have not tested this module on the xArm7 or the lite6 since I do not have access to them in the NYC office.

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Nov 21, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024
@@ -3,9 +3,9 @@ package register

import (
// register arms.
_ "github.com/viam-modules/viam-ufactory-xarm/arm"
Copy link
Member

Choose a reason for hiding this comment

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

This rather defeats the purpose of putting this in a module, no?

Copy link
Member

@biotinker biotinker left a comment

Choose a reason for hiding this comment

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

  1. arm registration should not attempt to register the module; the module itself should take care of that.
  2. I believe the order of operations described is incorrect. Presumably we will want to migrate all user configs to the module, then merge this, rather than the other way around, no?
  3. please detail testing done on this module

@nfranczak
Copy link
Member Author

  1. arm registration should not attempt to register the module; the module itself should take care of that.

My fault, will remove.

  1. I believe the order of operations described is incorrect. Presumably we will want to migrate all user configs to the module, then merge this, rather than the other way around, no?

If I run the migration and also remove the xArm RDK object today, then users can still continue to choose the xArm RDK object when setting up an arm up until Tuesday since the version of RDK that app.viam uses is not aware that the xArm RDK object no longer exists.

  1. please detail testing done on this module

Will update the PR's readme.

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Nov 21, 2024
@@ -8,11 +8,11 @@ import (
"sync"

"github.com/pkg/errors"
xarm "github.com/viam-modules/viam-ufactory-xarm/arm"
Copy link
Member

Choose a reason for hiding this comment

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

We should not be importing modules into RDK. This makes our import graph extremely messy.

We can replicate the imported function locally and use the test files you added.

@biotinker
Copy link
Member

I think we should wait before merging this. Here's why:

  1. I should test this on the xarm7 and lite6 before we commit to it
  2. If the waiter bot is running this module, that means that this module can clearly coexist with RDK. Thus, I would propose the module be finalized, we run the migration script to move everyone over, while leaving this in place for the time being; this way, just in case something does go terribly wrong with migration, we have the RDK version as a backup. Then, if the initial migration goes smoothly, then the next release cycle we merge this, ship it, and run the migration script again.

That has the lowest chance of breaking anything.

Broadly this PR looks good code-wise modulo the one comment I had about not importing the module back into RDK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants