From 9035f406e3882089a37924a4755d92854f1f32f0 Mon Sep 17 00:00:00 2001 From: Adrian Del Grosso <10929341+ad3154@users.noreply.github.com> Date: Wed, 11 Oct 2023 19:30:01 -0500 Subject: [PATCH 1/3] Updates from main repo Brought over the latest bugfixes and features from the main repo. Fixes include: resolved a possible crash with ETP reception, fixed some log statements, fixed element number deserialization in DDOPs. New features include: Address violation handling. --- src/can_address_claim_state_machine.cpp | 11 +++++++++ src/can_address_claim_state_machine.hpp | 5 ++++ src/can_extended_transport_protocol.cpp | 2 +- src/can_internal_control_function.cpp | 5 ++++ src/can_internal_control_function.hpp | 4 +++ src/can_network_manager.cpp | 26 ++++++++++++++++++++ src/can_network_manager.hpp | 14 +++++++++++ src/isobus_device_descriptor_object_pool.cpp | 11 +++++---- src/isobus_diagnostic_protocol.cpp | 18 ++++++++++++++ src/isobus_diagnostic_protocol.hpp | 9 ++++--- 10 files changed, 96 insertions(+), 9 deletions(-) diff --git a/src/can_address_claim_state_machine.cpp b/src/can_address_claim_state_machine.cpp index bf1605e..8342127 100644 --- a/src/can_address_claim_state_machine.cpp +++ b/src/can_address_claim_state_machine.cpp @@ -44,6 +44,17 @@ namespace isobus return m_currentState; } + void AddressClaimStateMachine::on_address_violation() + { + if (State::AddressClaimingComplete == get_current_state()) + { + CANStackLogger::warn("[AC]: Address violation for address %u", + get_claimed_address()); + + set_current_state(State::SendReclaimAddressOnRequest); + } + } + void AddressClaimStateMachine::process_commanded_address(std::uint8_t commandedAddress) { if (State::AddressClaimingComplete == get_current_state()) diff --git a/src/can_address_claim_state_machine.hpp b/src/can_address_claim_state_machine.hpp index 5329bde..751aee6 100644 --- a/src/can_address_claim_state_machine.hpp +++ b/src/can_address_claim_state_machine.hpp @@ -56,6 +56,11 @@ namespace isobus /// @returns The current state of the state machine State get_current_state() const; + /// @brief Used to inform the address claim state machine that two CFs are using the same source address. + /// This function may cause the state machine to emit an address claim depending on its state, as is + /// required by ISO11783-5. + void on_address_violation(); + /// @brief Attempts to process a commanded address. /// @details If the state machine has claimed successfully before, /// this will attempt to move a NAME from the claimed address to the new, specified address. diff --git a/src/can_extended_transport_protocol.cpp b/src/can_extended_transport_protocol.cpp index ec9d610..a594923 100644 --- a/src/can_extended_transport_protocol.cpp +++ b/src/can_extended_transport_protocol.cpp @@ -295,7 +295,7 @@ namespace isobus (StateMachineState::RxDataSession == tempSession->state) && (messageData[SEQUENCE_NUMBER_DATA_INDEX] == (tempSession->lastPacketNumber + 1))) { - for (std::uint8_t i = SEQUENCE_NUMBER_DATA_INDEX; (i < PROTOCOL_BYTES_PER_FRAME) && ((PROTOCOL_BYTES_PER_FRAME * tempSession->lastPacketNumber) + i < tempSession->get_message_data_length()); i++) + for (std::uint8_t i = SEQUENCE_NUMBER_DATA_INDEX; (i < PROTOCOL_BYTES_PER_FRAME) && (((PROTOCOL_BYTES_PER_FRAME * tempSession->processedPacketsThisSession) + i) < tempSession->get_message_data_length()); i++) { std::uint32_t currentDataIndex = (PROTOCOL_BYTES_PER_FRAME * tempSession->processedPacketsThisSession) + i; tempSession->sessionMessage.set_data(messageData[1 + SEQUENCE_NUMBER_DATA_INDEX + i], currentDataIndex); diff --git a/src/can_internal_control_function.cpp b/src/can_internal_control_function.cpp index b2fed6f..67fa0c7 100644 --- a/src/can_internal_control_function.cpp +++ b/src/can_internal_control_function.cpp @@ -42,6 +42,11 @@ namespace isobus return ControlFunction::destroy(expectedRefCount); } + void InternalControlFunction::on_address_violation(CANLibBadge) + { + stateMachine.on_address_violation(); + } + void InternalControlFunction::process_commanded_address(std::uint8_t commandedAddress, CANLibBadge) { stateMachine.process_commanded_address(commandedAddress); diff --git a/src/can_internal_control_function.hpp b/src/can_internal_control_function.hpp index 64293c3..ed175c9 100644 --- a/src/can_internal_control_function.hpp +++ b/src/can_internal_control_function.hpp @@ -51,6 +51,10 @@ namespace isobus /// @param[in] CANPort The CAN channel index for this control function to use InternalControlFunction(NAME desiredName, std::uint8_t preferredAddress, std::uint8_t CANPort, CANLibBadge); + /// @brief Used to inform the member address claim state machine that two CFs are using the same source address. + /// @note Address violation occurs when two CFs are using the same source address. + void on_address_violation(CANLibBadge); + /// @brief Used by the network manager to tell the ICF that the address claim state machine needs to process /// a J1939 command to move address. void process_commanded_address(std::uint8_t commandedAddress, CANLibBadge); diff --git a/src/can_network_manager.cpp b/src/can_network_manager.cpp index 5584f55..0a7ef28 100644 --- a/src/can_network_manager.cpp +++ b/src/can_network_manager.cpp @@ -392,6 +392,11 @@ namespace isobus return configuration; } + EventDispatcher> &CANNetworkManager::get_address_violation_event_dispatcher() + { + return addressViolationEventDispatcher; + } + bool CANNetworkManager::add_protocol_parameter_group_number_callback(std::uint32_t parameterGroupNumber, CANLibCallback callback, void *parentPointer) { bool retVal = false; @@ -840,6 +845,26 @@ namespace isobus } } + void CANNetworkManager::process_can_message_for_address_violations(const CANMessage ¤tMessage) + { + auto sourceAddress = currentMessage.get_identifier().get_source_address(); + + if ((BROADCAST_CAN_ADDRESS != sourceAddress) && + (NULL_CAN_ADDRESS != sourceAddress)) + { + for (auto &internalCF : internalControlFunctions) + { + if ((nullptr != internalCF) && + (internalCF->get_address() == sourceAddress) && + (currentMessage.get_can_port_index() == internalCF->get_can_port())) + { + internalCF->on_address_violation({}); + addressViolationEventDispatcher.call(internalCF); + } + } + } + } + void CANNetworkManager::process_control_function_state_change_callback(std::shared_ptr controlFunction, ControlFunctionState state) { #if !defined CAN_STACK_DISABLE_THREADS && !defined ARDUINO @@ -937,6 +962,7 @@ namespace isobus CANMessage currentMessage = get_next_can_message_from_rx_queue(); update_address_table(currentMessage); + process_can_message_for_address_violations(currentMessage); // Update Special Callbacks, like protocols and non-cf specific ones process_protocol_pgn_callbacks(currentMessage); diff --git a/src/can_network_manager.hpp b/src/can_network_manager.hpp index 3c1ec54..526beb4 100644 --- a/src/can_network_manager.hpp +++ b/src/can_network_manager.hpp @@ -24,6 +24,7 @@ #include "can_network_configuration.hpp" #include "can_transport_protocol.hpp" #include "nmea2000_fast_packet_protocol.hpp" +#include "event_dispatcher.hpp" #include #include @@ -175,6 +176,11 @@ namespace isobus /// @returns The configuration class for this network manager CANNetworkConfiguration &get_configuration(); + /// @brief Returns the network manager's event dispatcher for notifying consumers whenever an + /// address violation occurs involving an internal control function. + /// @returns An event dispatcher which can be used to get notified about address violations + EventDispatcher> &get_address_violation_event_dispatcher(); + protected: // Using protected region to allow protocols use of special functions from the network manager friend class AddressClaimStateMachine; ///< Allows the network manager to work closely with the address claiming process @@ -289,6 +295,13 @@ namespace isobus /// @param[in] currentMessage The message to process void process_any_control_function_pgn_callbacks(const CANMessage ¤tMessage); + /// @brief Validates that a CAN message has not caused an address violation. + /// If a violation is found, the network manager will notify the affected address claim state machine + /// to re-claim as is required by ISO 11783-5, and will attempt to activate a DTC that is defined in ISO 11783-5. + /// @note Address violation occurs when two CFs are using the same source address. + /// @param[in] currentMessage The message to process + void process_can_message_for_address_violations(const CANMessage ¤tMessage); + /// @brief Checks the control function state callback list to see if we need to call /// a control function state callback. /// @param[in] controlFunction The controlFunction whose state has changed @@ -361,6 +374,7 @@ namespace isobus std::list controlFunctionStateCallbacks; ///< List of all control function state callbacks std::vector globalParameterGroupNumberCallbacks; ///< A list of all global PGN callbacks std::vector anyControlFunctionParameterGroupNumberCallbacks; ///< A list of all global PGN callbacks + EventDispatcher> addressViolationEventDispatcher; // An event dispatcher for notifying consumers about address violations #if !defined CAN_STACK_DISABLE_THREADS && !defined ARDUINO std::mutex receiveMessageMutex; ///< A mutex for receive messages thread safety std::mutex protocolPGNCallbacksMutex; ///< A mutex for PGN callback thread safety diff --git a/src/isobus_device_descriptor_object_pool.cpp b/src/isobus_device_descriptor_object_pool.cpp index 39f8ec2..64add91 100644 --- a/src/isobus_device_descriptor_object_pool.cpp +++ b/src/isobus_device_descriptor_object_pool.cpp @@ -542,6 +542,7 @@ namespace isobus std::string deviceElementDesignator; std::uint16_t parentObject = static_cast(static_cast(binaryPool[9 + numberDesignatorBytes]) | (static_cast(binaryPool[10 + numberDesignatorBytes]) << 8)); std::uint16_t uniqueID = static_cast(static_cast(binaryPool[3]) | (static_cast(binaryPool[4]) << 8)); + std::uint16_t elementNumber = static_cast(binaryPool[7 + numberDesignatorBytes]) | (static_cast(binaryPool[8 + numberDesignatorBytes]) << 8); auto type = static_cast(binaryPool[5]); for (std::uint16_t i = 0; i < numberDesignatorBytes; i++) @@ -549,7 +550,7 @@ namespace isobus deviceElementDesignator.push_back(binaryPool[7 + i]); } - if (add_device_element(deviceElementDesignator, binaryPool[7 + numberDesignatorBytes], parentObject, type, uniqueID)) + if (add_device_element(deviceElementDesignator, elementNumber, parentObject, type, uniqueID)) { auto DETObject = std::static_pointer_cast(get_object_by_id(uniqueID)); @@ -975,8 +976,8 @@ namespace isobus } else if (task_controller_object::ObjectTypes::DeviceValuePresentation != child->get_object_type()) { - CANStackLogger::error("[DDOP]: Object %u has a child %u with an object type that is not allowed." + - currentProcessData->get_device_value_presentation_object_id(), + CANStackLogger::error("[DDOP]: Object %u has a child %u with an object type that is not allowed.", + currentProcessData->get_device_value_presentation_object_id(), child->get_object_id()); CANStackLogger::error("[DDOP]: A DPD object may only have DVP children."); retVal = false; @@ -1003,8 +1004,8 @@ namespace isobus } else if (task_controller_object::ObjectTypes::DeviceValuePresentation != child->get_object_type()) { - CANStackLogger::error("[DDOP]: Object %u has a child %u with an object type that is not allowed." + - currentProperty->get_device_value_presentation_object_id(), + CANStackLogger::error("[DDOP]: Object %u has a child %u with an object type that is not allowed.", + currentProperty->get_device_value_presentation_object_id(), child->get_object_id()); CANStackLogger::error("[DDOP]: A DPT object may only have DVP children."); retVal = false; diff --git a/src/isobus_diagnostic_protocol.cpp b/src/isobus_diagnostic_protocol.cpp index 53cb1b7..8992b8f 100644 --- a/src/isobus_diagnostic_protocol.cpp +++ b/src/isobus_diagnostic_protocol.cpp @@ -102,6 +102,7 @@ namespace isobus CANNetworkManager::CANNetwork.add_protocol_parameter_group_number_callback(static_cast(CANLibParameterGroupNumber::DiagnosticMessage22), process_message, this); CANNetworkManager::CANNetwork.add_protocol_parameter_group_number_callback(static_cast(CANLibParameterGroupNumber::DiagnosticMessage13), process_message, this); CANNetworkManager::CANNetwork.add_global_parameter_group_number_callback(static_cast(CANLibParameterGroupNumber::DiagnosticMessage13), process_message, this); + addressViolationEventHandle = CANNetworkManager::CANNetwork.get_address_violation_event_dispatcher().add_listener([this](std::shared_ptr affectedCF) { this->on_address_violation(affectedCF); }); if (auto requestProtocol = myControlFunction->get_pgn_request_protocol().lock()) { @@ -148,6 +149,7 @@ namespace isobus CANNetworkManager::CANNetwork.remove_protocol_parameter_group_number_callback(static_cast(CANLibParameterGroupNumber::DiagnosticMessage22), process_message, this); CANNetworkManager::CANNetwork.remove_protocol_parameter_group_number_callback(static_cast(CANLibParameterGroupNumber::DiagnosticMessage13), process_message, this); CANNetworkManager::CANNetwork.remove_global_parameter_group_number_callback(static_cast(CANLibParameterGroupNumber::DiagnosticMessage13), process_message, this); + addressViolationEventHandle.reset(); } } @@ -605,6 +607,22 @@ namespace isobus } } + void DiagnosticProtocol::on_address_violation(std::shared_ptr affectedControlFunction) + { + if ((nullptr != affectedControlFunction) && + (!get_j1939_mode()) && + (BROADCAST_CAN_ADDRESS != affectedControlFunction->get_address()) && + (NULL_CAN_ADDRESS != affectedControlFunction->get_address())) + { + constexpr std::uint32_t ADDRESS_VIOLATION_SPN_BASE = 2000; // Defined in ISO 11783-5 section 4.4.4.3 + + set_diagnostic_trouble_code_active(DiagnosticTroubleCode(ADDRESS_VIOLATION_SPN_BASE + affectedControlFunction->get_address(), + FailureModeIdentifier::ConditionExists, + LampStatus::None), + true); + } + } + bool DiagnosticProtocol::send_diagnostic_message_1() const { bool retVal = false; diff --git a/src/isobus_diagnostic_protocol.hpp b/src/isobus_diagnostic_protocol.hpp index e116c0d..93251df 100644 --- a/src/isobus_diagnostic_protocol.hpp +++ b/src/isobus_diagnostic_protocol.hpp @@ -391,9 +391,6 @@ namespace isobus /// @returns The two bit lamp state for CAN std::uint8_t convert_flash_state_to_byte(FlashState flash) const; - /// @brief A utility function that will clean up PGN registrations - void deregister_all_pgns(); - /// @brief This is a way to find the overall lamp states to report /// @details This searches the active DTC list to find if a lamp is on or off, and to find the overall flash state for that lamp. /// Basically, since the lamp states are global to the CAN message, we need a way to resolve the "total" lamp state from the list. @@ -410,6 +407,11 @@ namespace isobus /// @param[out] lampOn If the lamp state is on for any DTC void get_inactive_list_lamp_state_and_flash_state(Lamps targetLamp, FlashState &flash, bool &lampOn) const; + /// @brief A callback function used to consume address violation events and activate a DTC + /// as required in ISO11783-5. + /// @param[in] affectedControlFunction The control function affected by an address violation + void on_address_violation(std::shared_ptr affectedControlFunction); + /// @brief Sends a DM1 encoded CAN message /// @returns true if the message was sent, otherwise false bool send_diagnostic_message_1() const; @@ -488,6 +490,7 @@ namespace isobus static void process_flags(std::uint32_t flag, void *parentPointer); std::shared_ptr myControlFunction; ///< The internal control function that this protocol will send from + std::shared_ptr addressViolationEventHandle; ///< Stores the handle from registering for address violation events NetworkType networkType; ///< The diagnostic network type that this protocol will use std::vector activeDTCList; ///< Keeps track of all the active DTCs std::vector inactiveDTCList; ///< Keeps track of all the previously active DTCs From 691f8898a2cc2136f4a52a1a628da19914344a1b Mon Sep 17 00:00:00 2001 From: Adrian Del Grosso <10929341+ad3154@users.noreply.github.com> Date: Wed, 11 Oct 2023 19:31:04 -0500 Subject: [PATCH 2/3] Improved reliability of transmitting through the FlexCAN driver Asked the driver to use only 1 tx mailbox, and increased queue size to allow nearly two full ETP EDPO windows with max size. --- src/flex_can_t4_plugin.cpp | 11 ++++++----- src/flex_can_t4_plugin.hpp | 10 +++++----- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/flex_can_t4_plugin.cpp b/src/flex_can_t4_plugin.cpp index 26803a0..545f350 100644 --- a/src/flex_can_t4_plugin.cpp +++ b/src/flex_can_t4_plugin.cpp @@ -14,12 +14,12 @@ namespace isobus { #if defined(__IMXRT1062__) - FlexCAN_T4 FlexCANT4Plugin::can0; - FlexCAN_T4 FlexCANT4Plugin::can1; - FlexCAN_T4 FlexCANT4Plugin::can2; + FlexCAN_T4 FlexCANT4Plugin::can0; + FlexCAN_T4 FlexCANT4Plugin::can1; + FlexCAN_T4 FlexCANT4Plugin::can2; #elif defined(__MK20DX256__) || defined(__MK64FX512__) || defined(__MK66FX1M0__) - FlexCAN_T4 FlexCANT4Plugin::can0; - FlexCAN_T4 FlexCANT4Plugin::can1; + FlexCAN_T4 FlexCANT4Plugin::can0; + FlexCAN_T4 FlexCANT4Plugin::can1; #endif FlexCANT4Plugin::FlexCANT4Plugin(std::uint8_t channel) : @@ -104,6 +104,7 @@ namespace isobus message.id = canFrame.identifier; message.len = canFrame.dataLength; message.flags.extended = true; + message.seq = 1; // Try to get messages to go out sequentially... memcpy(message.buf, canFrame.data, canFrame.dataLength); if (0 == selectedChannel) diff --git a/src/flex_can_t4_plugin.hpp b/src/flex_can_t4_plugin.hpp index ecd5c6e..cf4ca47 100644 --- a/src/flex_can_t4_plugin.hpp +++ b/src/flex_can_t4_plugin.hpp @@ -49,12 +49,12 @@ namespace isobus private: #if defined(__IMXRT1062__) - static FlexCAN_T4 can0; - static FlexCAN_T4 can1; - static FlexCAN_T4 can2; + static FlexCAN_T4 can0; + static FlexCAN_T4 can1; + static FlexCAN_T4 can2; #elif defined(__MK20DX256__) || defined(__MK64FX512__) || defined(__MK66FX1M0__) - static FlexCAN_T4 can0; - static FlexCAN_T4 can1; + static FlexCAN_T4 can0; + static FlexCAN_T4 can1; #endif std::uint8_t selectedChannel; bool isOpen = false; From fbd1de33ebe410a55cbe55ad0a40b6125522e021 Mon Sep 17 00:00:00 2001 From: Adrian Del Grosso <10929341+ad3154@users.noreply.github.com> Date: Wed, 11 Oct 2023 19:50:22 -0500 Subject: [PATCH 3/3] [CI]: Update Arduino linting action settings --- .github/workflows/linting.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/linting.yml b/.github/workflows/linting.yml index 1c18b7b..cfc0966 100644 --- a/.github/workflows/linting.yml +++ b/.github/workflows/linting.yml @@ -5,3 +5,6 @@ jobs: steps: - uses: actions/checkout@v3 - uses: arduino/arduino-lint-action@v1 + with: + library-manager: update + project-type: library