Conversation
- Add corresponding methods at Path class to allow paths with 16-bit long attribute id's. - Add corresponding methods at Session class to allow paths with 16-bit long attribute id's. - Tested successfully with an Ethernet/IP adapter device with attributes id's greater than 255. Main reference: - "CIP Common Specification" Volume1, Release 1.0. ControlNet International and Open DeviceNet Vendor Association. June 2001 (Appendix C, page C-7)
- Follows the work from previous commit to allow attribute id's with 16 bits - Add corresponding methods at Session class to allow paths with 16-bit long attribute id's. - Not yet tested with a device. Main reference: - "CIP Common Specification" Volume1, Release 1.0. ControlNet International and Open DeviceNet Vendor Association. June 2001 (Appendix C, page C-7)
|
Hi, CI has done its job! The build fails because there is an ambiguity with the headers of the added methods at Session class. To solve this ambiguity , in our application code we call the set/getSingleAttributeX() methods passing always the argument "attribute_id" as a typed constant (either USINT or UINT), instead of passing a single number, which causes ambiguity since the compiler does not know if the number is USINT or UINT. At path_test.cpp, as well as session_test, attribute_id arguments are passed as (untyped) numbers , so there is an ambiguity on which method to call. An option could be to implement these methods with templates, but from my point of view this approach is not readable at all, since the template argument would be the type of argument_id, instead of being the type of return/input value of a get/set. Moreover I think it will also result in a backwards API break. In summary, this PR causes a backward break with potential existing code, so I'm open to receive proposals on which could be the better way to accept 2-byte long attributes id, but without breaking old API. I'll carry on because I think this feature is important, otherwise there is a subset of EthernetIP devices that simply will not work. Sorry for the inconveniences, inputs are welcome, andreu |
| addLogicalClass(class_id); | ||
| addLogicalInstance(instance_id); | ||
| addLogicalAttribute(attribute_id); | ||
| //this->print(); |
There was a problem hiding this comment.
Please remove (here and elsewhere).
There was a problem hiding this comment.
yes, they can be removed. They were used for debugging.
| if ( (ii+1)%2 == 0 ) std::cout << " "; | ||
| } | ||
| std::cout << std::dec << std::endl; | ||
| } |
There was a problem hiding this comment.
I know the pattern thus far in this code was to write to cout and avoid an explicit ros dependency, but I'd rather just pull in ros/console.h and use the established logging framework— then everything can be left on as debug-level outputs and simply enabled at runtime as required.
Note also that rather than printing here, you can implement a stream operator, which allows the printing code to decide on logging level rather than hard-coding it in the print function. Eg:
std::ostream& operator<<(std::ostream& os, const Path& p)
{
os << "PATH ";
// for ...
return os;
}
And then later:
ROS_DEBUG_STREAM("Processing " << my_path);
Thoughts on this as an alternative approach?
There was a problem hiding this comment.
Good idea to overload the operator "<<"
Doing so, as far as I understand we keep the no-dependency on ros/console at this level of the library, but it can be used with ROS_xx_STREAM()
thanks.
There was a problem hiding this comment.
Another possibility if you want to log while avoiding an explicit ros/control.h include is to use console_bridge:
…int()) by overloading the operator << to a given ostream.
|
Hi Mike, as you suggested, I've implemented the overloading of <<, and removed the print() method. Now, we should focus on the core of this PR, which is to allow to work with devices with attribute_id's > 255, so requiring 2Bytes. Simco motor drive (Wittenstein) is an example of a device with attribute_id's in the range of [2300,3000] In the commits of this PR, Set/GetsingleAttributeX() methods are implemented at Session allowing 2Byte long attributes. Path has been also modified to allow ethernetip paths with 2Byte long attributes. The problem remains with tests with non-typed numbers (hardcoded numbers), which causes ambiguity between EIP_USINT and EIP_UINT versions. I modified all these tests locally, and they passed ok, but I didn't push them, since I know that this implementation could imply an API break with existing code elsewhere (e.g. omron_os32c), so I still wonder how to solve this issue. thanks andreu |
2 participants
This PR allows to operate with devices with 2-Byte long Attribute ID's, instead of just 1-Byte long attribute ID.
Further details at commit comments.
Thanks for reviewing it.