[RP]: Make PGN request protocol member of ICF

examples/diagnostic_protocol/main.cpp

@@ -78,8 +78,7 @@ int main()
 		return -3;
 	}
 
-	auto pgnRequestProtocol = std::make_shared<isobus::ParameterGroupNumberRequestProtocol>(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,

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<std::uint32_t>(isobus::CANLibParameterGroupNumber::ProprietaryA), example_proprietary_a_pgn_request_handler, nullptr);
+	TestInternalECU->get_pgn_request_protocol().lock()->register_pgn_request_callback(static_cast<std::uint32_t>(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<std::uint32_t>(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<std::uint32_t>(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.

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<InternalControlFunction> 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<CANNetworkManager>);
@@ -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<CANNetworkManager>);
 
+		/// @brief Gets the PGN request protocol for this ICF
+		/// @returns The PGN request protocol for this ICF
+		std::weak_ptr<ParameterGroupNumberRequestProtocol> 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<ParameterGroupNumberRequestProtocol> pgnRequestProtocol; ///< The PGN request protocol for this ICF
 	};
 
 } // namespace isobus

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 <memory>
 
@@ -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> internalControlFunction);
+		ParameterGroupNumberRequestProtocol(std::shared_ptr<InternalControlFunction> internalControlFunction, CANLibBadge<InternalControlFunction>);
 
 		/// @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<ControlFunction> destination);
+		bool send_acknowledgement(AcknowledgementType type, std::uint32_t parameterGroupNumber, std::shared_ptr<ControlFunction> destination) const;
 
 		std::shared_ptr<InternalControlFunction> myControlFunction; ///< The internal control function that this protocol will send from
 		std::vector<PGNRequestCallbackInfo> pgnRequestCallbacks; ///< A list of all registered PGN callbacks and the PGN associated with each callback

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> internalControlFunction,
-		                   std::shared_ptr<ParameterGroupNumberRequestProtocol> 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<InternalControlFunction> myControlFunction; ///< The internal control function that this protocol will send from
-		std::weak_ptr<ParameterGroupNumberRequestProtocol> pgnRequestProtocol; ///< The PGN request protocol that this protocol will use
 		NetworkType networkType; ///< The diagnostic network type that this protocol will use
 		std::vector<DiagnosticTroubleCode> activeDTCList; ///< Keeps track of all the active DTCs
 		std::vector<DiagnosticTroubleCode> inactiveDTCList; ///< Keeps track of all the previously active DTCs

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<InternalControlFunction> sourceControlFunction, std::shared_ptr<ParameterGroupNumberRequestProtocol> pgnRequestProtocol);
+		explicit ControlFunctionFunctionalities(std::shared_ptr<InternalControlFunction> 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<InternalControlFunction> myControlFunction; ///< The control function to send messages as
-		std::weak_ptr<ParameterGroupNumberRequestProtocol> pgnRequestProtocol; ///< The PGN request protocol to handle PGN requests with
 		std::list<FunctionalityData> 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

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 <algorithm>
 
@@ -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<InternalControlFunction>(new InternalControlFunction(desiredName, preferredAddress, CANPort));
+		CANLibBadge<InternalControlFunction> badge; // This badge is used to allow creation of the PGN request protocol only from within this class
+		controlFunction->pgnRequestProtocol = std::make_unique<ParameterGroupNumberRequestProtocol>(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<CANNetworkManager>)
 	{
 		stateMachine.process_commanded_address(commandedAddress);
@@ -45,4 +56,9 @@ namespace isobus
 		return previousAddress != address;
 	}
 
+	std::weak_ptr<ParameterGroupNumberRequestProtocol> InternalControlFunction::get_pgn_request_protocol() const
+	{
+		return pgnRequestProtocol;
+	}
+
 } // namespace isobus

isobus/src/can_parameter_group_number_request_protocol.cpp

