search

jsk-ros-pkg / geneus

3 stars 16 forks source link

Non-fixed bool array geneus seems to be incorrect #38

Closed snozawa closed 9 years ago

snozawa commented 9 years ago

Non-fixed bool array geneus seems to be incorrect. I found this problem in hrpsys_ros_bridge conversion.

# Generate tempolary msg file
echo "bool data" > /tmp/TmpBool.msg
echo "bool[] data" > /tmp/TmpBoolArray.msg
echo "bool[2] data" > /tmp/TmpFixedBoolArray.msg
# I renamed data name as "data".

# Run geneus ( tempolarily in roseus package)
rosrun geneus gen_eus.py -p roseus -o /tmp -Iroseus:`rospack find roseus`/msg -Istd_msgs:`rospack find std_msgs`/msg /tmp/TmpBool.msg
rosrun geneus gen_eus.py -p roseus -o /tmp -Iroseus:`rospack find roseus`/msg -Istd_msgs:`rospack find std_msgs`/msg /tmp/TmpBoolArray.msg
rosrun geneus gen_eus.py -p roseus -o /tmp -Iroseus:`rospack find roseus`/msg -Istd_msgs:`rospack find std_msgs`/msg /tmp/TmpFixedBoolArray.msg

In generated .l files,

  (:deserialize
   (buf &optional (ptr- 0))
   ;; bool _tmpbool                                                                                                                                                                                                                      
     (setq _tmpbool (not (= 0 (sys::peek buf ptr- :char)))) (incf ptr- 1)
   ;;                                                                                                                                                                                                                                    
   self)

This means TmpBool.l and TmpFixedBoolArray.l returns t or nil and works correctly.

  (:deserialize
   (buf &optional (ptr- 0))
   ;; bool[] _tmpboolarray                                                                                                                                                                                                               
   (let (n)
     (setq n (sys::peek buf ptr- :integer)) (incf ptr- 4)
     (setq _tmpboolarray (let (r) (dotimes (i n) (push (instance object :init) r)) r))
     (dolist (elem- _tmpboolarray)
     (setq elem- (not (= 0 (sys::peek buf ptr- :char)))) (incf ptr- 1)
     ))
   ;;                                                                                                                                                                                                                                    
   self)

In this code,

     (dolist (elem- _tmpboolarray)
     (setq elem- (not (= 0 (sys::peek buf ptr- :char)))) (incf ptr- 1)

this setq does not overwrite _tmpboolarray elements.
So, this program does not return t or nil and returns object class instance.
Is this expected behavior?

Thanks in advance.

snozawa commented 9 years ago

To summarize,

k-okada commented 9 years ago

Thanks , could you describe that situation into Test code? https://github.com/jsk-ros-pkg/jsk_roseus/blob/master/roseus/test/test-geneus.test

◉ Kei Okada

2015/07/01 5:17、Shunichi Nozawa notifications@github.com のメッセージ:

To summarize,

Bool and fixed bool array seem to work correctly. Only bool non-fixed array seems not to work correctly. — Reply to this email directly or view it on GitHub.

snozawa commented 9 years ago

I see. How can we check new msg file? Can we add new msg file to roseus/msg just for test code?

# BoolArray.msg
bool[] data
# FixedBoolArray.msg
bool[2] data
snozawa commented 9 years ago

I created PR: https://github.com/jsk-ros-pkg/jsk_roseus/pull/330

k-okada commented 9 years ago

I have fixed this problem at https://github.com/jsk-ros-pkg/geneus/pull/39

please check https://github.com/snozawa/jsk_roseus/pull/1, this will change bool array from array to list

k-okada commented 9 years ago

what's the difference between test_for_bool_array and test_for_bool_array_new, I was working on test_for_bool_array_new, without knowing https://github.com/jsk-ros-pkg/jsk_roseus/pull/330 uses test_for_bool_array.

Should I fixed to use test_for_bool_array?

or if test_for_bool_array_new is ok, please create PR using this branch > @snozawa

snozawa commented 9 years ago

test_for_bool_array_new was temporary branch and test_for_bool_array is correct. Could you apply your change to tesT_for_bool_array instead of tesT_for_bool_array_new? (Sorry...)

k-okada commented 9 years ago

humm, if the difference between array and array_new is just a msg file name, then I prefer FixedArray and VariableArray, how do you think?

◉ Kei Okada

On Tue, Jul 7, 2015 at 10:47 PM, Shunichi Nozawa notifications@github.com wrote:

test_for_bool_array_new was temporary branch and test_for_bool_array is correct. Could you apply your change to tesT_for_bool_array instead of tesT_for_bool_array_new? (Sorry...)

— Reply to this email directly or view it on GitHub https://github.com/jsk-ros-pkg/geneus/issues/38#issuecomment-119206595.

snozawa commented 9 years ago

Do you mean FixedArray and VariableArray are better than FixedArrayForTest and VariableArrayForTest?

k-okada commented 9 years ago

Yes

◉ Kei Okada

2015/07/07 23:04、Shunichi Nozawa notifications@github.com のメッセージ:

Do you mean FixedArray and VariableArray are better than FixedArrayForTest and VariableArrayForTest?

— Reply to this email directly or view it on GitHub.

snozawa commented 9 years ago

Renaming FixedArray and VariableArray is based on your suggestion: https://github.com/jsk-ros-pkg/jsk_roseus/pull/330#issuecomment-118087514 I thought your comment was that FixedArray and VariableArray is too general and if we'd like to use these msgs just for rostest adding Test or something to msg file name is reasonable.

k-okada commented 9 years ago

Yeah, but if I compare FixedArray.msg and FixedArrayForTest.msg, I prefer the former one. Sorry about that

◉ Kei Okada

On Tue, Jul 7, 2015 at 11:36 PM, Shunichi Nozawa notifications@github.com wrote:

Renaming FixedArray and VariableArray is based on your suggestion: jsk-ros-pkg/jsk_roseus#330 (comment) https://github.com/jsk-ros-pkg/jsk_roseus/pull/330#issuecomment-118087514 I thought your comment was that FixedArray and VariableArray is too general and if we'd like to use these msgs adding Test or something to msg file name is reasonable.

— Reply to this email directly or view it on GitHub https://github.com/jsk-ros-pkg/geneus/issues/38#issuecomment-119223467.

snozawa commented 9 years ago

Yes, I agree with it.