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 single panda #1799

Merged
merged 36 commits into from
Oct 30, 2023
Merged

add single panda #1799

merged 36 commits into from
Oct 30, 2023

Conversation

yuki-asano
Copy link
Contributor

単腕のpandaを追加したものです。

Screenshot from 2023-05-09 16-07-56

@k-okada k-okada requested a review from pazeshun May 9, 2023 13:29
jsk_panda_robot/panda_eus/CMakeLists.txt Outdated Show resolved Hide resolved
jsk_panda_robot/panda_eus/euslisp/panda-interface.l Outdated Show resolved Hide resolved
jsk_panda_robot/panda_eus/euslisp/panda-interface.l Outdated Show resolved Hide resolved
reset-pose: [ 0.0, -45.0, 0.0, -135.0, 0.0, 90.0, 45.0,
0.0, -75.0 ]
reset-manip-pose: [ 0.0, -45.0, 0.0, -135.0, 0.0, 90.0, 45.0,
0.0, -75.0 ]
Copy link
Contributor

Choose a reason for hiding this comment

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

reset-pose seems different from

reset-pose: [ 0.0, 15.0, 0.0, -135.0, 0.0, 150.0, 45.0,
0.0, 15.0, 0.0, -135.0, 0.0, 150.0, 45.0,
0.0, -75.0]

Also, reset-pose and reset-manip-pose are the same.
Do you have any reason why you choose this angle-vector as reset-pose and reset-manip-pose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reasons of reset pose is coming from:

  • less footprint
  • to avoid environment at initial setting
  • looks like kinematically less torque. (especially for joint 1)

In terms of using the same angle-vector in two poses, there is no deeper reasons.
I never used reset-manip-pose so far, so just set the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

In yuki-asano#1, I added notice about the difference of reset-pose and reset-manip-pose (and also end-coords) to README

Copy link
Contributor

Choose a reason for hiding this comment

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

After yuki-asano#3, pannda and dual_panda use the same end-coords.

@yuki-asano
Copy link
Contributor Author

@pazeshun Thank you for your detail review at first. I'll check your comments.

@k-okada k-okada requested a review from pazeshun May 21, 2023 05:33
@yuki-asano
Copy link
Contributor Author

@pazeshun chould you approve changes?

@pazeshun
Copy link
Contributor

pazeshun commented Jun 2, 2023

@pazeshun chould you approve changes?

