test TS writes (#3515)

* test TS writes

* s

* this must be a gcc extension

firmware/console/binary/tunerstudio.cpp

@@ -89,13 +89,6 @@
 #include "rusEfiFunctionalTest.h"
 #endif /* EFI_SIMULATOR */
 
-#if EFI_TUNER_STUDIO
-
-/* 1S */
-#define TS_COMMUNICATION_TIMEOUT	TIME_MS2I(1000)
-
-extern persistent_config_container_s persistentState;
-
 #if !defined(EFI_NO_CONFIG_WORKING_COPY)
 /**
  * this is a local copy of the configuration. Any changes to this copy
@@ -105,12 +98,6 @@ persistent_config_s configWorkingCopy;
 
 #endif /* EFI_NO_CONFIG_WORKING_COPY */
 
-static efitimems_t previousWriteReportMs = 0;
-
-static void resetTs(void) {
-	memset(&tsState, 0, sizeof(tsState));
-}
-
 static void printErrorCounters(void) {
 	efiPrintf("TunerStudio size=%d / total=%d / errors=%d / H=%d / O=%d / P=%d / B=%d",
 			sizeof(tsOutputChannels), tsState.totalCounter, tsState.errorCounter, tsState.queryCommandCounter,
@@ -119,6 +106,19 @@ static void printErrorCounters(void) {
 			tsState.writeChunkCommandCounter, tsState.pageCommandCounter);
 }
 
+#if EFI_TUNER_STUDIO
+
+/* 1S */
+#define TS_COMMUNICATION_TIMEOUT	TIME_MS2I(1000)
+
+extern persistent_config_container_s persistentState;
+
+static efitimems_t previousWriteReportMs = 0;
+
+static void resetTs(void) {
+	memset(&tsState, 0, sizeof(tsState));
+}
+
 void printTsStats(void) {
 #if EFI_PROD_CODE
 #ifdef EFI_CONSOLE_RX_BRAIN_PIN
@@ -161,6 +161,8 @@ static void bluetoothSPP(const char *baudRate, const char *name, const char *pin
 }
 #endif  /* EFI_BLUETOOTH_SETUP */
 
+#endif // EFI_TUNER_STUDIO
+
 void tunerStudioDebug(TsChannelBase* tsChannel, const char *msg) {
 #if EFI_TUNER_STUDIO_VERBOSE
 	efiPrintf("%s: %s", tsChannel->name, msg);
@@ -209,7 +211,7 @@ static void handlePageSelectCommand(TsChannelBase *tsChannel, ts_response_format
  * On the contrary, 'hard parameters' are waiting for the Burn button to be clicked and configuration version
  * would be increased and much more complicated logic would be executed.
  */
-static void onlineApplyWorkingCopyBytes(uint32_t offset, int count) {
+static void onlineApplyWorkingCopyBytes(uint32_t offset, int count DECLARE_ENGINE_PARAMETER_SUFFIX) {
 	if (offset >= sizeof(engine_configuration_s)) {
 		int maxSize = sizeof(persistent_config_s) - offset;
 		if (count > maxSize) {
@@ -218,7 +220,7 @@ static void onlineApplyWorkingCopyBytes(uint32_t offset, int count) {
 		}
 		efiPrintf("applying soft change from %d length %d", offset, count);
 #if !defined(EFI_NO_CONFIG_WORKING_COPY)
-		memcpy(((char*) &persistentState.persistentConfiguration) + offset, ((char*) &configWorkingCopy) + offset,
+		memcpy(((char*)config) + offset, ((char*) &configWorkingCopy) + offset,
 				count);
 #endif /* EFI_NO_CONFIG_WORKING_COPY */
 
@@ -230,6 +232,8 @@ static void onlineApplyWorkingCopyBytes(uint32_t offset, int count) {
 	// is negligable comparing with the IO costs?
 }
 
+#if EFI_TUNER_STUDIO
+
 static const void * getStructAddr(live_data_e structId) {
 	switch (structId) {
 	case LDS_ENGINE:
@@ -277,6 +281,8 @@ static void handleGetStructContent(TsChannelBase* tsChannel, int structId, int s
 	tsChannel->sendResponse(TS_CRC, (const uint8_t *)addr, size);
 }
 
+#endif // EFI_TUNER_STUDIO
+
 // Validate whether the specified offset and count would cause an overrun in the tune.
 // Returns true if an overrun would occur.
 static bool validateOffsetCount(size_t offset, size_t count, TsChannelBase* tsChannel) {
@@ -299,8 +305,8 @@ bool rebootForPresetPending = false;
  * This command is needed to make the whole transfer a bit faster
  * @note See also handleWriteValueCommand
  */
-static void handleWriteChunkCommand(TsChannelBase* tsChannel, ts_response_format_e mode, uint16_t offset, uint16_t count,
+void handleWriteChunkCommand(TsChannelBase* tsChannel, ts_response_format_e mode, uint16_t offset, uint16_t count,
-		void *content) {
+		void *content DECLARE_ENGINE_PARAMETER_SUFFIX) {
 	tsState.writeChunkCommandCounter++;
 
 	efiPrintf("WRITE CHUNK mode=%d o=%d s=%d", mode, offset, count);
@@ -313,12 +319,14 @@ static void handleWriteChunkCommand(TsChannelBase* tsChannel, ts_response_format
 	if (!rebootForPresetPending) {
 		uint8_t * addr = (uint8_t *) (getWorkingPageAddr() + offset);
 		memcpy(addr, content, count);
-		onlineApplyWorkingCopyBytes(offset, count);
+		onlineApplyWorkingCopyBytes(offset, count PASS_ENGINE_PARAMETER_SUFFIX);
 	}
 
 	sendOkResponse(tsChannel, mode);
 }
 
+#if EFI_TUNER_STUDIO
+
 static void handleCrc32Check(TsChannelBase *tsChannel, ts_response_format_e mode, uint16_t offset, uint16_t count) {
 	tsState.crc32CheckCommandCounter++;
 
@@ -386,12 +394,16 @@ static void handlePageReadCommand(TsChannelBase* tsChannel, ts_response_format_e
 #endif
 }
 
+#endif // EFI_TUNER_STUDIO
+
 void requestBurn(void) {
+#if !EFI_UNIT_TEST
 	onBurnRequest(PASS_ENGINE_PARAMETER_SIGNATURE);
 
 #if EFI_INTERNAL_FLASH
 	setNeedToWriteConfiguration();
 #endif
+#endif // !EFI_UNIT_TEST
 }
 
 static void sendResponseCode(ts_response_format_e mode, TsChannelBase *tsChannel, const uint8_t responseCode) {
@@ -403,7 +415,7 @@ static void sendResponseCode(ts_response_format_e mode, TsChannelBase *tsChannel
 /**
  * 'Burn' command is a command to commit the changes
  */
-static void handleBurnCommand(TsChannelBase* tsChannel, ts_response_format_e mode) {
+void handleBurnCommand(TsChannelBase* tsChannel, ts_response_format_e mode DECLARE_ENGINE_PARAMETER_SUFFIX) {
 	efitimems_t nowMs = currentTimeMillis();
 	tsState.burnCommandCounter++;
 
@@ -412,7 +424,7 @@ static void handleBurnCommand(TsChannelBase* tsChannel, ts_response_format_e mod
 	// Skip the burn if a preset was just loaded - we don't want to overwrite it
 	if (!rebootForPresetPending) {
 #if !defined(EFI_NO_CONFIG_WORKING_COPY)
-		memcpy(&persistentState.persistentConfiguration, &configWorkingCopy, sizeof(persistent_config_s));
+		memcpy(config, &configWorkingCopy, sizeof(persistent_config_s));
 #endif /* EFI_NO_CONFIG_WORKING_COPY */
 
 		requestBurn();
@@ -422,6 +434,8 @@ static void handleBurnCommand(TsChannelBase* tsChannel, ts_response_format_e mod
 	efiPrintf("BURN in %dms", currentTimeMillis() - nowMs);
 }
 
+#if EFI_TUNER_STUDIO
+
 static bool isKnownCommand(char command) {
 	return command == TS_HELLO_COMMAND || command == TS_READ_COMMAND || command == TS_OUTPUT_COMMAND
 			|| command == TS_PAGE_COMMAND || command == TS_BURN_COMMAND || command == TS_SINGLE_WRITE_COMMAND
@@ -567,6 +581,8 @@ void syncTunerStudioCopy(void) {
 #endif /* EFI_NO_CONFIG_WORKING_COPY */
 }
 
+#endif // EFI_TUNER_STUDIO
+
 tunerstudio_counters_s tsState;
 TunerStudioOutputChannels tsOutputChannels;
 
@@ -576,6 +592,8 @@ void tunerStudioError(TsChannelBase* tsChannel, const char *msg) {
 	tsState.errorCounter++;
 }
 
+#if EFI_TUNER_STUDIO
+
 /**
  * Query with CRC takes place while re-establishing connection
  * Query without CRC takes place on TunerStudio startup

firmware/console/binary/tunerstudio.h

@@ -9,10 +9,6 @@
 #include "global.h"
 #include "tunerstudio_io.h"
 
-#if EFI_TUNER_STUDIO
-#include "thread_controller.h"
-#include "thread_priority.h"
-
 typedef struct {
 	int queryCommandCounter;
 	int outputChannelsCommandCounter;
@@ -30,6 +26,16 @@ typedef struct {
 
 extern tunerstudio_counters_s tsState;
 
+void tunerStudioDebug(TsChannelBase* tsChannel, const char *msg);
+void tunerStudioError(TsChannelBase* tsChannel, const char *msg);
+
+char *getWorkingPageAddr();
+
+#if EFI_TUNER_STUDIO
+#include "thread_controller.h"
+#include "thread_priority.h"
+
+
 #define CONNECTIVITY_THREAD_STACK (2 * UTILITY_THREAD_STACK_SIZE)
 
 /**
@@ -42,11 +48,6 @@ bool handlePlainCommand(TsChannelBase* tsChannel, uint8_t command);
  */
 void handleQueryCommand(TsChannelBase* tsChannel, ts_response_format_e mode);
 
-char *getWorkingPageAddr();
-
-void tunerStudioDebug(TsChannelBase* tsChannel, const char *msg);
-void tunerStudioError(TsChannelBase* tsChannel, const char *msg);
-
 void updateTunerStudioState(TunerStudioOutputChannels *tsOutputChannels DECLARE_ENGINE_PARAMETER_SUFFIX);
 void printTsStats(void);
 void requestBurn(void);
@@ -84,3 +85,6 @@ public:
 };
 
 #endif /* EFI_TUNER_STUDIO */
+
+void handleWriteChunkCommand(TsChannelBase* tsChannel, ts_response_format_e mode, uint16_t offset, uint16_t count, void *content DECLARE_ENGINE_PARAMETER_SUFFIX);
+void handleBurnCommand(TsChannelBase* tsChannel, ts_response_format_e mode DECLARE_ENGINE_PARAMETER_SUFFIX);

unit_tests/mocks.cpp

@@ -32,3 +32,6 @@ MockInjectorModel2::~MockInjectorModel2() { }
 
 MockStepperHardware::MockStepperHardware() { }
 MockStepperHardware::~MockStepperHardware() { }
+
+MockTsChannel::MockTsChannel() : TsChannelBase("mock") { }
+MockTsChannel::~MockTsChannel() { }

unit_tests/mocks.h

@@ -7,6 +7,7 @@
 #include "airmass.h"
 #include "injector_model.h"
 #include "stepper.h"
+#include "tunerstudio_io.h"
 
 #include "gmock/gmock.h"
 
@@ -106,3 +107,12 @@ public:
 
 	MOCK_METHOD(bool, step, (bool positive), (override));
 };
+
+class MockTsChannel : public TsChannelBase {
+public:
+	MockTsChannel();
+	virtual ~MockTsChannel();
+
+	MOCK_METHOD(void, write, (const uint8_t* buffer, size_t size, bool isEndOfPacket), (override));
+	MOCK_METHOD(size_t, readTimeout, (uint8_t* buffer, size_t size, int timeout), (override));
+};

unit_tests/tests/test_tunerstudio.cpp

@@ -1,11 +1,12 @@
 #include "pch.h"
+#include "tunerstudio.h"
 #include "tunerstudio_io.h"
 
 static uint8_t st5TestBuffer[16000];
 
-class MockTsChannel : public TsChannelBase {
+class BufferTsChannel : public TsChannelBase {
 public:
-	MockTsChannel() : TsChannelBase("Test") { }
+	BufferTsChannel() : TsChannelBase("Test") { }
 
 	void write(const uint8_t* buffer, size_t size, bool /*isLastWriteInTransaction*/) override {
 		memcpy(&st5TestBuffer[writeIdx], buffer, size);
@@ -28,7 +29,7 @@ public:
 #define PAYLOAD "123"
 #define SIZE strlen(PAYLOAD)
 
-static void assertCrcPacket(MockTsChannel& dut) {
+static void assertCrcPacket(BufferTsChannel& dut) {
 	ASSERT_EQ(dut.writeIdx, SIZE + 7);
 
 	// todo: proper uint16 comparison
@@ -48,7 +49,7 @@ static void assertCrcPacket(MockTsChannel& dut) {
 }
 
 TEST(binary, testWriteCrc) {
-	MockTsChannel test;
+	BufferTsChannel test;
 
 	// Let it pick which impl (small vs large) to use
 	test.reset();
@@ -65,3 +66,44 @@ TEST(binary, testWriteCrc) {
 	test.writeCrcPacket(CODE, (const uint8_t*)PAYLOAD, SIZE);
 	assertCrcPacket(test);
 }
+
+TEST(TunerstudioCommands, writeChunkEngineConfig) {
+	WITH_ENGINE_TEST_HELPER(TEST_ENGINE);
+	MockTsChannel channel;
+
+	uint8_t* configBytes = reinterpret_cast<uint8_t*>(config);
+
+	// Contains zero before the write
+	configBytes[100] = 0;
+	EXPECT_EQ(configBytes[100], 0);
+
+	// two step - writes to the engineConfiguration section require a burn
+	uint8_t val = 50;
+	handleWriteChunkCommand(&channel, TS_CRC, 100, 1, &val PASS_ENGINE_PARAMETER_SUFFIX);
+
+	// hasn't changed yet
+	EXPECT_EQ(configBytes[100], 0);
+
+	handleBurnCommand(&channel, TS_CRC PASS_ENGINE_PARAMETER_SUFFIX);
+
+	EXPECT_EQ(configBytes[100], 50);
+}
+
+TEST(TunerstudioCommands, writeChunkOutsideEngineConfig) {
+	WITH_ENGINE_TEST_HELPER(TEST_ENGINE);
+	MockTsChannel channel;
+
+	uint8_t* configBytes = reinterpret_cast<uint8_t*>(config);
+
+	size_t offset = sizeof(engine_configuration_s) + 100;
+
+	// Contains zero before the write
+	configBytes[offset] = 0;
+	EXPECT_EQ(configBytes[offset], 0);
+
+	// one step - writes past engineConfiguration don't need a burn
+	uint8_t val = 50;
+	handleWriteChunkCommand(&channel, TS_CRC, offset, 1, &val PASS_ENGINE_PARAMETER_SUFFIX);
+
+	EXPECT_EQ(configBytes[offset], 50);
+}