HPO handling first-pass implementation - #25
Conversation
…implementation deffered to a separate commit
| /// addressed to this device. | ||
| /// @param frame Raw CAN frame. | ||
| /// @return Populated HpoControlResponse on success, std::nullopt otherwise. | ||
| std::optional<HpoControlResponse> parseControlResponse(const socketcan::CanFrame & frame); |
There was a problem hiding this comment.
If the plan is to use SygnalHpoInterface in SygnalInterfaceSocketcan, then this parse func shouldn't live in here, it should live in SygnalControlInterface.
For context, SygnalInterfaceSocketcan uses a single SygnalControlInterface object (named control_interface_) to handle all the control and response messages.
But now we have a problem, Sygnal has overloaded single CAN IDs with multiple definitions depending on what device it's coming from / going to.
The following messages all need to be parsed differently depending if they are going to a MCM, IO, or HPO board:
- 353 ControlCommandResponse
- 97 ControlEnableResponse
- 352 ControlCommand
- 96 ControlEnable
@zeerekahmad As I see it we either need to come up with more advanced arbitration in SygnalControlInterface or refactor SygnalInterfaceSocketcan to not use a single unified SygnalControlInterface for all devices.
There was a problem hiding this comment.
My preference wouuld be to refactor the sygnalinterfacesocketcan to make it more like mvec and ssm controller.
Make each interface driver to a sygnal board into its own object. API into the sygnal interface socketcan doesn't need to change, but the implementation can be to create MCM, HPO, IO, and Relay objects tht handle their respective stuff.
Then in the object's parse function it can check the bus_id and reject if the CAN_ID or BUS_ID don't match (or in the case of MCM, can, bus or subsystem)
There was a problem hiding this comment.
The cleanest solution would be to ask Sygnal to not overload CAN IDs with multiple definitions, since that very much goes against best practices. But that would require either multiple version of this repo to support older MCM versions, or creating a plan to update existing Sygnal boards (which is a bit of a sticking point right now with the axiomatic problems)
If we don't want to ask Sygnal to fix the overloaded CAN IDs, then here are the 2 other refactor paths I can think of:
Option 1:
- ~~Get rid of SygnalControlCommandResponse class
- ~~SygnalHpoInterface should keep it's own parsing / packing of control commands
- ~~Refactor SygnalMcmInterface to own it's own parsing / packing of control commands
The parent SygnalInterfaceSocketcan passes each control cmd CAN frame to every MCM's & HPO's "parseControlResponce" function to see who owns it.
Option 2:
Refactor SygnalControlCommandResponse class to be aware of which CAN ID's belong to MCMs and which CAN ID's belong to HPOs, then have it dynamically parse the responses based on which device type send the message.SygnalHpoInterface deletes it's own parsing / packing of control commands.
Look at zeerek's comment above for the best way to refactor this
There was a problem hiding this comment.
I think I agree with Zeerek here. Refactor will save time in the long run
There was a problem hiding this comment.
Want me to start sketching out the refactor or is this something you want to own? @zeerekahmad / @Ryanbahl9
There was a problem hiding this comment.
for easy reference, here is are the over loaded message definitions for CAN ID 353 ControlCommandResponse:
MCM:
BO_ 353 ControlCommandResponse: 8 MCM
SG_ CRC : 56|8@1+ (1,0) [0|0] "" Vector__XXX
SG_ Value : 24|32@1- (1,0) [0|0] "" Vector__XXX
SG_ Count8 : 16|8@1+ (1,0) [0|0] "" Vector__XXX
SG_ InterfaceID : 13|3@1+ (1,0) [0|0] "" Vector__XXX
SG_ BusAddress : 0|7@1+ (1,0) [0|0] "" Vector__XXX
SG_ SubSystemID : 7|1@1+ (1,0) [0|0] "" Vector__XXX
IO:
BO_ 353 ControlCommandResponse: 8 MCM
SG_ CRC : 56|8@1+ (1,0) [0|0] "" Vector__XXX
SG_ Value : 24|32@1- (1,0) [0|0] "" Vector__XXX
SG_ Count8 : 16|8@1+ (1,0) [0|0] "" Vector__XXX
SG_ InterfaceID : 13|3@1+ (1,0) [0|0] "" Vector__XXX
SG_ BusAddress : 0|7@1+ (1,0) [0|0] "" Vector__XXX
HPO:
BO_ 353 ControlCommandResponse: 8 MCM
SG_ CRC : 56|8@1+ (1,0) [0|0] "" Vector__XXX
SG_ Value : 24|32@1- (1,0) [0|0] "" Vector__XXX
SG_ Count8 : 16|8@1+ (1,0) [0|0] "" Vector__XXX
SG_ MessageID : 8|8@1+ (1,0) [0|0] "" Vector__XXX
SG_ BusAddress : 0|7@1+ (1,0) [0|0] "" Vector__XXX
There was a problem hiding this comment.
MCM does the same thing, @eric-polymath you're welcome to take a stab at it! I can mock up a design doc by EOD at the most. I won't be able to get to it until sometime next week.
I'd say it can be a separate MR though with it's own issue that we can further discuss this topic on.
There was a problem hiding this comment.
I think we need to handle this on our side for now even if it means maintaining two repos / or manage within one repo with version tags as I don't see a path where we get compliance on not overloading the IDs at the moment.
There was a problem hiding this comment.
@zeerekahmad I'll get started then. We need to have something by today so we can move forward with the bringup/onsite verification.
| double value; | ||
| bool enable; | ||
| bool is_enable_response; | ||
| }; |
There was a problem hiding this comment.
This struct would also be a duplicate of this struct in SygnalControlInterface. So i'm not sure this should live here if we still want to use a unified SygnalControlInterface to send control commands to all sygnal boards.
But again, as per my last comment, we need to refactor SygnalControlInterface because they are overloading CAN IDs between DBC files
4 participants
First pass at HPO handling. Updated to account for new DBCs structure from sygnal for HPO boards at sygnal/sygnal_can_interface/database/hpo/ . Removed state handling for HPO boards as they do not hold state like MCMs do. Deferred work on sygnal_interface_socketcan and ROS2 node to a separate commit.
Design guidance: https://www.notion.so/polymathrobotics/Engineering-Design-Documents-120c0b1ac5fa80dba603d41cdd11ed7e?p=366c0b1ac5fa804a82aeeae1a36375b5&pm=s&utm_content=366c0b1a-c5fa-804a-82ae-eae1a36375b5&utm_campaign=T02C9VDRBDJ&n=slack&n=slack_link_unfurl&pvs=6