core: fix callback data not loading buffer correctly

Also stopped transport protocols listening to messages not destined to them

examples/transport_layer/main.cpp

@@ -32,6 +32,11 @@ int main()
 {
 	std::signal(SIGINT, signal_handler);
 
+#ifndef ISOBUS_VIRTUALCAN_AVAILABLE
+	std::cout << "This example requires the VirtualCAN plugin to be available. If using CMake, set the `-DCAN_DRIVER=VirtualCAN`." << std::endl;
+	return -1;
+#endif
+
 	std::shared_ptr<CANHardwarePlugin> originatorDriver = std::make_shared<VirtualCANPlugin>("test-channel");
 	std::shared_ptr<CANHardwarePlugin> recipientDriver = std::make_shared<VirtualCANPlugin>("test-channel");
 
@@ -181,7 +186,7 @@ void check_can_message(const CANMessage &message, void *)
 	{
 		if (message.get_data()[i] != (i % 0xFF))
 		{
-			std::cout << std::endl // End the progress bar
+			std::cerr << std::endl // End the progress bar
 			          << "Received CAN with incorrect data!!!" << std::endl;
 			return;
 		}

isobus/include/isobus/isobus/can_extended_transport_protocol.hpp

@@ -18,14 +18,10 @@
 
 namespace isobus
 {
-	//================================================================================================
-	/// @class ExtendedTransportProtocolManager
-	///
 	/// @brief A class that handles the ISO11783 extended transport protocol.
 	/// @details This class handles transmission and reception of CAN messages more than 1785 bytes.
 	/// Simply call Simply call `CANNetworkManager::send_can_message()`
 	/// with an appropriate data length, and the protocol will be automatically selected to be used.
-	//================================================================================================
 	class ExtendedTransportProtocolManager
 	{
 	public:
@@ -64,11 +60,7 @@ namespace isobus
 			AnyOtherError = 250 ///< Any reason not defined in the standard
 		};
 
-		//================================================================================================
-		/// @class ExtendedTransportProtocolSession
-		///
 		/// @brief A storage object to keep track of session information internally
-		//================================================================================================
 		class ExtendedTransportProtocolSession : public TransportProtocolSessionBase
 		{
 		public:
@@ -144,18 +136,18 @@ namespace isobus
 		private:
 			StateMachineState state = StateMachineState::None; ///< The state machine state for this session
 
-			std::uint8_t lastSequenceNumber = 0; ///< The last processed sequence number for this set of packets
-			std::uint32_t sequenceNumberOffset = 0; ///< The offset of the sequence number relative to the packet number
 			std::uint32_t lastAcknowledgedPacketNumber = 0; ///< The last acknowledged packet number by the receiver
+			std::uint32_t sequenceNumberOffset = 0; ///< The offset of the sequence number relative to the packet number
+			std::uint8_t lastSequenceNumber = 0; ///< The last processed sequence number for this set of packets
 			std::uint8_t dataPacketOffsetPacketCount = 0; ///< The number of packets that will be sent with the current DPO
 			std::uint8_t clearToSendPacketCountLimit = 0xFF; ///< The max packets that can be sent per DPO as indicated by the CTS message
 		};
 
-		static constexpr std::uint32_t REQUEST_TO_SEND_MULTIPLEXOR = 20; ///< ETP.CM_RTS Multiplexor
-		static constexpr std::uint32_t CLEAR_TO_SEND_MULTIPLEXOR = 21; ///< ETP.CM_CTS Multiplexor
-		static constexpr std::uint32_t DATA_PACKET_OFFSET_MULTIPLXOR = 22; ///< ETP.CM_DPO Multiplexor
-		static constexpr std::uint32_t END_OF_MESSAGE_ACKNOWLEDGE_MULTIPLEXOR = 23; ///< TP.CM_EOMA Multiplexor
-		static constexpr std::uint32_t CONNECTION_ABORT_MULTIPLEXOR = 255; ///< Abort multiplexor
+		static constexpr std::uint32_t REQUEST_TO_SEND_MULTIPLEXOR = 0x14; ///< (20) ETP.CM_RTS Multiplexor
+		static constexpr std::uint32_t CLEAR_TO_SEND_MULTIPLEXOR = 0x15; ///< (21) ETP.CM_CTS Multiplexor
+		static constexpr std::uint32_t DATA_PACKET_OFFSET_MULTIPLXOR = 0x16; ///< (22) ETP.CM_DPO Multiplexor
+		static constexpr std::uint32_t END_OF_MESSAGE_ACKNOWLEDGE_MULTIPLEXOR = 0x17; ///< (23) TP.CM_EOMA Multiplexor
+		static constexpr std::uint32_t CONNECTION_ABORT_MULTIPLEXOR = 0xFF; ///< (255) Abort multiplexor
 		static constexpr std::uint32_t MAX_PROTOCOL_DATA_LENGTH = 117440505; ///< The max number of bytes that this protocol can transfer
 		static constexpr std::uint16_t T1_TIMEOUT_MS = 750; ///< The t1 timeout as defined by the standard
 		static constexpr std::uint16_t T2_T3_TIMEOUT_MS = 1250; ///< The t2/t3 timeouts as defined by the standard

isobus/include/isobus/isobus/can_message_data.hpp

@@ -21,11 +21,7 @@
 
 namespace isobus
 {
-	//================================================================================================
-	/// @class CANMessageData
-	///
 	/// @brief A interface class that represents data payload of a CAN message of arbitrary length.
-	//================================================================================================
 	class CANMessageData
 	{
 	public:
@@ -47,11 +43,7 @@ namespace isobus
 		virtual std::unique_ptr<CANMessageData> copy_if_not_owned(std::unique_ptr<CANMessageData> self) const = 0;
 	};
 
-	//================================================================================================
-	/// @class CANMessageDataVector
-	///
 	/// @brief A class that represents data of a CAN message by holding a vector of bytes.
-	//================================================================================================
 	class CANMessageDataVector : public CANMessageData
 	  , public std::vector<std::uint8_t>
 	{
@@ -93,12 +85,8 @@ namespace isobus
 		std::unique_ptr<CANMessageData> copy_if_not_owned(std::unique_ptr<CANMessageData> self) const override;
 	};
 
-	//================================================================================================
-	/// @class CANMessageDataView
-	///
 	/// @brief A class that represents data of a CAN message by holding a view of an array of bytes.
 	/// The view is not owned by this class, it is simply holding a pointer to the array of bytes.
-	//================================================================================================
 	class CANMessageDataView : public CANMessageData
 	  , public CANDataSpan
 	{
@@ -127,11 +115,7 @@ namespace isobus
 		std::unique_ptr<CANMessageData> copy_if_not_owned(std::unique_ptr<CANMessageData> self) const override;
 	};
 
-	//================================================================================================
-	/// @class CANMessageDataCallback
-	///
 	/// @brief A class that represents data of a CAN message by using a callback function.
-	//================================================================================================
 	class CANMessageDataCallback : public CANMessageData
 	{
 	public:
@@ -166,6 +150,7 @@ namespace isobus
 		std::vector<std::uint8_t> buffer; ///< The buffer to store the data chunks.
 		std::size_t bufferSize; ///< The size of the buffer.
 		std::size_t dataOffset = 0; ///< The offset of the data in the buffer.
+		bool initialized = false; ///< Whether the buffer has been initialized.
 	};
 } // namespace isobus
 

isobus/include/isobus/isobus/can_network_configuration.hpp

@@ -37,7 +37,8 @@ namespace isobus
 		/// @returns The max number of concurrent TP sessions
 		std::uint32_t get_max_number_transport_protocol_sessions() const;
 
-		/// @brief Sets the minimum time to wait between sending BAM frames (default is 50 ms)
+		/// @brief Sets the minimum time to wait between sending BAM frames
+		/// (default is 50 ms for maximum J1939 compatibility)
 		/// @details The acceptable range as defined by ISO-11783 is 10 to 200 ms.
 		/// This is a minumum time, so if you set it to some value, like 10 ms, the
 		/// stack will attempt to transmit it as close to that time as it can, but it is
@@ -50,16 +51,16 @@ namespace isobus
 		std::uint32_t get_minimum_time_between_transport_protocol_bam_frames() const;
 
 		/// @brief Sets the max number of data frames the stack will use when
-		/// in an ETP session, between EDPO phases. The default is 255,
-		/// but decreasing it may reduce bus load at the expense of transfer time.
+		/// in an ETP session, between EDPO phases. The default is 16.
+		/// Note that the sending control function may choose to use a lower number of frames.
 		/// @param[in] numberFrames The max number of data frames to use
-		void set_max_number_of_etp_frames_per_edpo(std::uint8_t numberFrames);
+		void set_number_of_packets_per_dpo_message(std::uint8_t numberFrames);
 
 		/// @brief Returns the max number of data frames the stack will use when
-		/// in an ETP session, between EDPO phases. The default is 255,
-		/// but decreasing it may reduce bus load at the expense of transfer time.
+		/// in an ETP session, between EDPO phases. The default is 16.
+		/// Note that the sending control function may choose to use a lower number of frames.
 		/// @returns The number of data frames the stack will use when sending ETP messages between EDPOs
-		std::uint8_t get_max_number_of_etp_frames_per_edpo() const;
+		std::uint8_t get_number_of_packets_per_dpo_message() const;
 
 		/// @brief Sets the max number of data frames the stack will send from each
 		/// transport layer protocol, per update. The default is 255,
@@ -88,8 +89,8 @@ namespace isobus
 
 		std::uint32_t maxNumberTransportProtocolSessions = 4; ///< The max number of TP sessions allowed
 		std::uint32_t minimumTimeBetweenTransportProtocolBAMFrames = DEFAULT_BAM_PACKET_DELAY_TIME_MS; ///< The configurable time between BAM frames
-		std::uint8_t extendedTransportProtocolMaxNumberOfFramesPerEDPO = 0xFF; ///< Used to control throttling of ETP sessions.
 		std::uint8_t networkManagerMaxFramesToSendPerUpdate = 0xFF; ///< Used to control the max number of transport layer frames added to the driver queue per network manager update
+		std::uint8_t numberOfPacketsPerDPOMessage = 16; ///< The number of packets per DPO message for ETP sessions
 		std::uint8_t numberOfPacketsPerCTSMessage = 16; ///< The number of packets per CTS message for TP sessions
 	};
 } // namespace isobus

isobus/include/isobus/isobus/can_transport_protocol.hpp

@@ -18,19 +18,16 @@
 
 namespace isobus
 {
-	//================================================================================================
-	/// @class TransportProtocolManager
-	///
+
 	/// @brief A class that handles the ISO11783/J1939 transport protocol.
 	/// @details This class handles transmission and reception of CAN messages up to 1785 bytes.
 	/// Both broadcast and connection mode are supported. Simply call `CANNetworkManager::send_can_message()`
 	/// with an appropriate data length, and the protocol will be automatically selected to be used.
-	/// @note The use of broadcast messages is discouraged, as it has profound
+	/// @note The use of multi-frame broadcast messages (BAM) is discouraged, as it has profound
 	/// packet timing implications for your application, and is limited to only 1 active session at a time.
 	/// That session could be busy if you are using DM1 or any other BAM protocol, causing intermittent
 	/// transmit failures from this class. This is not a bug, rather a limitation of the protocol
 	/// definition.
-	//================================================================================================
 	class TransportProtocolManager
 	{
 	public:
@@ -63,11 +60,7 @@ namespace isobus
 			AnyOtherError = 250 ///< Any reason not defined in the standard
 		};
 
-		//================================================================================================
-		/// @class TransportProtocolSession
-		///
 		/// @brief A storage object to keep track of session information internally
-		//================================================================================================
 		class TransportProtocolSession : public TransportProtocolSessionBase
 		{
 		public:
@@ -164,11 +157,11 @@ namespace isobus
 			std::uint8_t clearToSendPacketCountMax = 0xFF; ///< The max packets that can be sent per CTS as indicated by the RTS message
 		};
 
-		static constexpr std::uint32_t REQUEST_TO_SEND_MULTIPLEXOR = 16; ///< TP.CM_RTS Multiplexor
-		static constexpr std::uint32_t CLEAR_TO_SEND_MULTIPLEXOR = 17; ///< TP.CM_CTS Multiplexor
-		static constexpr std::uint32_t END_OF_MESSAGE_ACKNOWLEDGE_MULTIPLEXOR = 19; ///< TP.CM_EOM_ACK Multiplexor
-		static constexpr std::uint32_t BROADCAST_ANNOUNCE_MESSAGE_MULTIPLEXOR = 32; ///< TP.BAM Multiplexor
-		static constexpr std::uint32_t CONNECTION_ABORT_MULTIPLEXOR = 255; ///< Abort multiplexor
+		static constexpr std::uint32_t REQUEST_TO_SEND_MULTIPLEXOR = 0x10; ///< (16) TP.CM_RTS Multiplexor
+		static constexpr std::uint32_t CLEAR_TO_SEND_MULTIPLEXOR = 0x11; ///< (17) TP.CM_CTS Multiplexor
+		static constexpr std::uint32_t END_OF_MESSAGE_ACKNOWLEDGE_MULTIPLEXOR = 0x13; ///< (19) TP.CM_EOM_ACK Multiplexor
+		static constexpr std::uint32_t BROADCAST_ANNOUNCE_MESSAGE_MULTIPLEXOR = 0x20; ///< (32) TP.BAM Multiplexor
+		static constexpr std::uint32_t CONNECTION_ABORT_MULTIPLEXOR = 0xFF; ///< (255) Abort multiplexor
 		static constexpr std::uint32_t MAX_PROTOCOL_DATA_LENGTH = 1785; ///< The max number of bytes that this protocol can transfer
 		static constexpr std::uint16_t T1_TIMEOUT_MS = 750; ///< The t1 timeout as defined by the standard
 		static constexpr std::uint16_t T2_T3_TIMEOUT_MS = 1250; ///< The t2/t3 timeouts as defined by the standard

isobus/include/isobus/isobus/can_transport_protocol_base.hpp

@@ -16,11 +16,7 @@
 
 namespace isobus
 {
-	//================================================================================================
-	/// @class TransportProtocolSessionBase
-	///
 	/// @brief An object to keep track of session information internally
-	//================================================================================================
 	class TransportProtocolSessionBase
 	{
 	public:

isobus/src/can_extended_transport_protocol.cpp

@@ -165,6 +165,9 @@ namespace isobus
 			                                                                     nullptr, // No callback
 			                                                                     nullptr);
 
+			// Request the maximum number of packets per DPO via the CTS message
+			newSession->set_cts_number_of_packet_limit(configuration->get_number_of_packets_per_dpo_message());
+
 			newSession->set_state(StateMachineState::SendClearToSend);
 			activeSessions.push_back(newSession);
 			CANStackLogger::debug("[ETP]: New rx session for 0x%05X. Source: %hu, destination: %hu", parameterGroupNumber, source->get_address(), destination->get_address());
@@ -483,7 +486,7 @@ namespace isobus
 	void ExtendedTransportProtocolManager::process_message(const CANMessage &message)
 	{
 		// TODO: Allow sniffing of messages to all addresses, not just the ones we normally listen to (#297)
-		if (message.has_valid_source_control_function() && message.has_valid_destination_control_function())
+		if (message.has_valid_source_control_function() && message.is_destination_our_device())
 		{
 			switch (message.get_identifier().get_parameter_group_number())
 			{
@@ -830,10 +833,10 @@ namespace isobus
 		{
 			packetsThisSegment = session->get_cts_number_of_packet_limit();
 		}
-		else if (packetsThisSegment > 16)
+		if (packetsThisSegment > configuration->get_number_of_packets_per_dpo_message())
 		{
-			//! @todo apply CTS number of packets recommendation of 16 via a configuration option
-			packetsThisSegment = 16;
+			CANStackLogger::debug("[TP]: Received Request To Send (RTS) with a CTS packet count of %hu, which is greater than the configured maximum of %hu, using the configured maximum instead.", packetsThisSegment, configuration->get_number_of_packets_per_dpo_message());
+			packetsThisSegment = configuration->get_number_of_packets_per_dpo_message();
 		}
 
 		const std::array<std::uint8_t, CAN_DATA_LENGTH> buffer{

isobus/src/can_message_data.cpp

@@ -99,9 +99,15 @@ namespace isobus
 
 	std::uint8_t CANMessageDataCallback::get_byte(std::size_t index)
 	{
-		if (index >= dataOffset + bufferSize)
+		if (index >= totalSize)
 		{
-			dataOffset += bufferSize;
+			return 0;
+		}
+
+		if ((index >= dataOffset + bufferSize) || (index < dataOffset) || (!initialized))
+		{
+			initialized = true;
+			dataOffset = index;
 			callback(0, dataOffset, std::min(totalSize - dataOffset, bufferSize), buffer.data(), parentPointer);
 		}
 		return buffer[index - dataOffset];

isobus/src/can_network_configuration.cpp

@@ -38,14 +38,14 @@ namespace isobus
 		return minimumTimeBetweenTransportProtocolBAMFrames;
 	}
 
-	void CANNetworkConfiguration::set_max_number_of_etp_frames_per_edpo(std::uint8_t numberFrames)
+	void CANNetworkConfiguration::set_number_of_packets_per_dpo_message(std::uint8_t numberFrames)
 	{
-		extendedTransportProtocolMaxNumberOfFramesPerEDPO = numberFrames;
+		numberOfPacketsPerDPOMessage = numberFrames;
 	}
 
-	std::uint8_t CANNetworkConfiguration::get_max_number_of_etp_frames_per_edpo() const
+	std::uint8_t CANNetworkConfiguration::get_number_of_packets_per_dpo_message() const
 	{
-		return extendedTransportProtocolMaxNumberOfFramesPerEDPO;
+		return numberOfPacketsPerDPOMessage;
 	}
 
 	void CANNetworkConfiguration::set_max_number_of_network_manager_protocol_frames_per_update(std::uint8_t numberFrames)

isobus/src/can_transport_protocol.cpp

@@ -539,7 +539,7 @@ namespace isobus
 	void TransportProtocolManager::process_message(const CANMessage &message)
 	{
 		// TODO: Allow sniffing of messages to all addresses, not just the ones we normally listen to (#297)
-		if (message.has_valid_source_control_function() && (message.has_valid_destination_control_function() || message.is_broadcast()))
+		if (message.has_valid_source_control_function() && (message.is_destination_our_device() || message.is_broadcast()))
 		{
 			switch (message.get_identifier().get_parameter_group_number())
 			{

test/helpers/control_function_helpers.cpp

@@ -134,4 +134,20 @@ namespace test_helpers
 		return std::make_shared<WrappedControlFunction>(NAME(0), address, 0);
 	}
 
+	class WrappedInternalControlFunction : public InternalControlFunction
+	{
+	public:
+		WrappedInternalControlFunction(NAME name, std::uint8_t address, std::uint8_t canPort) :
+		  InternalControlFunction(name, address, canPort, {})
+		{
+			// We need to set the address manually, since there won't be an address claim state machine running
+			ControlFunction::address = address;
+		}
+	};
+
+	std::shared_ptr<isobus::InternalControlFunction> create_mock_internal_control_function(std::uint8_t address)
+	{
+		return std::make_shared<WrappedInternalControlFunction>(NAME(0), address, 0);
+	}
+
 }; // namespace test_helpers

test/helpers/control_function_helpers.hpp

@@ -12,6 +12,8 @@ namespace test_helpers
 
 	std::shared_ptr<isobus::ControlFunction> create_mock_control_function(std::uint8_t address);
 
+	std::shared_ptr<isobus::InternalControlFunction> create_mock_internal_control_function(std::uint8_t address);
+
 }; // namespace test_helpers
 
 #endif // CONTROL_FUNCTION_HELPERS_HPP