kroshu / kuka_drivers

Repository containing ROS2 drivers for multiple KUKA robots
Apache License 2.0
78 stars 23 forks source link

Feature/rsi driver #99

Closed Svastits closed 9 months ago

gavanderhoorn commented 1 year ago

Out of curiosity: what's the rationale behind the custom XML parsing/encoding/extraction code?

altex111 commented 1 year ago

Out of curiosity: what's the rationale behind the custom XML parsing/encoding/extraction code?

Because of the real-time nature of the communication we needed a real time xml parser, witch do not have any heap allocation in it. We did not found any witch fitted thees criteria, because they allocate memory while deserialize. So i had to write my own.

gavanderhoorn commented 1 year ago

Because of the real-time nature of the communication we needed a real time xml parser, witch do not have any heap allocation in it. We did not found any witch fitted thees criteria, because they allocate memory while deserialize. So i had to write my own.

Ah, ok.

If you're mostly worried about heap allocations, could using something like TinyXML with TLSF be an option? Could save you from maintaining your own parser. It won't do anything for control flow, but at least allocation should be bounded in time that way.

PS: I never really understood why KUKA chose to use XML for the RSI payload, especially because of the real-time requirements for everything. A binary protocol payload would have been much easier.


Edit: I've also seen people not use an xml parser at all (well, in the RT critical phase that is), by just assuming a certain structure for the incoming messages, treating it all as a single string, and using a pointer directly into that string to pass floats and ints to functions like strtod(..) and strtol(..).

In that approach you completely ignore the fact its XML, and just "pick out" the parts of the string you're interested in.

Same for command messages: treat XML as a string template, and replace the required parts with something like snprintf(pointer_to_where_in_the_xml_str_a_double_needs_to_go, DOUBLE_SIZE_IN_RSI_XML, "%8.3d", my_double_var_).

Not saying you should do this here, but I thought it was an interesting approach that avoids parsing XML completely.

Svastits commented 1 year ago

You should remove all code that is not necessary for the reworked driver (e.g. RSIState class)

sonarcloud[bot] commented 1 year ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 58 Code Smells

0.0% 0.0% Coverage
4.4% 4.4% Duplication

warning The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17. Read more here

Svastits commented 9 months ago

The enhancements implemented in this PR will be implemented in the kuka-external-control-sdk instead

gavanderhoorn commented 9 months ago

@Svastits: just so we understand correctly: RSI support is still planned, but the implementation strategy will be different?

Does #144 already work to add RSI support?

Svastits commented 9 months ago

Exactly, we still plan to improve RSI support: we will create a non-ROS2 client library/SDK, that we will use in the ROS2 driver.

144 is only slightly related to this effort: the SDK is planned to have the same interface as the current kuka-external-control-sdk, which currently only support iiQKA cobots. Implementation of the RSI improvements is already ongoig, I can share more details in email :)

gavanderhoorn commented 9 months ago

Implementation of the RSI improvements is already ongoig, I can share more details in email

I'd be interested to learn more about this.

Especially about the current state of the RSI integration.


Edit: FYI: @andreflorindo.