ros-industrial / siemens_experimental

Apache License 2.0
8 stars 6 forks source link

initial commit #2

Closed durovsky closed 9 years ago

durovsky commented 9 years ago

Initial commit

shaun-edwards commented 9 years ago

Do you plan to utilize an abstract base class for the device/controller like we discussed?

shaun-edwards commented 9 years ago

All done with the review, please respond to my comments above.

durovsky commented 9 years ago

Bottom comment (regarding in/out module data definitions): I don't like existing definitions of module data:

input_length_, input_state_, input_address_, input_data_ using new and pointers in cp1616_io_controller.cpp

and

in_data_, in_data_len_, in_data_iocs_, in_data_iops_ defined as fixed array in cp1616_io_device.h

I would like to made those definitions consistent in both modes and and more c++ style. I m thinking about turning it into std::vector but I m not sure if existing IO Base function will accept arguments hidden in std::vector. See for example page 48 for PNIO_data_read in IO Base documentation IO Base

What do you think? should I try to rewrite in/out module data into std::vector?

Abstract class - I removed it for now, but I hope that we will get back there soon

shaun-edwards commented 9 years ago

If I understand correctly, PNIO_data_read requires a pointer to the proper memory address. You should be able to pass a vector like so: &(my_vector[index]). This assumes that you are allocating all of your arrays and the underlying library doesn't do this for you. It's worth a try, but not a requirement.

shaun-edwards commented 9 years ago

Please commit your changes for the previous comments (it should clean up the discussion above).

durovsky commented 9 years ago

second commit includes all changes according to above discussion except "vectorization" of module data ...this is a goal for next commit

shaun-edwards commented 9 years ago

@durovsky, I'd like to approve this commit and worry about the vectorization with the next PR. Is this OK?

durovsky commented 9 years ago

Yes, it is OK