Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions sygnal_can_interface/sygnal_can_interface_lib/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ add_library(
src/crc8.cpp
src/sygnal_mcm_interface.cpp
src/sygnal_command_interface.cpp
src/sygnal_hpo_interface.cpp
src/sygnal_interface_socketcan.cpp
)

Expand Down Expand Up @@ -108,6 +109,22 @@ if(BUILD_TESTING)
WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
)

add_executable(sygnal_hpo_interface_tests
test/sygnal_hpo_interface_test.cpp
)
target_link_libraries(sygnal_hpo_interface_tests
PRIVATE ${PROJECT_NAME} sygnal_dbc::sygnal_dbc Catch2::Catch2WithMain
)
ament_add_test(
sygnal_hpo_interface_tests
GENERATE_RESULT_FOR_RETURN_CODE_ZERO
COMMAND "$<TARGET_FILE:sygnal_hpo_interface_tests>"
-r junit -s
-o test_results/${PROJECT_NAME}/sygnal_hpo_interface_tests_output.xml
ENV CATCH_CONFIG_CONSOLE_WIDTH=120
WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
)

if(DEFINED ENV{CAN_AVAILABLE})
add_executable(sygnal_interface_socketcan_tests
test/sygnal_interface_socketcan_test.cpp
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
// Copyright (c) 2025-present Polymath Robotics, Inc. All rights reserved
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#ifndef SYGNAL_CAN_INTERFACE_LIB__SYGNAL_HPO_INTERFACE_HPP_
#define SYGNAL_CAN_INTERFACE_LIB__SYGNAL_HPO_INTERFACE_HPP_

#include <array>
#include <chrono>
#include <optional>
#include <string>

#include "socketcan_adapter/can_frame.hpp"

namespace polymath::sygnal
{

constexpr uint8_t HPO_NUM_INTERFACES = 7;

/// @brief Parsed HPO ControlEnableResponse / ControlCommandResponse.
/// is_enable_response disambiguates which payload is meaningful:
/// when true, the `enable` field carries the response; when false,
/// the `value` field carries the float command response.
struct HpoControlResponse
{
uint8_t message_id;
uint8_t bus_address;
double value;
bool enable;
bool is_enable_response;
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/// @brief Parsed HPO ErrorStatus frame fields.
struct HpoErrorStatus
{
uint8_t bus_address;
uint8_t subsystem_id;
uint8_t error_type;
uint16_t error_can_id;
uint8_t config_section_id;
uint8_t config_interface_id;
};

/// @brief Self-contained HPO board representation.
///
/// Owns the per-device interface bits (7 per-interface + 1 overall) and produces /
/// consumes the CAN frames the HPO understands. Frames are routed to the right
/// instance by `bus_address_`: every parse helper rejects frames that do not match
/// this device's bus address. Command frames are populated with `bus_address_`
/// automatically so callers cannot mismatch addresses.
///
/// Unlike the MCM, the HPO does not hold a Sygnal state machine. The per-interface
/// and overall-interface heartbeat signals are 1-bit booleans (true = HPO in control,
/// false = released). The MCM's SystemState byte is not tracked here.
class SygnalHpoInterface
{
public:
SygnalHpoInterface();
explicit SygnalHpoInterface(uint8_t bus_address);
~SygnalHpoInterface() = default;

/// @brief Try to parse a CAN frame as an HPO heartbeat addressed to this device.
/// @param frame Raw CAN frame.
/// @return true if the frame matched and internal state was updated.
bool parseHeartbeatFrame(const socketcan::CanFrame & frame);

/// @brief Try to parse a CAN frame as a ControlEnableResponse or ControlCommandResponse
/// 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);

@Ryanbahl9 Ryanbahl9 May 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@Ryanbahl9 Ryanbahl9 May 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I agree with Zeerek here. Refactor will save time in the long run

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Want me to start sketching out the refactor or is this something you want to own? @zeerekahmad / @Ryanbahl9

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zeerekahmad I'll get started then. We need to have something by today so we can move forward with the bringup/onsite verification.


/// @brief Try to parse a CAN frame as an ErrorStatus addressed to this device.
/// @param frame Raw CAN frame.
/// @return Populated HpoErrorStatus on success, std::nullopt otherwise.
std::optional<HpoErrorStatus> parseErrorFrame(const socketcan::CanFrame & frame);

/// @brief Build a ControlEnable frame for this device.
/// @param message_id 8-bit HPO MessageID.
/// @param enable true to grant HPO control, false to release.
/// @param[out] error_message Populated on failure.
/// @return Packed CAN frame on success.
std::optional<socketcan::CanFrame> createControlEnableFrame(
uint8_t message_id, bool enable, std::string & error_message);

/// @brief Build a ControlCommand frame for this device.
/// @param message_id 8-bit HPO MessageID.
/// @param value Command value (encoded as float per the DBC).
/// @param[out] error_message Populated on failure.
/// @return Packed CAN frame on success.
std::optional<socketcan::CanFrame> createControlCommandFrame(
uint8_t message_id, double value, std::string & error_message);

uint8_t get_bus_address() const
{
return bus_address_;
}

std::array<bool, HPO_NUM_INTERFACES> get_interface_states() const
{
return hpo_interface_states_;
}

bool get_overall_interface_state() const
{
return hpo_overall_interface_state_;
}

std::chrono::system_clock::time_point get_last_heartbeat_timestamp() const
{
return last_heartbeat_timestamp_;
}

private:
uint8_t bus_address_;
std::array<bool, HPO_NUM_INTERFACES> hpo_interface_states_;
bool hpo_overall_interface_state_;
std::chrono::system_clock::time_point last_heartbeat_timestamp_;
};

} // namespace polymath::sygnal

#endif // SYGNAL_CAN_INTERFACE_LIB__SYGNAL_HPO_INTERFACE_HPP_
Loading
Loading