Dynamixel SDK cpp RAII updates#681
Conversation
…ng with the new release. Enhancements include safer dynamic storage in C++ SDK and unified versioning.
…install targets for clarity.
….0 and SOVERSION 3 for ROS 2 shared library, and updated uninstall process to remove SDK-owned paths by name.
…local buffers, replacing them with stack-allocated vectors for improved memory management and performance.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request upgrades the Dynamixel SDK to version 5.0.0, transitioning from manual memory management to std::vector for group and packet buffers in both the standalone C++ SDK and ROS 2 wrappers. The update also includes modernizing casts to static_cast, replacing bzero with memset, and refactoring the CMake uninstall logic. Review feedback focuses on enhancing robustness by using std::vector::size() instead of manual length calculations during packet transmission, which prevents potential buffer over-reads if parameter population is incomplete.
…r for improved input handling.
…ite classes to prevent transmission errors. Return COMM_TX_ERROR for invalid parameter sizes before executing write operations.
…lecting the new release. Changes include enhancements in the C++ SDK and consistent versioning across C, C++, Python, and ROS 2 packages.
…to feature-cpp-raii
Jack-Punter
left a comment
Jack-Punter
left a comment
There was a problem hiding this comment.
I just came to take a look at to see how these functions where working for something and one things thats bugged me when i've looked before is the use of dynamic memory, espicially in the read/write paths which in practice are likely to be used in hot-loops in controllers where people might desire some real-time guarantees. I noticed this PR, thought i'd have a look and see whats being changed.
I haven't done a thorough review, just looking at some of the places I was interested in in my investigation and imo theres room to remove some of the allocations that are going on here, especially in the "fast" read paths. While your changes definitely improve the situation by only doing one allocaiton per instance, I think itd be better to avoid the allocation and indirection entierly.
| if (rxpacket_.size() < static_cast<size_t>(RXPACKET_MAX_LEN)) | ||
| rxpacket_.resize(static_cast<size_t>(RXPACKET_MAX_LEN)); | ||
| uint8_t *rxpacket = rxpacket_.data(); |
There was a problem hiding this comment.
While this is definitely better than doing a malloc/free evey time you read from the bus, im of the opinion it would be better to avoid memory allocations and the additional branch (even if it will only trigger once) in these methods all together. Especially as they're likely to be used in hot-loops where real-time considerations might need to be made.
RXPACKET_MAX_LEN is only 250 bytes so this could easily just be a static array either as a member or just as a local.
| if (rxpacket_.size() < static_cast<size_t>(RXPACKET_MAX_LEN)) | ||
| rxpacket_.resize(static_cast<size_t>(RXPACKET_MAX_LEN)); | ||
| uint8_t *rxpacket = rxpacket_.data(); |
There was a problem hiding this comment.
Same thing here and in the same 2 places in the ros folder.
2 participants
This PR refactors the C++ SDK and ROS 2 C++ wrapper internals to use RAII-based buffer management instead of manual
new[]/delete[]andmalloc/free. Dynamic packet, group read/write, and error buffers are migrated tostd::vector<uint8_t>, reducing manual memory handling and making ownership clearer across the packet handlers, group handlers, and fast read paths.In addition, the PR updates package and library versioning for the 5.0.0 release. The standalone C++ library and ROS 2 shared library now declare VERSION 5.0.0 and SOVERSION 3 to reflect the C++ ABI change caused by public class layout updates. Related version metadata was also aligned across C, C++, Python, documentation, Arduino metadata, and ROS package manifests/changelogs.
The PR also revises the standalone C++ uninstall flow. Instead of relying on install_manifest.txt, the uninstall script now removes SDK-owned install paths by name under the configured install prefix, including headers, libraries, CMake package files, and shared data such as the control table.