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

support continuous joints #104

Merged
merged 2 commits into from
Aug 31, 2023
Merged

Conversation

atticusrussell
Copy link
Contributor

Cylindrical and Revolute Onshape mates with "continuous" or "wheel" in the name are created as continuous joints. Addresses part of #99.

  • docs updated for new feature (also fixed existing syntax error with bullets in docs)
  • removed warning about missing joint limits for continuous, because not applicable.

I tested and confirmed the functionality of my changes using a known good Onshape assembly.

@atticusrussell
Copy link
Contributor Author

Unsure how to properly request a review. @Gregwar does this look appropriate to merge?

@DevnathNair
Copy link

@atticusrussell Additionaly, It'd be cleaner if the continuous keyword was removed from the link name while writing to urdf/sdf in cases wherein they aren't strictly wheels.
For example for a joint named dof_continuous_c_bore_l would be rendered as c_bore_l in the urdf/sdf.

@Gregwar
Copy link
Contributor

Gregwar commented Aug 31, 2023

Hello, sorry to be late here!

The change seems good but I agree with @DevnathNair that the property should not be in the dof names.
Moreover, testing for it to be anywhere on the name might be dangerous

If we merge this, we would have 3 "modifiers" on the DOFs:

  • inv to invert the sign
  • speed to require onshape to robot bullet to use speed control (I think there are little users to this features )
  • continous or wheel

So we could have dof_shaft1_inv_continuous_speed, which seems really cumbersome

Maybe those could move to the config.json file

"joints": {
   "shaft1": {
      "invert": true,
      "continuous": true
   }
}

This could also be tweaked to merge this configuration so that custom joint limits or max efforts could be overwritten here. Maybe this is out of the scope, we can merge and open another issue to update the way it is handled in a future version.

What are your thoughts?

@DevnathNair
Copy link

@Gregwar I agree.
Although it's very convenient to specify the type in the name of the link itself it'll become a code smell as more link specific overrides are added.

However, I suggest we keep the wheel keyword as it is both readable and functional. (I can't think of any case where a wheel link wouldn't be continuous).

As far as I can tell, we should be able to let dof_ and _inv live since they're at the first and last of the name. Anything else as you rightly suggested should be in the config.json.

Maybe a "joint_group" key could be added to the parser and config.json to assign properties en-masse like so:

{
    "joints": {
        "shaft1": {
            "invert": true,
            "continuous": true
        },
        "shaft2": {},
        "shaft3": {}
    },
    "joint_group": {
        "joints": ["shaft1", "shaft2", "shaft3"],
        "properties": {
            "continuous": true
        }
    }
}

@Gregwar
Copy link
Contributor

Gregwar commented Aug 31, 2023

Maybe another way to achieve this would be to have a "default" entry in the joints list that defines the fallback values for each joints:

{
  "joints": {
    "default": {
        "continous": true
    },
    "joint1": {
        "invert": true
    }
  }
}

@Gregwar
Copy link
Contributor

Gregwar commented Aug 31, 2023

Anyway, the PR is harmless, let's merge and discuss this in another thread

@Gregwar Gregwar merged commit 93b8934 into Rhoban:master Aug 31, 2023
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.

None yet

3 participants