ros-industrial-attic / keyence_experimental

BSD 3-Clause "New" or "Revised" License
11 stars 14 forks source link

Add Keyence sensors descriptions #22

Closed VictorLamoine closed 7 years ago

VictorLamoine commented 7 years ago

LJ-V7080

roslaunch keyence_experimental test_lj_v7080.launch capture du 2017-03-22 17-20-08

LV-V7200

roslaunch keyence_experimental test_lj_v7200.launch lj-v7200

VictorLamoine commented 7 years ago

Looks good on my side. This is the opportunity to work with the same URDF models; that'll make extrinsic calibration easier for both of us!

@Jmeyer1292 what do you think?

Jmeyer1292 commented 7 years ago

I think your optical frames match the frames in which we get data. So if that's the goal then we are good to go. There is a camera system on these however and the optical frame is somewhere just inside the lense in the housing. I don't know if that matters.

VictorLamoine commented 7 years ago

The naming is probably not appropriate.

What about renaming: lj_v7200_optical_frame > lj_v7200_data_frame ?

When doing the extrinsic calibration we are looking for the transform between the tool and lj_v7200_optical_frame=lj_v7200_data_frame frames. The rest should not matter.

gavanderhoorn commented 7 years ago

I think I introduced the naming for the frames that are used (at least in the previous version of the code). Reason I chose _optical_frame was to model things similar to how the kinect and some other laser scanners do things in ROS. There is value in trying to follow (even de-facto) standards with this, so I would suggest not using _data_frame.

VictorLamoine commented 7 years ago

Ok let's keep it as is for consistency with other sensors. Then it's ready to be merged unless you have other remarks :)

gavanderhoorn commented 7 years ago

Placing the base_link outside the geometry is also slightly counter-intuitive to me. There are no real rules for this afaik, but convention seems to dictate that it should be the 'logical' base of the object your modelling.

There is also no special meaning attached to the frame itself - other than that it is considered the starting point or root of the model - and it would seem you've placed it at that specific location for a reason @VictorLamoine?

VictorLamoine commented 7 years ago

The base link is not in the URDF model. It is added for the test launch file/convenience: https://github.com/ros-industrial/keyence_experimental/pull/22/files#diff-43d0913b0b6da028826f9078c520e45fR5

Users would import the lj_v7xxx_macro.xacro macro, instantiate a LJ-V7xxx and then create a joint between the lj_v7xxx_optical_frame and whatever they want to place the sensor.

gavanderhoorn commented 7 years ago

@Jmeyer1292 wrote:

I think your optical frames match the frames in which we get data. So if that's the goal then we are good to go. There is a camera system on these however and the optical frame is somewhere just inside the lense in the housing.

_optical_frame is indeed often used to indicate the optical origin / centre of the lensing or sensor system in ROS RGB(D) sensor models. It is often also the origin of the coordinate system in which measurements (ie: pixels / points) are measured. That is actually the reason I chose to place it where it is, as the conversion from raw data to points in the cloud is relatively straightforward in that case (the keyence profiles are centered around 0 with the origin at the focal / reflexion point.

To make this correspond to how other sensors relate their measurements to _optical_frame, I placed it at the position it is now.

All of that is obviously relatively arbitrary, and the profile points could be easily transformed into other frames.

I would however suggest to keep close to other existing sensors/drivers, as I wrote above.

gavanderhoorn commented 7 years ago

@VictorLamoine wrote:

The base link is not in the URDF model. It is added in the test launch file for convenience

Users would import the lj_v7080_macro.xacro macro, instantiate a LJ-Vxxx and then create a joint between the lj_v7080_optical_frame and whatever they want to place the sensor.

Ok.

Perhaps consider adding a base_link, as it is by convention always the start of a ROS urdf model. In a way the base_link is then part of the 'public interface' of the xacro / model, and the scanner model is connected to a larger composite by attaching it using that link to some other external link.

The _optical_frame could then be a child of base_link.

VictorLamoine commented 7 years ago

When doing the extrinsic calibration we find the transformation between the tool and the lj_v7200_optical_frame. When done we update the transformation between those two frame and are ready to scan.

One idea would be to give the choice to the user to use lj_v7200_base_link, or lj_v7200_optical_frame; but I think this is not possible with XACRO/URDF; where are going to end with a non-attached link in the tree.

Did I mis-understand something you wanted to say Gijs?

gavanderhoorn commented 7 years ago

No, I had not considered the calibration aspect here.

My initial reaction would be to back transform the calibration results so that you update the transform between whatever attachment point you use outside the sensor (ie: tool) and lj_v7200_base_link, but I'm not sure how difficult that would be.

It would be the most re-usable way to model the sensor I think though.

Something we've done in the past is this:

  1. have a base_link, and use it as 'normal'
  2. have the TF frame that you make your measurements relative to not part of the URDF, but published by a driver or a static_transform_publisher

I'm not saying that this is what you should do here, but it could be an option.

VictorLamoine commented 7 years ago

The frames are NOT correct; don't merge as is. I'll give you proof

VictorLamoine commented 7 years ago

The X axis is inverted; I'll apply a 180° rotation around the Z axis to fix that.

Proof that the axis are wrong:

lv_navigator rviz_old_and_wrong picture

gavanderhoorn commented 7 years ago

See keyence_ljv_description/urdf/lj_v7080_macro.xacro for the 'old' model btw.

VictorLamoine commented 7 years ago
VictorLamoine commented 7 years ago

This is ready to be merged :) It's of course open to further modifications if needed!

gavanderhoorn commented 7 years ago

@VictorLamoine wrote:

I updated the screenshot in my first post

I think you've got the launch files and the screenshots mixed up :)

VictorLamoine commented 7 years ago

Ah yes, fixed!