From d2bfc473b2402e3aaf34f723d1e38add4fe34e2b Mon Sep 17 00:00:00 2001 From: Daan Steenbergen Date: Mon, 31 Jul 2023 11:37:00 +0200 Subject: [PATCH] [RP]: Make PGN request protocol member of ICF --- examples/diagnostic_protocol/main.cpp | 3 +-- examples/pgn_requests/main.cpp | 6 ++---- .../isobus/can_internal_control_function.hpp | 11 +++++++++++ ...n_parameter_group_number_request_protocol.hpp | 11 ++++++++--- .../isobus/isobus/isobus_diagnostic_protocol.hpp | 3 --- .../isobus/isobus/isobus_functionalities.hpp | 4 +--- isobus/src/can_internal_control_function.cpp | 16 ++++++++++++++++ ...n_parameter_group_number_request_protocol.cpp | 6 +++--- isobus/src/isobus_diagnostic_protocol.cpp | 15 +++++---------- isobus/src/isobus_functionalities.cpp | 13 ++++++++----- test/cf_functionalities_tests.cpp | 9 ++++----- test/diagnostic_protocol_tests.cpp | 10 ++++++---- 12 files changed, 65 insertions(+), 42 deletions(-) diff --git a/examples/diagnostic_protocol/main.cpp b/examples/diagnostic_protocol/main.cpp index 5d4592b1..770d72e3 100644 --- a/examples/diagnostic_protocol/main.cpp +++ b/examples/diagnostic_protocol/main.cpp @@ -78,8 +78,7 @@ int main() return -3; } - auto pgnRequestProtocol = std::make_shared(TestInternalECU); - isobus::DiagnosticProtocol diagnosticProtocol(TestInternalECU, pgnRequestProtocol); + isobus::DiagnosticProtocol diagnosticProtocol(TestInternalECU); diagnosticProtocol.initialize(); // Important: we need to update the diagnostic protocol using the hardware interface periodic update event, diff --git a/examples/pgn_requests/main.cpp b/examples/pgn_requests/main.cpp index 343e9b7e..8ce95e43 100644 --- a/examples/pgn_requests/main.cpp +++ b/examples/pgn_requests/main.cpp @@ -140,16 +140,14 @@ int main() return -3; } - isobus::ParameterGroupNumberRequestProtocol pgnRequestProtocol(TestInternalECU); - // Register a callback to handle PROPA PGN Requests - pgnRequestProtocol.register_pgn_request_callback(static_cast(isobus::CANLibParameterGroupNumber::ProprietaryA), example_proprietary_a_pgn_request_handler, nullptr); + TestInternalECU->get_pgn_request_protocol().lock()->register_pgn_request_callback(static_cast(isobus::CANLibParameterGroupNumber::ProprietaryA), example_proprietary_a_pgn_request_handler, nullptr); // Now, if you send a PGN request for EF00 to our internal control function, the stack will acknowledge it. Other requests will be NACK'ed (negative acknowledged) // NOTE the device you send from MUST have address claimed. // Now we'll set up a callback to handle requests for repetition rate for the PROPA PGN - pgnRequestProtocol.register_request_for_repetition_rate_callback(static_cast(isobus::CANLibParameterGroupNumber::ProprietaryA), example_proprietary_a_request_for_repetition_rate_handler, nullptr); + TestInternalECU->get_pgn_request_protocol().lock()->register_request_for_repetition_rate_callback(static_cast(isobus::CANLibParameterGroupNumber::ProprietaryA), example_proprietary_a_request_for_repetition_rate_handler, nullptr); // Now we'll get a callback when someone requests a repetition rate for PROPA. // The application (not the stack) must handle these requests, as the CAN stack does not know what data to send when responding. diff --git a/isobus/include/isobus/isobus/can_internal_control_function.hpp b/isobus/include/isobus/isobus/can_internal_control_function.hpp index 6ce7adde..b7ca8954 100644 --- a/isobus/include/isobus/isobus/can_internal_control_function.hpp +++ b/isobus/include/isobus/isobus/can_internal_control_function.hpp @@ -21,6 +21,7 @@ namespace isobus { class CANNetworkManager; + class ParameterGroupNumberRequestProtocol; //================================================================================================ /// @class InternalControlFunction @@ -39,6 +40,11 @@ namespace isobus /// @param[in] CANPort The CAN channel index for this control function to use static std::shared_ptr create(NAME desiredName, std::uint8_t preferredAddress, std::uint8_t CANPort); + /// @brief Destroys this control function, by removing it from the network manager + /// @param[in] expectedRefCount The expected number of shared pointers to this control function after removal + /// @returns true if the control function was successfully removed from everywhere in the stack, otherwise false + bool destroy(std::uint32_t expectedRefCount = 1) override; + /// @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); @@ -47,6 +53,10 @@ namespace isobus /// @returns Wether the control function has changed address by the end of the update bool update_address_claiming(CANLibBadge); + /// @brief Gets the PGN request protocol for this ICF + /// @returns The PGN request protocol for this ICF + std::weak_ptr get_pgn_request_protocol() const; + protected: /// @brief The protected constructor for the internal control function, which is called by the (inherited) factory function /// @param[in] desiredName The NAME for this control function to claim as @@ -56,6 +66,7 @@ namespace isobus private: AddressClaimStateMachine stateMachine; ///< The address claimer for this ICF + std::shared_ptr pgnRequestProtocol; ///< The PGN request protocol for this ICF }; } // namespace isobus diff --git a/isobus/include/isobus/isobus/can_parameter_group_number_request_protocol.hpp b/isobus/include/isobus/isobus/can_parameter_group_number_request_protocol.hpp index 47d9ea89..89f7c68c 100644 --- a/isobus/include/isobus/isobus/can_parameter_group_number_request_protocol.hpp +++ b/isobus/include/isobus/isobus/can_parameter_group_number_request_protocol.hpp @@ -14,7 +14,6 @@ #include "isobus/isobus/can_badge.hpp" #include "isobus/isobus/can_control_function.hpp" #include "isobus/isobus/can_network_manager.hpp" -#include "isobus/isobus/can_protocol.hpp" #include @@ -33,11 +32,17 @@ namespace isobus public: /// @brief The constructor for this protocol /// @param[in] internalControlFunction The internal control function that owns this protocol and will be used to send messages - explicit ParameterGroupNumberRequestProtocol(std::shared_ptr internalControlFunction); + ParameterGroupNumberRequestProtocol(std::shared_ptr internalControlFunction, CANLibBadge); /// @brief The destructor for this protocol ~ParameterGroupNumberRequestProtocol(); + /// @brief Remove the copy constructor + ParameterGroupNumberRequestProtocol(const ParameterGroupNumberRequestProtocol &) = delete; + + /// @brief Remove the copy assignment operator + ParameterGroupNumberRequestProtocol &operator=(const ParameterGroupNumberRequestProtocol &) = delete; + /// @brief Sends a PGN request to the specified control function /// @param[in] pgn The PGN to request /// @param[in] source The internal control function to send from @@ -149,7 +154,7 @@ namespace isobus /// @param[in] parameterGroupNumber The PGN to acknowledge /// @param[in] destination The destination control function to send the acknowledgement to /// @returns true if the message was sent, false otherwise - bool send_acknowledgement(AcknowledgementType type, std::uint32_t parameterGroupNumber, std::shared_ptr destination); + bool send_acknowledgement(AcknowledgementType type, std::uint32_t parameterGroupNumber, std::shared_ptr destination) const; std::shared_ptr myControlFunction; ///< The internal control function that this protocol will send from std::vector pgnRequestCallbacks; ///< A list of all registered PGN callbacks and the PGN associated with each callback diff --git a/isobus/include/isobus/isobus/isobus_diagnostic_protocol.hpp b/isobus/include/isobus/isobus/isobus_diagnostic_protocol.hpp index b553e33e..097a8be8 100644 --- a/isobus/include/isobus/isobus/isobus_diagnostic_protocol.hpp +++ b/isobus/include/isobus/isobus/isobus_diagnostic_protocol.hpp @@ -213,10 +213,8 @@ namespace isobus /// @brief The constructor for this protocol /// @param[in] internalControlFunction The internal control function that owns this protocol and will be used to send messages - /// @param[in] pgnRequestProtocol The PGN request protocol that will be used to respond to PGN requests /// @param[in] networkType The type of diagnostic network that this protocol will reflect DiagnosticProtocol(std::shared_ptr internalControlFunction, - std::shared_ptr pgnRequestProtocol, NetworkType networkType = NetworkType::ProprietaryNetwork1); /// @brief The destructor for this protocol @@ -490,7 +488,6 @@ 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::weak_ptr pgnRequestProtocol; ///< The PGN request protocol that this protocol will use 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 diff --git a/isobus/include/isobus/isobus/isobus_functionalities.hpp b/isobus/include/isobus/isobus/isobus_functionalities.hpp index fc19427e..899500d7 100644 --- a/isobus/include/isobus/isobus/isobus_functionalities.hpp +++ b/isobus/include/isobus/isobus/isobus_functionalities.hpp @@ -161,8 +161,7 @@ namespace isobus /// @brief Constructor for a ControlFunctionFunctionalities object /// @param[in] sourceControlFunction The control function to use when sending messages - /// @param[in] pgnRequestProtocol The pgn request protocol for receiving control functionality requests - ControlFunctionFunctionalities(std::shared_ptr sourceControlFunction, std::shared_ptr pgnRequestProtocol); + explicit ControlFunctionFunctionalities(std::shared_ptr sourceControlFunction); /// @brief Destructor for a ControlFunctionFunctionalities object ~ControlFunctionFunctionalities(); @@ -468,7 +467,6 @@ namespace isobus static constexpr std::uint8_t NUMBER_TIM_AUX_VALVES = 32; ///< The max number of TIM aux valves std::shared_ptr myControlFunction; ///< The control function to send messages as - std::weak_ptr pgnRequestProtocol; ///< The PGN request protocol to handle PGN requests with std::list supportedFunctionalities; ///< A list of all configured functionalities and their data std::mutex functionalitiesMutex; ///< Since messages come in on a different thread than the main app (probably), this mutex protects the functionality data ProcessingFlags txFlags; ///< Handles retries for sending the CF functionalities message diff --git a/isobus/src/can_internal_control_function.cpp b/isobus/src/can_internal_control_function.cpp index 08ad0f76..fac84a1e 100644 --- a/isobus/src/can_internal_control_function.cpp +++ b/isobus/src/can_internal_control_function.cpp @@ -12,6 +12,7 @@ #include "isobus/isobus/can_constants.hpp" #include "isobus/isobus/can_network_manager.hpp" +#include "isobus/isobus/can_parameter_group_number_request_protocol.hpp" #include @@ -27,10 +28,20 @@ namespace isobus { // Unfortunately, we can't use `std::make_shared` here because the constructor is private auto controlFunction = std::shared_ptr(new InternalControlFunction(desiredName, preferredAddress, CANPort)); + CANLibBadge badge; // This badge is used to allow creation of the PGN request protocol only from within this class + controlFunction->pgnRequestProtocol = std::make_unique(controlFunction, badge); CANNetworkManager::CANNetwork.on_control_function_created(controlFunction, {}); return controlFunction; } + bool InternalControlFunction::destroy(std::uint32_t expectedRefCount) + { + // We need to destroy the PGN request protocol before we destroy the control function + pgnRequestProtocol.reset(); + + return ControlFunction::destroy(expectedRefCount); + } + void InternalControlFunction::process_commanded_address(std::uint8_t commandedAddress, CANLibBadge) { stateMachine.process_commanded_address(commandedAddress); @@ -45,4 +56,9 @@ namespace isobus return previousAddress != address; } + std::weak_ptr InternalControlFunction::get_pgn_request_protocol() const + { + return pgnRequestProtocol; + } + } // namespace isobus diff --git a/isobus/src/can_parameter_group_number_request_protocol.cpp b/isobus/src/can_parameter_group_number_request_protocol.cpp index 29c0609f..3cef5a85 100644 --- a/isobus/src/can_parameter_group_number_request_protocol.cpp +++ b/isobus/src/can_parameter_group_number_request_protocol.cpp @@ -19,7 +19,7 @@ namespace isobus { - ParameterGroupNumberRequestProtocol::ParameterGroupNumberRequestProtocol(std::shared_ptr internalControlFunction) : + ParameterGroupNumberRequestProtocol::ParameterGroupNumberRequestProtocol(std::shared_ptr internalControlFunction, CANLibBadge) : myControlFunction(internalControlFunction) { assert(nullptr != myControlFunction && "ParameterGroupNumberRequestProtocol::ParameterGroupNumberRequestProtocol() called with nullptr internalControlFunction"); @@ -27,7 +27,7 @@ namespace isobus CANNetworkManager::CANNetwork.add_protocol_parameter_group_number_callback(static_cast(CANLibParameterGroupNumber::RequestForRepetitionRate), process_message, this); } - ParameterGroupNumberRequestProtocol ::~ParameterGroupNumberRequestProtocol() + ParameterGroupNumberRequestProtocol::~ParameterGroupNumberRequestProtocol() { CANNetworkManager::CANNetwork.remove_protocol_parameter_group_number_callback(static_cast(CANLibParameterGroupNumber::ParameterGroupNumberRequest), process_message, this); CANNetworkManager::CANNetwork.remove_protocol_parameter_group_number_callback(static_cast(CANLibParameterGroupNumber::RequestForRepetitionRate), process_message, this); @@ -265,7 +265,7 @@ namespace isobus } } - bool ParameterGroupNumberRequestProtocol::send_acknowledgement(AcknowledgementType type, std::uint32_t parameterGroupNumber, std::shared_ptr destination) + bool ParameterGroupNumberRequestProtocol::send_acknowledgement(AcknowledgementType type, std::uint32_t parameterGroupNumber, std::shared_ptr destination) const { bool retVal = false; diff --git a/isobus/src/isobus_diagnostic_protocol.cpp b/isobus/src/isobus_diagnostic_protocol.cpp index 5a126081..f05fefeb 100644 --- a/isobus/src/isobus_diagnostic_protocol.cpp +++ b/isobus/src/isobus_diagnostic_protocol.cpp @@ -74,10 +74,9 @@ namespace isobus return failureModeIdentifier; } - DiagnosticProtocol::DiagnosticProtocol(std::shared_ptr internalControlFunction, std::shared_ptr pgnRequestProtocol, NetworkType networkType) : - ControlFunctionFunctionalitiesMessageInterface(internalControlFunction, pgnRequestProtocol), + DiagnosticProtocol::DiagnosticProtocol(std::shared_ptr internalControlFunction, NetworkType networkType) : + ControlFunctionFunctionalitiesMessageInterface(internalControlFunction), myControlFunction(internalControlFunction), - pgnRequestProtocol(pgnRequestProtocol), networkType(networkType), txFlags(static_cast(TransmitFlags::NumberOfFlags), process_flags, this) { @@ -104,7 +103,7 @@ namespace isobus 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); - if (auto requestProtocol = pgnRequestProtocol.lock()) + if (auto requestProtocol = myControlFunction->get_pgn_request_protocol().lock()) { requestProtocol->register_pgn_request_callback(static_cast(CANLibParameterGroupNumber::DiagnosticMessage1), process_parameter_group_number_request, this); requestProtocol->register_pgn_request_callback(static_cast(CANLibParameterGroupNumber::DiagnosticMessage2), process_parameter_group_number_request, this); @@ -114,12 +113,8 @@ namespace isobus requestProtocol->register_pgn_request_callback(static_cast(CANLibParameterGroupNumber::DiagnosticProtocolIdentification), process_parameter_group_number_request, this); requestProtocol->register_pgn_request_callback(static_cast(CANLibParameterGroupNumber::SoftwareIdentification), process_parameter_group_number_request, this); requestProtocol->register_pgn_request_callback(static_cast(CANLibParameterGroupNumber::ECUIdentificationInformation), process_parameter_group_number_request, this); - retVal = true; - } - else - { - CANStackLogger::error("[DP] ParameterGroupNumberRequestProtocol expired in DiagnosticProtocol::initialize()"); } + retVal = true; } else { @@ -139,7 +134,7 @@ namespace isobus { initialized = false; - if (auto requestProtocol = pgnRequestProtocol.lock()) + if (auto requestProtocol = myControlFunction->get_pgn_request_protocol().lock()) { requestProtocol->remove_pgn_request_callback(static_cast(CANLibParameterGroupNumber::DiagnosticMessage1), process_parameter_group_number_request, this); requestProtocol->remove_pgn_request_callback(static_cast(CANLibParameterGroupNumber::DiagnosticMessage2), process_parameter_group_number_request, this); diff --git a/isobus/src/isobus_functionalities.cpp b/isobus/src/isobus_functionalities.cpp index f49379ba..57b40638 100644 --- a/isobus/src/isobus_functionalities.cpp +++ b/isobus/src/isobus_functionalities.cpp @@ -16,24 +16,27 @@ namespace isobus { - ControlFunctionFunctionalities::ControlFunctionFunctionalities(std::shared_ptr sourceControlFunction, std::shared_ptr pgnRequestProtocol) : + ControlFunctionFunctionalities::ControlFunctionFunctionalities(std::shared_ptr sourceControlFunction) : myControlFunction(sourceControlFunction), - pgnRequestProtocol(pgnRequestProtocol), txFlags(static_cast(TransmitFlags::NumberOfFlags), process_flags, this) { set_functionality_is_supported(Functionalities::MinimumControlFunction, 1, true); // Support the absolute minimum by default - if (nullptr != pgnRequestProtocol) + if (auto pgnRequestProtocol = sourceControlFunction->get_pgn_request_protocol().lock()) { pgnRequestProtocol->register_pgn_request_callback(static_cast(CANLibParameterGroupNumber::ControlFunctionFunctionalities), pgn_request_handler, this); } + else + { + CANStackLogger::error("[DP]: Failed to register PGN request callback for ControlFunctionFunctionalities due to the protocol being expired"); + } } ControlFunctionFunctionalities::~ControlFunctionFunctionalities() { - if (auto protocol = pgnRequestProtocol.lock()) + if (auto pgnRequestProtocol = myControlFunction->get_pgn_request_protocol().lock()) { - protocol->remove_pgn_request_callback(static_cast(CANLibParameterGroupNumber::ControlFunctionFunctionalities), pgn_request_handler, this); + pgnRequestProtocol->remove_pgn_request_callback(static_cast(CANLibParameterGroupNumber::ControlFunctionFunctionalities), pgn_request_handler, this); } } diff --git a/test/cf_functionalities_tests.cpp b/test/cf_functionalities_tests.cpp index d4e89da2..2db9f414 100644 --- a/test/cf_functionalities_tests.cpp +++ b/test/cf_functionalities_tests.cpp @@ -11,8 +11,8 @@ using namespace isobus; class TestControlFunctionFunctionalities : public ControlFunctionFunctionalities { public: - explicit TestControlFunctionFunctionalities(std::shared_ptr sourceControlFunction, std::shared_ptr pgnRequestProtocol) : - ControlFunctionFunctionalities(sourceControlFunction, pgnRequestProtocol) + explicit TestControlFunctionFunctionalities(std::shared_ptr sourceControlFunction) : + ControlFunctionFunctionalities(sourceControlFunction) { } @@ -49,8 +49,7 @@ TEST(CONTROL_FUNCTION_FUNCTIONALITIES_TESTS, CFFunctionalitiesTest) ASSERT_TRUE(internalECU->get_address_valid()); - auto pgnRequestProtocol = std::make_shared(internalECU); - TestControlFunctionFunctionalities cfFunctionalitiesUnderTest(internalECU, pgnRequestProtocol); + TestControlFunctionFunctionalities cfFunctionalitiesUnderTest(internalECU); std::this_thread::sleep_for(std::chrono::milliseconds(50)); @@ -615,5 +614,5 @@ TEST(CONTROL_FUNCTION_FUNCTIONALITIES_TESTS, CFFunctionalitiesTest) EXPECT_EQ(255, testMessageData.at(19)); // 255 Sections //! @todo try to reduce the reference count, such that that we don't use destroyed control functions later on - ASSERT_TRUE(internalECU->destroy(3)); + ASSERT_TRUE(internalECU->destroy(2)); } diff --git a/test/diagnostic_protocol_tests.cpp b/test/diagnostic_protocol_tests.cpp index ce392563..0ebfb2c8 100644 --- a/test/diagnostic_protocol_tests.cpp +++ b/test/diagnostic_protocol_tests.cpp @@ -12,16 +12,19 @@ TEST(DIAGNOSTIC_PROTOCOL_TESTS, CreateAndDestroyProtocolObjects) NAME TestDeviceNAME(0); auto TestInternalECU = InternalControlFunction::create(TestDeviceNAME, 0x1C, 0); - auto pgnRequestProtocol = std::make_shared(TestInternalECU); - auto diagnosticProtocol = std::make_unique(TestInternalECU, pgnRequestProtocol); + auto diagnosticProtocol = std::make_unique(TestInternalECU); EXPECT_TRUE(diagnosticProtocol->initialize()); EXPECT_FALSE(diagnosticProtocol->initialize()); // Should not be able to initialize twice + auto pgnRequestProtocol = TestInternalECU->get_pgn_request_protocol().lock(); + ASSERT_TRUE(pgnRequestProtocol); + EXPECT_NO_THROW(diagnosticProtocol->terminate()); diagnosticProtocol.reset(); EXPECT_EQ(pgnRequestProtocol->get_number_registered_pgn_request_callbacks(), 0); EXPECT_EQ(pgnRequestProtocol->get_number_registered_request_for_repetition_rate_callbacks(), 0); + pgnRequestProtocol.reset(); ASSERT_TRUE(TestInternalECU->destroy()); @@ -43,8 +46,7 @@ TEST(DIAGNOSTIC_PROTOCOL_TESTS, MessageEncoding) auto TestInternalECU = InternalControlFunction::create(TestDeviceNAME, 0xAA, 0); - auto pgnRequestProtocol = std::make_shared(TestInternalECU); - DiagnosticProtocol protocolUnderTest(TestInternalECU, pgnRequestProtocol, DiagnosticProtocol::NetworkType::SAEJ1939Network1PrimaryVehicleNetwork); + DiagnosticProtocol protocolUnderTest(TestInternalECU, DiagnosticProtocol::NetworkType::SAEJ1939Network1PrimaryVehicleNetwork); EXPECT_FALSE(protocolUnderTest.get_initialized()); protocolUnderTest.initialize();