Add Interface Library and ROS2 Implementation for the MCM and MCM Interfaces#8
Add Interface Library and ROS2 Implementation for the MCM and MCM Interfaces#8zeerekahmad merged 15 commits intomainfrom
Conversation
27de7b5 to
4ca8510
Compare
troygibb
left a comment
There was a problem hiding this comment.
Couple structural changes that are necessary here. Interfaces look mostly good!
On top of the inline comments, IMO the promise structure feels leaky in the sygnal interface. There shouldn't be any business logic there in the frame sending and receiving, it should operate as a pure CAN send/receive library.
You can achieve the same promise based structure on a higher level class closer to the node, which also should have a lot broken out of it as well, keep the ros2 node as thin as possible so it's easier to test!
Speaking of, I don't see tests for this complicated promise based structure. I would highly highly recommend adding tests here, as it looks like the most brittle and error prone part of this feature.
sygnal_can_interface/sygnal_can_interface_ros2/launch/sygnal_can_interface_launch.py
Show resolved
Hide resolved
sygnal_can_interface/sygnal_can_interface_ros2/src/sygnal_can_interface_node.cpp
Outdated
Show resolved
Hide resolved
sygnal_can_interface/sygnal_can_interface_ros2/src/sygnal_can_interface_node.cpp
Show resolved
Hide resolved
I'll add some tests for it! However, I think the socketcan layer is the exact layer where the promise logic should live. It is actually very lightweight and decreases how error prone the code can be by adding a robust and standard system for expecting replies from commands. It's also what ROS2 uses under the hood! A promise is basically a wrapper that gets filled by an object on receipt and has robust and helpful functions such as waits and timeouts built in. A caller can send something and then wait for a response. The job of the can attached layer/socketcan layer is to send and receive. By returning a promise, it ties what's sent and the response together in a clean way directly where this logic exists rather than in an external location. Abstraction further for the point of cleanliness is unnecessary and adds complexity that is just not needed. |
Appreciate the thought into this, but structurally it would be a lot easier to test if you kept the socketcan piece as a pure message pass through / serialize/deserialize layer and put the promises structure one level above it. Managing all of the promise locks under load and resolving them is manifestly tricky logic. It should be tested well, and the only practical way to do that is to isolate it from the CAN dependency. The point is not cleanliness here, it's testability. |
|
After discussing in person, we are leaving the future promise behavior inside the socketcan portion. This is because the serialization and deserialization is effectively done in the object classes that wrap the DBCs. The socketcan portion does ONLY future promise and passes data into objects to deserialize and stores the data. |
Description
This MR adds a foundational library for interfacing with the Sygnal MCM system via C++ utilizing SocketCan Adapter. It follows the patterns of the MVEC library to expose a generic interface to the sygnal system while also publishing feedback and diagnostics from the MCM itself.
Highlighted Changes
Test Plan
Unit Tests