@@ -19,15 +19,15 @@
 namespace isobus
 {
 
-	ParameterGroupNumberRequestProtocol::ParameterGroupNumberRequestProtocol(std::shared_ptr<InternalControlFunction> internalControlFunction) :
+	ParameterGroupNumberRequestProtocol::ParameterGroupNumberRequestProtocol(std::shared_ptr<InternalControlFunction> internalControlFunction, CANLibBadge<InternalControlFunction>) :
 	  myControlFunction(internalControlFunction)
 	{
 		assert(nullptr != myControlFunction && "ParameterGroupNumberRequestProtocol::ParameterGroupNumberRequestProtocol() called with nullptr internalControlFunction");
 		CANNetworkManager::CANNetwork.add_protocol_parameter_group_number_callback(static_cast<std::uint32_t>(CANLibParameterGroupNumber::ParameterGroupNumberRequest), process_message, this);
 		CANNetworkManager::CANNetwork.add_protocol_parameter_group_number_callback(static_cast<std::uint32_t>(CANLibParameterGroupNumber::RequestForRepetitionRate), process_message, this);
 	}
 
-	ParameterGroupNumberRequestProtocol ::~ParameterGroupNumberRequestProtocol()
+	ParameterGroupNumberRequestProtocol::~ParameterGroupNumberRequestProtocol()
 	{
 		CANNetworkManager::CANNetwork.remove_protocol_parameter_group_number_callback(static_cast<std::uint32_t>(CANLibParameterGroupNumber::ParameterGroupNumberRequest), process_message, this);
 		CANNetworkManager::CANNetwork.remove_protocol_parameter_group_number_callback(static_cast<std::uint32_t>(CANLibParameterGroupNumber::RequestForRepetitionRate), process_message, this);
@@ -265,7 +265,7 @@ namespace isobus
 		}
 	}
 
-	bool ParameterGroupNumberRequestProtocol::send_acknowledgement(AcknowledgementType type, std::uint32_t parameterGroupNumber, std::shared_ptr<ControlFunction> destination)
+	bool ParameterGroupNumberRequestProtocol::send_acknowledgement(AcknowledgementType type, std::uint32_t parameterGroupNumber, std::shared_ptr<ControlFunction> destination) const
 	{
 		bool retVal = false;
 

isobus/src/isobus_diagnostic_protocol.cpp

@@ -74,10 +74,9 @@ namespace isobus
 		return failureModeIdentifier;
 	}
 
-	DiagnosticProtocol::DiagnosticProtocol(std::shared_ptr<InternalControlFunction> internalControlFunction, std::shared_ptr<ParameterGroupNumberRequestProtocol> pgnRequestProtocol, NetworkType networkType) :
-	  ControlFunctionFunctionalitiesMessageInterface(internalControlFunction, pgnRequestProtocol),
+	DiagnosticProtocol::DiagnosticProtocol(std::shared_ptr<InternalControlFunction> internalControlFunction, NetworkType networkType) :
+	  ControlFunctionFunctionalitiesMessageInterface(internalControlFunction),
 	  myControlFunction(internalControlFunction),
