Do not reparse the full (already parsed) IOP after new segments arrive

isobus/include/isobus/isobus/isobus_virtual_terminal_working_set_base.hpp

@@ -104,12 +104,24 @@
 		                                  std::uint8_t *&iopData,
 		                                  std::uint32_t &iopLength) const;
 
+		/// @brief A class to hold together IOP data and a processing state flag
+		class IopDataComponent
+		{
+		public:
+			///< Constructor where the IOP data can be passed
+			/// @param[in] d A pointer to the object pool data
+			IopDataComponent(const std::vector<std::uint8_t> &d) :
+			  data(d) {}
+			bool processed = false; ///< This variable is set to true after this IOP section is being parset
+			std::vector<std::uint8_t> data; ///< Stores the raw IOP data
+		};
+
 		std::mutex managedWorkingSetMutex; ///< A mutex to protect the interface of the managed working set
 		VTColourTable workingSetColourTable; ///< This working set's colour table
 		std::uint32_t iopSize = 0; ///< Total size of the IOP in bytes
 		std::uint32_t transferredIopSize = 0; ///< Total number of IOP bytes transferred
 		std::map<std::uint16_t, std::shared_ptr<VTObject>> vtObjectTree; ///< The C++ object representation (deserialized) of the object pool being managed
-		std::vector<std::vector<std::uint8_t>> iopFilesRawData; ///< Raw IOP File data from the client
+		std::vector<IopDataComponent> iopFilesRawData; ///< Raw IOP data from the client
 		std::uint16_t workingSetID = NULL_OBJECT_ID; ///< Stores the object ID of the working set object itself
 		std::uint16_t faultingObjectID = NULL_OBJECT_ID; ///< Stores the faulting object ID to send to a client when parsing the pool fails
 	};

isobus/src/isobus_virtual_terminal_server_managed_working_set.cpp

@@ -185,11 +185,20 @@ namespace isobus
 			         " IOP components.");
 			for (std::size_t i = 0; i < iopFilesRawData.size(); i++)
 			{
-				if (!parse_iop_into_objects(iopFilesRawData[i].data(), static_cast<std::uint32_t>(iopFilesRawData[i].size())))
+				if (iopFilesRawData[i].processed)
+				{
+					continue;
+				}
+
+				if (!parse_iop_into_objects(iopFilesRawData[i].data.data(), static_cast<std::uint32_t>(iopFilesRawData[i].data.size())))
 				{
 					lSuccess = false;
 					break;
 				}
+				else
+				{
+					iopFilesRawData[i].processed = true;
+				}
 			}
 
 			if (lSuccess)

isobus/src/isobus_virtual_terminal_working_set_base.cpp

@@ -84,7 +84,7 @@
 	void VirtualTerminalWorkingSetBase::add_iop_raw_data(const std::vector<std::uint8_t> &dataToAdd)
 	{
 		transferredIopSize += dataToAdd.size();
-		iopFilesRawData.push_back(dataToAdd);
+		iopFilesRawData.push_back(IopDataComponent(dataToAdd));
 	}
 
 	std::size_t VirtualTerminalWorkingSetBase::get_number_iop_files() const
@@ -94,7 +94,7 @@
 
 	std::vector<std::uint8_t> &VirtualTerminalWorkingSetBase::get_iop_raw_data(std::size_t index)
 	{
-		return iopFilesRawData.at(index);
+		return iopFilesRawData.at(index).data;
 	}
 
 	VTColourVector VirtualTerminalWorkingSetBase::get_colour(std::uint8_t colourIndex) const

test/CMakeLists.txt

@@ -62,6 +62,7 @@ set(TEST_SRC
     heartbeat_tests.cpp
     tc_server_tests.cpp
     helpers/test_time_source.cpp
+    vt_server_managed_working_set_tests.cpp
     helpers/control_function_helpers.cpp
     helpers/messaging_helpers.cpp)
 

test/vt_server_managed_working_set_tests.cpp

@@ -0,0 +1,72 @@
+#include <gtest/gtest.h>
+
+#include "isobus/isobus/isobus_virtual_terminal_server_managed_working_set.hpp"
+
+using namespace isobus;
+
+TEST(VirtualTerminalServerTest, AlreadyParsedIOPSegmentIsNotReparsed)
+{
+	VirtualTerminalServerManagedWorkingSet workingSet;
+
+	// Container 0x1234:
+	// ID          = 0x1234
+	// Type        = Container
+	// Width       = 100
+	// Height      = 100
+	// Hidden      = false
+	// Children    = 0
+	// Macros      = 0
+	std::vector<std::uint8_t> firstSegment = {
+		0x34, 0x12, static_cast<std::uint8_t>(VirtualTerminalObjectType::Container), 0x64, 0x00, 0x64, 0x00, 0x00, 0x00, 0x00
+	};
+
+	workingSet.add_iop_raw_data(firstSegment);
+
+	workingSet.start_parsing_thread();
+	workingSet.join_parsing_thread();
+
+	auto firstObject = workingSet.get_object_by_id(0x1234);
+	ASSERT_NE(nullptr, firstObject);
+	ASSERT_EQ(VirtualTerminalObjectType::Container, firstObject->get_object_type());
+
+	auto firstContainer = std::static_pointer_cast<Container>(firstObject);
+
+	EXPECT_FALSE(firstContainer->get_hidden());
+
+	// Simulate a Change Child Position / Hide-Show related runtime state
+	// change after the initial IOP has been parsed.
+	firstContainer->set_hidden(true);
+
+	EXPECT_TRUE(firstContainer->get_hidden());
+
+	// A second dynamically transferred IOP segment.
+	std::vector<std::uint8_t> secondSegment = {
+		0x78, 0x56, static_cast<std::uint8_t>(VirtualTerminalObjectType::Container), 0x32, 0x00, 0x32, 0x00, 0x00, 0x00, 0x00
+	};
+
+	workingSet.add_iop_raw_data(secondSegment);
+
+	workingSet.start_parsing_thread();
+	workingSet.join_parsing_thread();
+
+	// The new segment must have been parsed.
+	auto secondObject = workingSet.get_object_by_id(0x5678);
+	ASSERT_NE(nullptr, secondObject);
+	EXPECT_EQ(VirtualTerminalObjectType::Container,
+	          secondObject->get_object_type());
+
+	// Most important regression check:
+	// runtime state of an object from an already parsed segment must survive.
+	auto firstObjectAfterSecondParse =
+	  workingSet.get_object_by_id(0x1234);
+
+	ASSERT_NE(nullptr, firstObjectAfterSecondParse);
+
+	auto firstContainerAfterSecondParse =
+	  std::static_pointer_cast<Container>(firstObjectAfterSecondParse);
+
+	EXPECT_TRUE(firstContainerAfterSecondParse->get_hidden());
+
+	// Make sure that it was not reconstructed/replaced at all.
+	EXPECT_EQ(firstObject.get(), firstObjectAfterSecondParse.get());
+}