From 9f76dc4c6bc32e9c1d04013fccae46dadf76fc04 Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Sat, 10 Dec 2022 04:35:39 -0800 Subject: [PATCH] Use a shared buffer for "high memory" operations (#4878) * big buffer * mostly hooked up big buffer * bad merge * s * sneak preview of trigger scope * s * constness * channel limits * s * unnecessary * efilib * TcpServerSandbox * includes fix * binary format --- firmware/console/binary/output_channels.txt | 2 +- firmware/console/binary/trigger_scope.cpp | 42 ++++++++++++++ firmware/console/binary/trigger_scope.h | 5 ++ firmware/console/binary/tunerstudio.cpp | 39 +++++++++++-- firmware/console/binary/tunerstudio.mk | 4 +- firmware/controllers/core/big_buffer.cpp | 57 +++++++++++++++++++ firmware/controllers/core/big_buffer.h | 50 ++++++++++++++++ firmware/controllers/core/core.mk | 2 +- firmware/development/perf_trace.cpp | 36 ++++++------ firmware/development/perf_trace.h | 10 +--- firmware/integration/rusefi_config.txt | 12 ++-- firmware/tunerstudio/rusefi.input | 17 +++++- firmware/util/efilib.h | 27 +++++++++ .../binaryprotocol/test/TcpServerSandbox.java | 30 +++++----- 14 files changed, 279 insertions(+), 54 deletions(-) create mode 100644 firmware/console/binary/trigger_scope.cpp create mode 100644 firmware/console/binary/trigger_scope.h create mode 100644 firmware/controllers/core/big_buffer.cpp create mode 100644 firmware/controllers/core/big_buffer.h diff --git a/firmware/console/binary/output_channels.txt b/firmware/console/binary/output_channels.txt index d731b22f2a..5c541ca335 100644 --- a/firmware/console/binary/output_channels.txt +++ b/firmware/console/binary/output_channels.txt @@ -11,7 +11,7 @@ struct_no_prefix output_channels_s bit sd_present bit sd_logging_internal -bit unusedb4; +bit triggerScopeReady bit unusedb5; bit isFanOn;radiator fan bit isO2HeaterOn; diff --git a/firmware/console/binary/trigger_scope.cpp b/firmware/console/binary/trigger_scope.cpp new file mode 100644 index 0000000000..a5e7d4273e --- /dev/null +++ b/firmware/console/binary/trigger_scope.cpp @@ -0,0 +1,42 @@ +#include "pch.h" + +#include "trigger_scope.h" + +static BigBufferHandle buffer; + +static bool isRunning = false; + +// Enable one buffer's worth of perf tracing, and retrieve the buffer size in bytes +void triggerScopeEnable() { + buffer = getBigBuffer(BigBufferUser::TriggerScope); + + // TODO: trigger ADC + + isRunning = true; + engine->outputChannels.triggerScopeReady = true; +} + +void triggerScopeDisable() { + // we're done with the buffer - let somebody else have it + buffer = {}; + + isRunning = false; + engine->outputChannels.triggerScopeReady = false; +} + +static int theta = 0; + +// Retrieve the trace buffer +const BigBufferHandle& triggerScopeGetBuffer() { + if (buffer) { + for (size_t i = 0; i < buffer.size(); i++) + { + buffer.get()[i] = 128 + 100 * sin((float)theta / 50); + theta++; + } + } + + // engine->outputChannels.triggerScopeReady = false; + + return buffer; +} diff --git a/firmware/console/binary/trigger_scope.h b/firmware/console/binary/trigger_scope.h new file mode 100644 index 0000000000..ec5cd34bac --- /dev/null +++ b/firmware/console/binary/trigger_scope.h @@ -0,0 +1,5 @@ +#pragma once + +void triggerScopeEnable(); +void triggerScopeDisable(); +const BigBufferHandle& triggerScopeGetBuffer(); diff --git a/firmware/console/binary/tunerstudio.cpp b/firmware/console/binary/tunerstudio.cpp index 32e7dcdaaa..97c0f73b04 100644 --- a/firmware/console/binary/tunerstudio.cpp +++ b/firmware/console/binary/tunerstudio.cpp @@ -74,6 +74,7 @@ #include "bluetooth.h" #include "tunerstudio_io.h" #include "tooth_logger.h" +#include "trigger_scope.h" #include "electronic_throttle.h" #include "live_data.h" @@ -366,7 +367,6 @@ static bool isKnownCommand(char command) { || command == TS_IO_TEST_COMMAND || command == TS_GET_SCATTERED_GET_COMMAND || command == TS_SET_LOGGER_SWITCH - || command == TS_GET_LOGGER_GET_BUFFER || command == TS_GET_COMPOSITE_BUFFER_DONE_DIFFERENTLY || command == TS_GET_TEXT || command == TS_CRC_CHECK_COMMAND @@ -747,6 +747,36 @@ int TunerStudio::handleCrcCommand(TsChannelBase* tsChannel, char *data, int inco case TS_COMPOSITE_DISABLE: DisableToothLogger(); break; + case TS_COMPOSITE_READ: + { + auto toothBuffer = GetToothLoggerBuffer(); + + if (toothBuffer) { + tsChannel->sendResponse(TS_CRC, toothBuffer.Value.Buffer, toothBuffer.Value.Length, true); + } else { + // TS asked for a tooth logger buffer, but we don't have one to give it. + sendErrorCode(tsChannel, TS_RESPONSE_OUT_OF_RANGE); + } + } + break; + case TS_TRIGGER_SCOPE_ENABLE: + triggerScopeEnable(); + break; + case TS_TRIGGER_SCOPE_DISABLE: + triggerScopeDisable(); + break; + case TS_TRIGGER_SCOPE_READ: + { + const auto& buffer = triggerScopeGetBuffer(); + + if (buffer) { + tsChannel->sendResponse(TS_CRC, buffer.get(), buffer.size(), true); + } else { + // TS asked for a tooth logger buffer, but we don't have one to give it. + sendErrorCode(tsChannel, TS_RESPONSE_OUT_OF_RANGE); + } + } + break; default: // dunno what that was, send NAK return false; @@ -756,10 +786,9 @@ int TunerStudio::handleCrcCommand(TsChannelBase* tsChannel, char *data, int inco break; case TS_GET_COMPOSITE_BUFFER_DONE_DIFFERENTLY: - EnableToothLoggerIfNotEnabled(); - // falls through - case TS_GET_LOGGER_GET_BUFFER: { + EnableToothLoggerIfNotEnabled(); + auto toothBuffer = GetToothLoggerBuffer(); if (toothBuffer) { @@ -780,7 +809,7 @@ int TunerStudio::handleCrcCommand(TsChannelBase* tsChannel, char *data, int inco case TS_PERF_TRACE_GET_BUFFER: { auto trace = perfTraceGetBuffer(); - tsChannel->sendResponse(TS_CRC, trace.Buffer, trace.Size, true); + tsChannel->sendResponse(TS_CRC, trace.get(), trace.size(), true); } break; diff --git a/firmware/console/binary/tunerstudio.mk b/firmware/console/binary/tunerstudio.mk index 45e7866d29..c1162e1d43 100644 --- a/firmware/console/binary/tunerstudio.mk +++ b/firmware/console/binary/tunerstudio.mk @@ -8,4 +8,6 @@ TUNERSTUDIO_SRC_CPP = $(PROJECT_DIR)/console/binary/tunerstudio_io.cpp \ $(PROJECT_DIR)/console/binary/tunerstudio.cpp \ $(PROJECT_DIR)/console/binary/tunerstudio_commands.cpp \ $(PROJECT_DIR)/console/binary/bluetooth.cpp \ - $(PROJECT_DIR)/console/binary/signature.cpp + $(PROJECT_DIR)/console/binary/signature.cpp \ + $(PROJECT_DIR)/console/binary/trigger_scope.cpp \ + diff --git a/firmware/controllers/core/big_buffer.cpp b/firmware/controllers/core/big_buffer.cpp new file mode 100644 index 0000000000..c8ff5824fd --- /dev/null +++ b/firmware/controllers/core/big_buffer.cpp @@ -0,0 +1,57 @@ +#include "pch.h" + +#include "big_buffer.h" + +static BigBufferUser s_currentUser; +static uint8_t s_bigBuffer[BIG_BUFFER_SIZE]; + +static void releaseBuffer(void* bufferPtr, BigBufferUser user) { + if (bufferPtr != &s_bigBuffer || user != s_currentUser) { + // todo: panic! + } + + s_currentUser = BigBufferUser::None; +} + +BigBufferHandle::BigBufferHandle(void* buffer, BigBufferUser user) + : m_bufferPtr(buffer) + , m_user(user) +{ +} + +BigBufferHandle::BigBufferHandle(BigBufferHandle&& other) { + // swap contents of the two objects + m_bufferPtr = other.m_bufferPtr; + other.m_bufferPtr = nullptr; + + m_user = other.m_user; + other.m_user = BigBufferUser::None; +} + +BigBufferHandle& BigBufferHandle::operator= (BigBufferHandle&& other) { + // swap contents of the two objects + m_bufferPtr = other.m_bufferPtr; + other.m_bufferPtr = nullptr; + + m_user = other.m_user; + other.m_user = BigBufferUser::None; + + return *this; +} + +BigBufferHandle::~BigBufferHandle() { + if (m_bufferPtr) { + releaseBuffer(m_bufferPtr, m_user); + } +} + +BigBufferHandle getBigBuffer(BigBufferUser user) { + if (s_currentUser != BigBufferUser::None) { + // fatal + return {}; + } + + s_currentUser = user; + + return BigBufferHandle(s_bigBuffer, user); +} diff --git a/firmware/controllers/core/big_buffer.h b/firmware/controllers/core/big_buffer.h new file mode 100644 index 0000000000..1eaf223d4f --- /dev/null +++ b/firmware/controllers/core/big_buffer.h @@ -0,0 +1,50 @@ +// This file handles the "big buffer" - a shared buffer that can be used by multiple users depending on which function is enabled + +#pragma once + +#ifndef BIG_BUFFER_SIZE +#define BIG_BUFFER_SIZE 8192 +#endif + +enum class BigBufferUser { + None, + ToothLogger, + PerfTrace, + TriggerScope, +}; + +class BigBufferHandle { +public: + BigBufferHandle() = default; + BigBufferHandle(void* buffer, BigBufferUser user); + ~BigBufferHandle(); + + // But allow moving (passing ownership of the buffer) + BigBufferHandle(BigBufferHandle&& other); + BigBufferHandle& operator=(BigBufferHandle&&); + + // Implicit conversion operator to bool, so you can do things like if (myBuffer) { ... } as if it was a raw pointer + constexpr explicit operator bool() const { + return m_bufferPtr != nullptr; + } + + template + const TBuffer* get() const { + return reinterpret_cast(m_bufferPtr); + } + + template + TBuffer* get() { + return reinterpret_cast(m_bufferPtr); + } + + size_t size() const { + return BIG_BUFFER_SIZE; + } + +private: + void* m_bufferPtr = nullptr; + BigBufferUser m_user = BigBufferUser::None; +}; + +BigBufferHandle getBigBuffer(BigBufferUser user); diff --git a/firmware/controllers/core/core.mk b/firmware/controllers/core/core.mk index 67fddd7643..b230bc7159 100644 --- a/firmware/controllers/core/core.mk +++ b/firmware/controllers/core/core.mk @@ -1,4 +1,4 @@ CONTROLLERS_CORE_SRC_CPP = \ $(PROJECT_DIR)/controllers/core/state_sequence.cpp \ - + $(PROJECT_DIR)/controllers/core/big_buffer.cpp \ diff --git a/firmware/development/perf_trace.cpp b/firmware/development/perf_trace.cpp index f92f700d79..c6260b5b2a 100644 --- a/firmware/development/perf_trace.cpp +++ b/firmware/development/perf_trace.cpp @@ -6,24 +6,12 @@ * See JsonOutput.java in rusEfi console */ - #include "pch.h" - #ifndef ENABLE_PERF_TRACE #error ENABLE_PERF_TRACE must be defined! #endif -#ifndef TRACE_BUFFER_LENGTH -#define TRACE_BUFFER_LENGTH 2048 -#endif /* TRACE_BUFFER_LENGTH */ - -// Disable the buffer if we're not enabled at all -#if !ENABLE_PERF_TRACE -#undef TRACE_BUFFER_LENGTH -#define TRACE_BUFFER_LENGTH 1 -#endif - enum class EPhase : char { Start, @@ -44,12 +32,19 @@ struct TraceEntry // Ensure that the struct is the size we think it is - the binary layout is important static_assert(sizeof(TraceEntry) == 8); +#define TRACE_BUFFER_LENGTH (BIG_BUFFER_SIZE / sizeof(TraceEntry)) + // This buffer stores a trace - we write the full buffer once, then disable tracing -static TraceEntry s_traceBuffer[TRACE_BUFFER_LENGTH]; +static BigBufferHandle s_traceBuffer; static size_t s_nextIdx = 0; static bool s_isTracing = false; +static void stopTrace() { + s_isTracing = false; + s_nextIdx = 0; +} + static void perfEventImpl(PE event, EPhase phase) { // Bail if we aren't allowed to trace @@ -58,7 +53,7 @@ static void perfEventImpl(PE event, EPhase phase) } // Bail if we aren't tracing - if (!s_isTracing) { + if (!s_isTracing || !s_traceBuffer) { return; } @@ -79,8 +74,7 @@ static void perfEventImpl(PE event, EPhase phase) idx = s_nextIdx++; if (s_nextIdx >= TRACE_BUFFER_LENGTH) { - s_nextIdx = 0; - s_isTracing = false; + stopTrace(); } // Restore previous interrupt state - don't restore if they weren't enabled @@ -90,7 +84,7 @@ static void perfEventImpl(PE event, EPhase phase) } // We can safely write data out of the lock, our spot is reserved - volatile TraceEntry& entry = s_traceBuffer[idx]; + volatile TraceEntry& entry = s_traceBuffer.get()[idx]; entry.Event = event; entry.Phase = phase; @@ -122,12 +116,14 @@ void perfEventInstantGlobal(PE event) { } void perfTraceEnable() { + s_traceBuffer = getBigBuffer(BigBufferUser::PerfTrace); s_isTracing = true; } -const TraceBufferResult perfTraceGetBuffer() { +const BigBufferHandle perfTraceGetBuffer() { // stop tracing if you try to get the buffer early - s_isTracing = false; + stopTrace(); - return {reinterpret_cast(s_traceBuffer), sizeof(s_traceBuffer)}; + // transfer ownership of the buffer to the caller + return efi::move(s_traceBuffer); } diff --git a/firmware/development/perf_trace.h b/firmware/development/perf_trace.h index 9eac614e27..c546e82c3f 100644 --- a/firmware/development/perf_trace.h +++ b/firmware/development/perf_trace.h @@ -6,6 +6,8 @@ */ #pragma once +#include "big_buffer.h" + #include #include @@ -79,14 +81,8 @@ void perfEventInstantGlobal(PE event); // Enable one buffer's worth of perf tracing, and retrieve the buffer size in bytes void perfTraceEnable(); -struct TraceBufferResult -{ - const uint8_t* const Buffer; - const size_t Size; -}; - // Retrieve the trace buffer -const TraceBufferResult perfTraceGetBuffer(); +const BigBufferHandle perfTraceGetBuffer(); #if ENABLE_PERF_TRACE class ScopePerf diff --git a/firmware/integration/rusefi_config.txt b/firmware/integration/rusefi_config.txt index eb7a6553f0..7e277fa737 100644 --- a/firmware/integration/rusefi_config.txt +++ b/firmware/integration/rusefi_config.txt @@ -2063,9 +2063,6 @@ end_struct #define TS_GET_COMPOSITE_BUFFER_DONE_DIFFERENTLY '8' #define TS_GET_SCATTERED_GET_COMMAND '9' -#define TS_COMPOSITE_ENABLE 1 -#define TS_COMPOSITE_DISABLE 2 - ! Performance tracing #define TS_PERF_TRACE_BEGIN '_' #define TS_PERF_TRACE_GET_BUFFER 'b' @@ -2103,7 +2100,14 @@ end_struct ! High speed logger commands #define TS_SET_LOGGER_SWITCH 'l' -#define TS_GET_LOGGER_GET_BUFFER 'L' + +#define TS_COMPOSITE_ENABLE 1 +#define TS_COMPOSITE_DISABLE 2 +#define TS_COMPOSITE_READ 3 + +#define TS_TRIGGER_SCOPE_ENABLE 4 +#define TS_TRIGGER_SCOPE_DISABLE 5 +#define TS_TRIGGER_SCOPE_READ 6 #define PROTOCOL_COIL1_SHORT_NAME "c1" #define PROTOCOL_INJ1_SHORT_NAME "i1" diff --git a/firmware/tunerstudio/rusefi.input b/firmware/tunerstudio/rusefi.input index eb58a3a338..ce731324e7 100644 --- a/firmware/tunerstudio/rusefi.input +++ b/firmware/tunerstudio/rusefi.input @@ -113,7 +113,7 @@ enable2ndByteCanID = false loggerDef = compositeLogger, "Trigger Logger", composite startCommand = "@#TS_SET_LOGGER_SWITCH#@\x01" stopCommand = "@#TS_SET_LOGGER_SWITCH#@\x02" - dataReadCommand = "@#TS_GET_LOGGER_GET_BUFFER#@" + dataReadCommand = "@#TS_SET_LOGGER_SWITCH#@\x03" dataReadTimeout = 10000 ; time in ms dataReadyCondition = { toothLogReady } continuousRead = true @@ -133,6 +133,21 @@ enable2ndByteCanID = false ; it seems that TS also needs to know the diff.size of a tooth calcField = toothTime, "ToothTime", "ms", { time - pastValue(time, 1) } + loggerDef = triggerScope, "Trigger Oscilloscope", csv + startCommand = "@#TS_SET_LOGGER_SWITCH#@\x04" + stopCommand = "@#TS_SET_LOGGER_SWITCH#@\x05" + dataReadCommand = "@#TS_SET_LOGGER_SWITCH#@\x06" + dataReadTimeout = 1000 + dataReadyCondition = { triggerScopeReady } + continuousRead = true + + recordDef = 0, 0, 4 + recordField = lowBits1, "low", 0, 8, 1, "" , hidden + recordField = highBits1, "high", 8, 8, 1, "" , hidden + calcField = channel1, "Channel 1", "v", { 6.6 / 4096 * ( lowBits1 * 256 + highBits1 ) } + recordField = lowBits2, "low2", 16, 8, 1, "" , hidden + recordField = highBits2, "high2", 24, 8, 1, "" , hidden + calcField = channel2, "Channel 2", "v", { 6.6 / 4096 * ( lowBits2 * 256 + highBits2 ) } [VeAnalyze] diff --git a/firmware/util/efilib.h b/firmware/util/efilib.h index e540fb76fb..ef98383f63 100644 --- a/firmware/util/efilib.h +++ b/firmware/util/efilib.h @@ -123,4 +123,31 @@ static constexpr Gpio operator+(size_t a, Gpio b) { return b + a; } +namespace efi +{ +template +struct remove_reference { + using type = _Ty; +}; + +template +struct remove_reference<_Ty&> { + using type = _Ty; +}; + +template +struct remove_reference<_Ty&&> { + using type = _Ty; +}; + +template +using remove_reference_t = typename remove_reference<_Ty>::type; + +// FUNCTION TEMPLATE move +template +constexpr remove_reference_t<_Ty>&& move(_Ty&& _Arg) noexcept { + return static_cast&&>(_Arg); +} +} + #endif /* __cplusplus */ diff --git a/java_console/io/src/test/java/com/rusefi/binaryprotocol/test/TcpServerSandbox.java b/java_console/io/src/test/java/com/rusefi/binaryprotocol/test/TcpServerSandbox.java index 8dffe81ab6..a9ff6bbeac 100644 --- a/java_console/io/src/test/java/com/rusefi/binaryprotocol/test/TcpServerSandbox.java +++ b/java_console/io/src/test/java/com/rusefi/binaryprotocol/test/TcpServerSandbox.java @@ -102,22 +102,24 @@ public class TcpServerSandbox { } else if (command == Fields.TS_CRC_CHECK_COMMAND) { stream.sendPacket(BinaryProtocolServer.createCrcResponse(TOTALLY_EMPTY_CONFIGURATION)); } else if (command == Fields.TS_SET_LOGGER_SWITCH) { - stream.sendPacket(TS_OK.getBytes()); - } else if (command == Fields.TS_GET_LOGGER_GET_BUFFER) { - ByteArrayOutputStream baos = new ByteArrayOutputStream(); - baos.write(TS_OK.charAt(0)); - LittleEndianOutputStream dout = new LittleEndianOutputStream(baos); - int count = 256; - dout.writeShort(count * 5); + if (payload[1] == Fields.TS_COMPOSITE_READ) { + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + baos.write(TS_OK.charAt(0)); + LittleEndianOutputStream dout = new LittleEndianOutputStream(baos); + int count = 256; + dout.writeShort(count * 5); - for (int i = 0; i < count; i++) { - baos.write(i); - baos.write(0); - baos.write(0); - baos.write(0); - baos.write(100); + for (int i = 0; i < count; i++) { + baos.write(i); + baos.write(0); + baos.write(0); + baos.write(0); + baos.write(100); + } + stream.sendPacket(baos.toByteArray()); + } else { + stream.sendPacket(TS_OK.getBytes()); } - stream.sendPacket(baos.toByteArray()); } else if (command == Fields.TS_OUTPUT_COMMAND) { byte[] response = getOutputCommandResponse(payload, ecuState.outputs); stream.sendPacket(response);