-	  pgnRequestProtocol(pgnRequestProtocol),
 	  networkType(networkType),
 	  txFlags(static_cast<std::uint32_t>(TransmitFlags::NumberOfFlags), process_flags, this)
 	{
@@ -104,7 +103,7 @@ namespace isobus
 			CANNetworkManager::CANNetwork.add_protocol_parameter_group_number_callback(static_cast<std::uint32_t>(CANLibParameterGroupNumber::DiagnosticMessage13), process_message, this);
 			CANNetworkManager::CANNetwork.add_global_parameter_group_number_callback(static_cast<std::uint32_t>(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<std::uint32_t>(CANLibParameterGroupNumber::DiagnosticMessage1), process_parameter_group_number_request, this);
 				requestProtocol->register_pgn_request_callback(static_cast<std::uint32_t>(CANLibParameterGroupNumber::DiagnosticMessage2), process_parameter_group_number_request, this);
@@ -114,12 +113,8 @@ namespace isobus
 				requestProtocol->register_pgn_request_callback(static_cast<std::uint32_t>(CANLibParameterGroupNumber::DiagnosticProtocolIdentification), process_parameter_group_number_request, this);
 				requestProtocol->register_pgn_request_callback(static_cast<std::uint32_t>(CANLibParameterGroupNumber::SoftwareIdentification), process_parameter_group_number_request, this);
 				requestProtocol->register_pgn_request_callback(static_cast<std::uint32_t>(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<std::uint32_t>(CANLibParameterGroupNumber::DiagnosticMessage1), process_parameter_group_number_request, this);
 				requestProtocol->remove_pgn_request_callback(static_cast<std::uint32_t>(CANLibParameterGroupNumber::DiagnosticMessage2), process_parameter_group_number_request, this);

isobus/src/isobus_functionalities.cpp

@@ -16,24 +16,27 @@
 
 namespace isobus
 {
-	ControlFunctionFunctionalities::ControlFunctionFunctionalities(std::shared_ptr<InternalControlFunction> sourceControlFunction, std::shared_ptr<ParameterGroupNumberRequestProtocol> pgnRequestProtocol) :
+	ControlFunctionFunctionalities::ControlFunctionFunctionalities(std::shared_ptr<InternalControlFunction> sourceControlFunction) :
 	  myControlFunction(sourceControlFunction),
-	  pgnRequestProtocol(pgnRequestProtocol),
 	  txFlags(static_cast<std::uint32_t>(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<std::uint32_t>(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<std::uint32_t>(CANLibParameterGroupNumber::ControlFunctionFunctionalities), pgn_request_handler, this);
+			pgnRequestProtocol->remove_pgn_request_callback(static_cast<std::uint32_t>(CANLibParameterGroupNumber::ControlFunctionFunctionalities), pgn_request_handler, this);
 		}
 	}
 

test/cf_functionalities_tests.cpp

@@ -11,8 +11,8 @@ using namespace isobus;
 class TestControlFunctionFunctionalities : public ControlFunctionFunctionalities
 {
 public:
-	explicit TestControlFunctionFunctionalities(std::shared_ptr<InternalControlFunction> sourceControlFunction, std::shared_ptr<ParameterGroupNumberRequestProtocol> pgnRequestProtocol) :
-	  ControlFunctionFunctionalities(sourceControlFunction, pgnRequestProtocol)
+	explicit TestControlFunctionFunctionalities(std::shared_ptr<InternalControlFunction> sourceControlFunction) :
+	  ControlFunctionFunctionalities(sourceControlFunction)
 	{
 	}
 
@@ -49,8 +49,7 @@ TEST(CONTROL_FUNCTION_FUNCTIONALITIES_TESTS, CFFunctionalitiesTest)
 
 	ASSERT_TRUE(internalECU->get_address_valid());
 
-	auto pgnRequestProtocol = std::make_shared<ParameterGroupNumberRequestProtocol>(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));
 }

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<ParameterGroupNumberRequestProtocol>(TestInternalECU);
-	auto diagnosticProtocol = std::make_unique<DiagnosticProtocol>(TestInternalECU, pgnRequestProtocol);
+	auto diagnosticProtocol = std::make_unique<DiagnosticProtocol>(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<ParameterGroupNumberRequestProtocol>(TestInternalECU);
-	DiagnosticProtocol protocolUnderTest(TestInternalECU, pgnRequestProtocol, DiagnosticProtocol::NetworkType::SAEJ1939Network1PrimaryVehicleNetwork);
+	DiagnosticProtocol protocolUnderTest(TestInternalECU, DiagnosticProtocol::NetworkType::SAEJ1939Network1PrimaryVehicleNetwork);
 
 	EXPECT_FALSE(protocolUnderTest.get_initialized());
 	protocolUnderTest.initialize();