I should not approve until CI passes (#1208 (comment)).
And currently, CI seems to fail due to apt server problem:

  + sudo apt-get install -y --force-yes -q -qq python-rosdep python-wstool python-catkin-tools
  W: --force-yes is deprecated, use one of the options starting with --allow instead.
  E: Package 'python-rosdep' has no installation candidate
  E: Package 'python-wstool' has no installation candidate
  E: Package 'python-catkin-tools' has no installation candidate

https://github.com/jsk-ros-pkg/jsk_robot/actions/runs/5150618620/jobs/9276407913?pr=1799
In this situation, we should wait for some time while sometimes restarting CI (too frequent restarting has a negative influence, though).
I'll take care about CI, so please wait.

@yuki-asano
Copy link
Contributor Author

OK. I missed the failed CI in the last. It is hidden and appears by scroling..

Copy link
Contributor

@pazeshun pazeshun left a comment

Choose a reason for hiding this comment

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

@k-okada
I think this PR is now OK, but I have one concern.
Currently, reset-pose, reset-manip-pose, and end-coords of panda are different from dual_panda:
https://github.com/jsk-ros-pkg/jsk_robot/pull/1799/files#diff-75f2fe8730d1b6c5ca7bf942abc99bf5955664a80692a4e7263428b14268a1c2R10-R19

rarm-end-coords:
parent: rarm_link7
translate: [0, 0, 0.2]
rotate : [-0.35740972270209, -0.86285515644477, -0.35740630816298, 98.42112728719206]
larm-end-coords:
parent: larm_link7
translate: [0, 0, 0.2]
rotate : [-0.35740972270209, -0.86285515644477, -0.35740630816298, 98.42112728719206]
angle-vector:
reset-pose: [ 0.0, 15.0, 0.0, -135.0, 0.0, 150.0, 45.0,
0.0, 15.0, 0.0, -135.0, 0.0, 150.0, 45.0,
0.0, -75.0]
reset-manip-pose: [ -29.862, 22.5706, 6.95363, -118.83, 69.8182, 77.5441, 82.0761,
37.506, 23.1119, -15.2759, -119.03, -67.9519, 80.0034, 7.1317,
0.0, -75.0 ]

Especially as for end-coords, I think panda's end-coords is better because it is on the center of the fingertip.
However, we already have some demo codes depending on dual_panda's end-coords, so it is not easy to change it.
I think it is OK to leave them as they are because I wrote a notice about them:
https://github.com/jsk-ros-pkg/jsk_robot/pull/1799/files#diff-e424322edffb86d671056c36f1dd1502df5529e99aedfd12f7cb3c101be5f36eR110
What do you think?

@pazeshun
Copy link
Contributor

@k-okada Kindly ping

@k-okada
Copy link
Member

k-okada commented Jun 15, 2023

@pazeshun sorry for being late,

However, we already have some demo codes depending on dual_panda's end-coords, so it is not easy to change it.

which demo uses this feature? I think we want to change the end coords near future(1-3 years), so how about add warning message to source code too,

(:end-coords (&rest args)
  (warning-message 2?3? (yeallow) "the endo coords of dual_pand is differnet from single panda and might be change in near future.)
  (send-super* :end-coords args))

in
https://github.com/jsk-ros-pkg/jsk_robot/pull/1799/files#diff-da9673f8d3480112a7dbd60e430a179c2df50bf7f12437ae7419bd9777e0f357
if this is not so difficult.

@pazeshun
Copy link
Contributor

which demo uses this feature?

For example, https://github.com/jsk-ros-pkg/jsk_demos/pull/1363/files#diff-e69875d71091c4531bb74bf07af5b703bc585828d41f8fea5aa8fbde9f4ef1e0R22.
Also I used end-coords directly in my previous demo: https://gitlab.jsk.imi.i.u-tokyo.ac.jp/hasegawa/hasegawa_moonshot/-/blob/7b71edf5a7cb1c6ada46214f5aafb8d03f9d6cfa/hasegawa_moonshot/euslisp/lib/drilling-interface.l#L64-L66.
Perhaps @haraduka may also use end-coords directly.

I think we want to change the end coords near future(1-3 years)

If you might allow this, then I think we should change end-coords right now, otherwise we will break too many demos because the number of demos will increase.
But just now, we do not have enough time to test previous demos with the new end-coords.
Do you OK to keep this PR open until testing finishes?
Or should we just add a warning message and merge this PR?
(But I'm afraid too many warning messages may be shown because :end-coords may be called everywhere. Also I may forget to fix end-coords...)

@yuki-asano
Copy link
Contributor Author

yuki-asano commented Jul 21, 2023

I prefer to this if this is allowed.

Or should we just add a warning message and merge this PR?

pazeshun and others added 2 commits August 4, 2023 13:11
Move end-coords of single panda and dual_panda to the center of the fingers
Copy link
Contributor

@pazeshun pazeshun left a comment

Choose a reason for hiding this comment

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

@k-okada
Now panda and dual_panda use the same end-coords:
yuki-asano#3 (comment)
Could you review this?

@haraduka and I confirmed that slightly-modified version of @haraduka 's demo code works with this PR.
I'll fix my old demo to work with this PR (it is easy because only small translation is needed).
jsk-ros-pkg/jsk_demos#1363 will also be affected and it is difficult to check because we need to move panda to old pedestals etc., so I just left a comment:
jsk-ros-pkg/jsk_demos#1363 (comment)

Copy link
Contributor

@pazeshun pazeshun left a comment

Choose a reason for hiding this comment

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

Sorry, this PR may break gripper movement methods.
I will check deeply and make a PR to fix.

Copy link
Contributor

@pazeshun pazeshun left a comment

Choose a reason for hiding this comment

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

@k-okada
Now gripper movement methods were fixed: yuki-asano#4 (comment)
Could you review this?

Why we could not find that bug at #1799 (review) is that we considered that strange gripper movement as a bug of gripper itself, which we have sometimes faced so far.

@k-okada
Copy link
Member

k-okada commented Oct 29, 2023

@pazeshun LGTM, as for yuki-asano#1 (comment), can we fix by
jsk-ros-pkg/jsk_roseus#746 ??

@k-okada k-okada added the hacktoberfest-accepted https://hacktoberfest.digitalocean.com/hacktoberfest-update label Oct 29, 2023
@pazeshun
Copy link
Contributor

@k-okada Thank you, I confirmed jsk-ros-pkg/jsk_roseus#746 fixes yuki-asano#1 (comment) even when :wait nil:

3.irteusgl$ send *ri* :get-start-grasp-result :rarm :wait nil
[ERROR] [1698643393.384314525]: [/franka_gripper/grasp] :get-result called without sending goal, returns null result
nil

Can we merge this PR as it is and consider jsk-ros-pkg/jsk_roseus#746 as further improvement?

@k-okada k-okada merged commit a63b5e3 into jsk-ros-pkg:master Oct 30, 2023
8 checks passed
@yuki-asano
Copy link
Contributor Author

Thank you!! @pazeshun @k-okada

@yuki-asano yuki-asano deleted the panda-devel-2 branch October 31, 2023 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted https://hacktoberfest.digitalocean.com/hacktoberfest-update panda dual franka robot in JSK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants