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

[geneus] treat uint8[] as string like rospy #15

Merged
merged 2 commits into from
Apr 14, 2015

Conversation

furushchev
Copy link
Member

This is claimed at #14
Until merging this change, existing codes that treats messages containing uint8[] will fail.(e.g.: converting sensor_msgs/PointCloud2 to eus-pointcloud)

@garaemon
Copy link
Member

garaemon commented Apr 8, 2015

Does it mean we need to modify existing code?

@k-okada
Copy link
Member

k-okada commented Apr 8, 2015

it depends on what kind of code you write, where is the code that uses FC2OCSBasicInfoSmall.msg?
Before i rewrite geneus, uint8[] is convert to string, and at here b157cd5, I have change them tointeger-vector but that may not correct. at least it fails at https://github.com/jsk-ros-pkg/jsk_roseus/blob/master/roseus/roseus_c_util.c#L50

@garaemon
Copy link
Member

garaemon commented Apr 8, 2015

@garaemon
Copy link
Member

garaemon commented Apr 8, 2015

(elt <string> <index>) returns character constant and it may be no problem.

@k-okada
Copy link
Member

k-okada commented Apr 8, 2015

humm, if we convert uint to integer-vector

  • check integer-vector, not string in roseus_c_utils.c
  • previous string implementation is very efficient because we do not run dolist, but memcpy to fill the data in deserialize phase, can we use that technique to integer-vector?

@garaemon
Copy link
Member

garaemon commented Apr 8, 2015

Could you test roslaunch jsk_network_tools joint_state_compress_sample.launch Y_DIFF=1.0 before merging this?

If it work, I'm willing to accept this change.

@garaemon
Copy link
Member

garaemon commented Apr 8, 2015

jsk-ros-pkg/jsk_common#852

This pull request may be required to run that.

@k-okada
Copy link
Member

k-okada commented Apr 8, 2015

ok, please check this @furushchev

@furushchev
Copy link
Member Author

I tried to test joint_state_compress_sample.launch, and looked rviz, but the poses of robots("no TF prefix" and with "decompressed") are quite different each other.(I use PR2 on local)
Is this correct? If correct, how can I test?

@furushchev
Copy link
Member Author

Poses are different both on normal geneus environment and "this pullreq" geneus environment.

@garaemon
Copy link
Member

garaemon commented Apr 9, 2015

It should be almost same

2015年4月9日木曜日、Furushchevnotifications@github.comさんは書きました:

Poses are different both on normal geneus environment and "this pullreq"
geneus environment.


Reply to this email directly or view it on GitHub
#15 (comment).

✉︎ from iPhone

@furushchev
Copy link
Member Author

@garaemon I added test for angle vector compress/decompress jsk-ros-pkg/jsk_common#856.
I'll now check if occurs any problem using this test.
;; I could not check the diff of this pullreq using RViz, because poses are already different both on normal/"new" geneus environment.

@furushchev
Copy link
Member Author

I checked error in both env:

  • norm of difference between raw and decompressed angle-vector
    • 1.50817 (normal geneus env)
    • 1.50817 (new geneus env)
  • norm of difference between raw and decompressed angle-vector

in existing geneus env:

head_tilt_joint: 0.000615
head_pan_joint: -0.003425
r_wrist_roll_joint: 0.002261
r_wrist_flex_joint: 0.005686
r_forearm_roll_joint: -0.003865
r_elbow_flex_joint: 6.28319
r_upper_arm_roll_joint: 0.003725
r_shoulder_lift_joint: 6.28319
r_shoulder_pan_joint: 0.003425
l_wrist_roll_joint: 0.002261
l_wrist_flex_joint: -0.005686
l_forearm_roll_joint: -0.003865
l_elbow_flex_joint: 6.28319
l_upper_arm_roll_joint: 0.003725
l_shoulder_lift_joint: 6.28319
l_shoulder_pan_joint: 0.011204
torso_lift_joint: 0.001526
(norm diff): 12.5664
  • in new geneus env:
head_tilt_joint: -0.13585
head_pan_joint: -0.564602
r_wrist_roll_joint: -0.3536
r_wrist_flex_joint: -0.65
r_forearm_roll_joint: 0.003865
r_elbow_flex_joint: 1.30592
r_upper_arm_roll_joint: -0.540196
r_shoulder_lift_joint: 0.936318
r_shoulder_pan_joint: -0.790692
l_wrist_roll_joint: 1.2963
l_wrist_flex_joint: -1.54137
l_forearm_roll_joint: 0.98565
l_elbow_flex_joint: 4.97727
l_upper_arm_roll_joint: -0.540196
l_shoulder_lift_joint: 5.34687
l_shoulder_pan_joint: -0.011204
torso_lift_joint: 1.29626
(norm diff): 8.04649

the difference of joint_states are bigger in existing environment than new environment.

@furushchev
Copy link
Member Author

I got it, lift_joint are rotational joint? (2 * pi different?)
Anyway there is the difference among enviromnents... What should I do...

@garaemon
Copy link
Member

@garaemon
Copy link
Member

I will confirm this patch and please wait to merge this

2015年4月11日土曜日、Furushchevnotifications@github.comさんは書きました:

I got it, lift_joint are rotational joint? (2 * pi different?)
Anyway there is the difference among enviromnents... What should I do...


Reply to this email directly or view it on GitHub
#15 (comment).

✉︎ from iPhone

@furushchev
Copy link
Member Author

The problem is two:

  • lift_joint error is 2 * pi above when (de)compressed
  • (de)serialization of uint8[n](fixed length) does not work by new geneus change

I fixed and updated new geneus pullreq and now I think it's ok (all geneus test passed and joint_state_compressor returns the same values as on existing geneus environment.)

@furushchev
Copy link
Member Author

NOTE: Still please do not merge before the test ( jsk-ros-pkg/jsk_roseus#269 ) passes.

@furushchev
Copy link
Member Author

Sorry I mistook comment.
At first please merge this for testing message.l generated by new geneus:

After merging this PR, please re-run tests above to check

k-okada added a commit that referenced this pull request Apr 14, 2015
[geneus] treat uint8[] as string like rospy
@k-okada k-okada merged commit 9f0cc7c into jsk-ros-pkg:master Apr 14, 2015
@k-okada k-okada mentioned this pull request Apr 20, 2015
k-okada added a commit that referenced this pull request Apr 21, 2015
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.

3 participants