FOME-Tech
/
fome-fw
mirror of https://github.com/FOME-Tech/fome-fw.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

ts refactoring (#439) 

* replace large buffer instances

* handlePageReadCommand uses locked

* cleanup

* explicitly call big vs. small packet implementation

* correctness

* unused UNUSED

* cleaner TS logging & cleanup

* about time we bumped this
This commit is contained in:
Matthew Kennedy 2024-06-09 20:26:27 -07:00 committed by GitHub
parent 6cc65b4c70
commit 2eb6c25313
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 65 additions and 165 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
firmware/console/binary/ts_can_channel.cpp
View File

@ -34,7 +34,7 @@ public:
void stop() override;
// Special override for writeCrcPacket for small packets
void writeCrcPacket(const uint8_t* buf, size_t size, bool allowLongPackets = false) override;
void copyAndWriteSmallCrcPacket(const uint8_t* buf, size_t size) override;
};
@ -52,7 +52,7 @@ void CanTsChannel::start() {
void CanTsChannel::stop() {
}
void CanTsChannel::writeCrcPacket(const uint8_t* buf, size_t size, bool allowLongPackets) {
void CanTsChannel::copyAndWriteSmallCrcPacket(const uint8_t* buf, size_t size) {
#ifdef TS_CAN_DEVICE_SHORT_PACKETS_IN_ONE_FRAME
// a special case for short packets: we can send them in 1 frame, without CRC & size,
// because the CAN protocol is already protected by its own checksum.
@ -68,7 +68,7 @@ void CanTsChannel::writeCrcPacket(const uint8_t* buf, size_t size, bool allowLon
#endif /* TS_CAN_DEVICE_SHORT_PACKETS_IN_ONE_FRAME */
// Packet too large, use default implementation
TsChannelBase::writeCrcPacket(buf, size, allowLongPackets);
TsChannelBase::copyAndWriteSmallCrcPacket(buf, size);
}
void CanTsChannel::write(const uint8_t* buffer, size_t size, bool) {

96
firmware/console/binary/tunerstudio.cpp
View File

@ -125,6 +125,8 @@ static void sendOkResponse(TsChannelBase *tsChannel) {
}
void sendErrorCode(TsChannelBase *tsChannel, uint8_t code) {
efiPrintf("TS <- Err: %d", code);
tsChannel->writeCrcResponse(code);
}
@ -135,6 +137,8 @@ void TunerStudio::sendErrorCode(TsChannelBase* tsChannel, uint8_t code) {
void TunerStudio::handlePageSelectCommand(TsChannelBase *tsChannel) {
tsState.pageCommandCounter++;
efiPrintf("TS -> Set page (no-op)");
sendOkResponse(tsChannel);
}
@ -154,7 +158,7 @@ void TunerStudio::handleWriteChunkCommand(TsChannelBase* tsChannel, uint16_t off
return;
}
efiPrintf("WRITE CHUNK o=%d s=%d", offset, count);
efiPrintf("TS -> Write chunk offset %d count %d", offset, count);
if (validateOffsetCount(offset, count, tsChannel)) {
return;
@ -183,8 +187,11 @@ void TunerStudio::handleCrc32Check(TsChannelBase *tsChannel, uint16_t offset, ui
const uint8_t* start = getWorkingPageAddr() + offset;
uint32_t crc = SWAP_UINT32(crc32(start, count));
tsChannel->writeCrcPacket((const uint8_t *) &crc, 4);
uint32_t crc = crc32(start, count);
efiPrintf("TS <- Get CRC offset %d count %d result %08x", offset, count, crc);
crc = SWAP_UINT32(crc);
tsChannel->copyAndWriteSmallCrcPacket((const uint8_t *) &crc, sizeof(crc));
}
/**
@ -192,15 +199,13 @@ void TunerStudio::handleCrc32Check(TsChannelBase *tsChannel, uint16_t offset, ui
* @note Writing values one by one is pretty slow
*/
void TunerStudio::handleWriteValueCommand(TsChannelBase* tsChannel, uint16_t offset, uint8_t value) {
UNUSED(tsChannel);
tsState.writeValueCommandCounter++;
if (isLockedFromUser()) {
sendErrorCode(tsChannel, TS_RESPONSE_UNRECOGNIZED_COMMAND);
return;
}
tunerStudioDebug(tsChannel, "got W (Write)"); // we can get a lot of these
efiPrintf("TS -> Write value offset %d value %d", offset, value);
if (validateOffsetCount(offset, 1, tsChannel)) {
return;
@ -222,9 +227,7 @@ void TunerStudio::handlePageReadCommand(TsChannelBase* tsChannel, uint16_t offse
return;
}
#if EFI_TUNER_STUDIO_VERBOSE
efiPrintf("READ offset=%d size=%d", offset, count);
#endif
efiPrintf("TS <- Read chunk offset %d count %d", offset, count);
if (validateOffsetCount(offset, count, tsChannel)) {
return;
@ -233,15 +236,12 @@ void TunerStudio::handlePageReadCommand(TsChannelBase* tsChannel, uint16_t offse
uint8_t* addr;
if (isLockedFromUser()) {
// to have rusEFI console happy just send all zeros within a valid packet
addr = (uint8_t*)&tsChannel->scratchBuffer + SCRATCH_BUFFER_PREFIX_SIZE;
addr = (uint8_t*)&tsChannel->scratchBuffer;
memset(addr, 0, count);
} else {
addr = getWorkingPageAddr() + offset;
}
tsChannel->writeCrcPacket(addr, count);
#if EFI_TUNER_STUDIO_VERBOSE
// efiPrintf("Sending %d done", count);
#endif
tsChannel->writeCrcPacketLocked(addr, count);
}
#endif // EFI_TUNER_STUDIO
@ -265,7 +265,7 @@ static void handleBurnCommand(TsChannelBase* tsChannel) {
tsState.burnCommandCounter++;
efiPrintf("got B (Burn)");
efiPrintf("TS -> Burn");
// Skip the burn if a preset was just loaded - we don't want to overwrite it
if (!rebootForPresetPending) {
@ -273,7 +273,7 @@ static void handleBurnCommand(TsChannelBase* tsChannel) {
}
tsChannel->writeCrcResponse(TS_RESPONSE_BURN_OK);
efiPrintf("BURN in %dms", (int)(t.getElapsedSeconds() * 1e3));
efiPrintf("Burned in %.1fms", t.getElapsedSeconds() * 1e3);
}
#if EFI_TUNER_STUDIO && (EFI_PROD_CODE || EFI_SIMULATOR)
@ -293,36 +293,6 @@ static bool isKnownCommand(char command) {
|| command == TS_QUERY_BOOTLOADER;
}
/**
* rusEfi own test command
*/
static void handleTestCommand(TsChannelBase* tsChannel) {
tsState.testCommandCounter++;
char testOutputBuffer[64];
/**
* this is NOT a standard TunerStudio command, this is my own
* extension of the protocol to simplify troubleshooting
*/
tunerStudioDebug(tsChannel, "got T (Test)");
tsChannel->write((const uint8_t*)GIT_HASH_SHORT, sizeof(GIT_HASH_SHORT));
chsnprintf(testOutputBuffer, sizeof(testOutputBuffer), " %d %d", engine->engineState.warnings.lastErrorCode, tsState.testCommandCounter);
tsChannel->write((const uint8_t*)testOutputBuffer, strlen(testOutputBuffer));
chsnprintf(testOutputBuffer, sizeof(testOutputBuffer), " uptime=%ds ", (int)getTimeNowS());
tsChannel->write((const uint8_t*)testOutputBuffer, strlen(testOutputBuffer));
chsnprintf(testOutputBuffer, sizeof(testOutputBuffer), __DATE__ " %s\r\n", PROTOCOL_TEST_RESPONSE_TAG);
tsChannel->write((const uint8_t*)testOutputBuffer, strlen(testOutputBuffer));
if (hasFirmwareError()) {
const char* error = getCriticalErrorMessage();
chsnprintf(testOutputBuffer, sizeof(testOutputBuffer), "error=%s\r\n", error);
tsChannel->write((const uint8_t*)testOutputBuffer, strlen(testOutputBuffer));
}
tsChannel->flush();
}
/**
* this command is part of protocol initialization
*
@ -331,17 +301,17 @@ static void handleTestCommand(TsChannelBase* tsChannel) {
*/
void TunerStudio::handleQueryCommand(TsChannelBase* tsChannel, ts_response_format_e mode) {
tsState.queryCommandCounter++;
#if EFI_TUNER_STUDIO_VERBOSE
efiPrintf("got S/H (queryCommand) mode=%d", mode);
printErrorCounters();
#endif
const char *signature = getTsSignature();
efiPrintf("TS <- Query signature: %s", signature);
printErrorCounters();
auto buffer = (const uint8_t *)signature;
size_t size = strlen(signature) + 1;
if (mode == TS_CRC) {
tsChannel->writeCrcPacket(buffer, size);
tsChannel->copyAndWriteSmallCrcPacket(buffer, size);
} else {
tsChannel->write(buffer, size, true);
tsChannel->flush();
@ -359,12 +329,8 @@ bool TunerStudio::handlePlainCommand(TsChannelBase* tsChannel, uint8_t command)
return false;
} else if (command == TS_HELLO_COMMAND || command == TS_QUERY_COMMAND) {
// We interpret 'Q' as TS_HELLO_COMMAND, since TS uses hardcoded 'Q' during ECU detection (scan all serial ports)
efiPrintf("Got naked Query command");
handleQueryCommand(tsChannel, TS_PLAIN);
return true;
} else if (command == TS_TEST_COMMAND || command == 'T') {
handleTestCommand(tsChannel);
return true;
} else if (command == TS_COMMAND_F) {
/**
* http://www.msextra.com/forums/viewtopic.php?f=122&t=48327
@ -554,7 +520,7 @@ extern CommandHandler console_line_callback;
static void handleGetVersion(TsChannelBase* tsChannel) {
char versionBuffer[32];
chsnprintf(versionBuffer, sizeof(versionBuffer), "FOME " QUOTE(SHORT_BOARD_NAME) " %d@" GIT_HASH_SHORT, getRusEfiVersion());
tsChannel->writeCrcPacket((const uint8_t *) versionBuffer, strlen(versionBuffer) + 1);
tsChannel->copyAndWriteSmallCrcPacket((const uint8_t *) versionBuffer, strlen(versionBuffer) + 1);
}
#if EFI_TEXT_LOGGING
@ -569,7 +535,7 @@ static void handleGetText(TsChannelBase* tsChannel) {
logMsg("get test sending [%d]\r\n", outputSize);
#endif
tsChannel->writeCrcPacket(reinterpret_cast<const uint8_t*>(output), outputSize, true);
tsChannel->writeCrcPacketLocked(reinterpret_cast<const uint8_t*>(output), outputSize);
#if EFI_SIMULATOR
logMsg("sent [%d]\r\n", outputSize);
#endif
@ -610,7 +576,6 @@ int TunerStudio::handleCrcCommand(TsChannelBase* tsChannel, uint8_t* data, int i
cmdOutputChannels(tsChannel, offset, count);
break;
case TS_HELLO_COMMAND:
tunerStudioDebug(tsChannel, "got Query command");
handleQueryCommand(tsChannel, TS_CRC);
break;
case TS_GET_FIRMWARE_VERSION:
@ -645,11 +610,6 @@ int TunerStudio::handleCrcCommand(TsChannelBase* tsChannel, uint8_t* data, int i
case TS_READ_COMMAND:
handlePageReadCommand(tsChannel, offset, count);
break;
case TS_TEST_COMMAND:
[[fallthrough]];
case 'T':
handleTestCommand(tsChannel);
break;
case TS_IO_TEST_COMMAND:
{
uint16_t subsystem = SWAP_UINT16(data16[0]);
@ -675,7 +635,7 @@ int TunerStudio::handleCrcCommand(TsChannelBase* tsChannel, uint8_t* data, int i
auto toothBuffer = GetToothLoggerBufferNonblocking();
if (toothBuffer) {
tsChannel->writeCrcPacket(reinterpret_cast<const uint8_t*>(toothBuffer->buffer), toothBuffer->nextIdx * sizeof(composite_logger_s), true);
tsChannel->writeCrcPacketLocked(reinterpret_cast<const uint8_t*>(toothBuffer->buffer), toothBuffer->nextIdx * sizeof(composite_logger_s));
ReturnToothLoggerBuffer(toothBuffer);
} else {
@ -696,7 +656,7 @@ int TunerStudio::handleCrcCommand(TsChannelBase* tsChannel, uint8_t* data, int i
const auto& buffer = triggerScopeGetBuffer();
if (buffer) {
tsChannel->writeCrcPacket(buffer.get<uint8_t>(), buffer.size(), true);
tsChannel->writeCrcPacketLocked(buffer.get<uint8_t>(), buffer.size());
} else {
// TS asked for a tooth logger buffer, but we don't have one to give it.
sendErrorCode(tsChannel, TS_RESPONSE_OUT_OF_RANGE);
@ -721,14 +681,14 @@ int TunerStudio::handleCrcCommand(TsChannelBase* tsChannel, uint8_t* data, int i
case TS_PERF_TRACE_GET_BUFFER:
{
auto trace = perfTraceGetBuffer();
tsChannel->writeCrcPacket(trace.get<uint8_t>(), trace.size(), true);
tsChannel->writeCrcPacketLocked(trace.get<uint8_t>(), trace.size());
}
break;
#endif /* ENABLE_PERF_TRACE */
case TS_GET_CONFIG_ERROR: {
const char* configError = getCriticalErrorMessage();
tsChannel->writeCrcPacket(reinterpret_cast<const uint8_t*>(configError), strlen(configError), true);
tsChannel->writeCrcPacketLocked(reinterpret_cast<const uint8_t*>(configError), strlen(configError));
break;
}
case TS_QUERY_BOOTLOADER: {
@ -737,7 +697,7 @@ int TunerStudio::handleCrcCommand(TsChannelBase* tsChannel, uint8_t* data, int i
bldata = TS_QUERY_BOOTLOADER_OPENBLT;
#endif
tsChannel->writeCrcPacket(&bldata, 1, false);
tsChannel->copyAndWriteSmallCrcPacket(&bldata, 1);
break;
}
default:

6
firmware/console/binary/tunerstudio_commands.cpp
View File

@ -56,15 +56,15 @@ void TunerStudio::cmdOutputChannels(TsChannelBase* tsChannel, uint16_t offset, u
tsState.outputChannelsCommandCounter++;
updateTunerStudioState();
tsChannel->assertPacketSize(count, false);
// this method is invoked too often to print any debug information
uint8_t * scratchBuffer = (uint8_t *)tsChannel->scratchBuffer;
/**
* collect data from all models
*/
copyRange(scratchBuffer + 3, getLiveDataFragments(), offset, count);
copyRange(scratchBuffer, getLiveDataFragments(), offset, count);
tsChannel->crcAndWriteBuffer(TS_RESPONSE_OK, count);
tsChannel->writeCrcPacketLocked(TS_RESPONSE_OK, scratchBuffer, count);
}
#endif // EFI_TUNER_STUDIO

76
firmware/console/binary/tunerstudio_io.cpp
View File

@ -21,7 +21,7 @@ size_t TsChannelBase::read(uint8_t* buffer, size_t size) {
#define isBigPacket(size) ((size) > BLOCKING_FACTOR + 7)
void TsChannelBase::copyAndWriteSmallCrcPacket(uint8_t responseCode, const uint8_t* buf, size_t size) {
void TsChannelBase::copyAndWriteSmallCrcPacket(const uint8_t* buf, size_t size) {
// don't transmit too large a buffer
efiAssertVoid(ObdCode::OBD_PCM_Processor_Fault, !isBigPacket(size), "copyAndWriteSmallCrcPacket tried to transmit too large a packet")
@ -30,57 +30,33 @@ void TsChannelBase::copyAndWriteSmallCrcPacket(uint8_t responseCode, const uint8
// tsOutputChannels) during the CRC computation. Instead compute the CRC on our
// local buffer that nobody else will write.
if (size) {
memcpy(scratchBuffer + SCRATCH_BUFFER_PREFIX_SIZE, buf, size);
memcpy(scratchBuffer, buf, size);
}
crcAndWriteBuffer(responseCode, size);
writeCrcPacketLocked(TS_RESPONSE_OK, &scratchBuffer[0], size);
}
void TsChannelBase::crcAndWriteBuffer(uint8_t responseCode, size_t size) {
efiAssertVoid(ObdCode::OBD_PCM_Processor_Fault, !isBigPacket(size), "crcAndWriteBuffer tried to transmit too large a packet")
// Index 0/1 = packet size (big endian)
*(uint16_t*)scratchBuffer = SWAP_UINT16(size + 1);
// Index 2 = response code
scratchBuffer[2] = responseCode;
// CRC is computed on the responseCode and payload but not length
uint32_t crc = crc32(&scratchBuffer[2], size + 1); // command part of CRC
// Place the CRC at the end
crc = SWAP_UINT32(crc);
memcpy(scratchBuffer + size + SCRATCH_BUFFER_PREFIX_SIZE, &crc, sizeof(crc));
// Write to the underlying stream
write(reinterpret_cast<uint8_t*>(scratchBuffer), size + 7, true);
flush();
}
uint32_t TsChannelBase::writePacketHeader(const uint8_t responseCode, const size_t size) {
void TsChannelBase::writeCrcPacketLocked(const uint8_t responseCode, const uint8_t* buf, const size_t size) {
uint8_t headerBuffer[3];
*(uint16_t*)headerBuffer = SWAP_UINT16(size + 1);
*(uint8_t*)(headerBuffer + 2) = responseCode;
// Write header
write(headerBuffer, sizeof(headerBuffer), /*isEndOfPacket*/false);
// Command part of CRC
return crc32((void*)(headerBuffer + 2), 1);
}
void TsChannelBase::writeCrcPacketLarge(const uint8_t responseCode, const uint8_t* buf, const size_t size) {
uint8_t crcBuffer[4];
// Command part of CRC
uint32_t crc = writePacketHeader(responseCode, size);
// Data part of CRC
crc = crc32inc((void*)buf, crc, size);
*(uint32_t*)crcBuffer = SWAP_UINT32(crc);
// If data, write that
if (size) {
write(buf, size, /*isEndOfPacket*/false);
}
// Command part of CRC
uint32_t crc = crc32((void*)(headerBuffer + 2), 1);
// Data part of CRC
crc = crc32inc((void*)buf, crc, size);
uint8_t crcBuffer[4];
*(uint32_t*)crcBuffer = SWAP_UINT32(crc);
// Lastly the CRC footer
write(crcBuffer, sizeof(crcBuffer), /*isEndOfPacket*/true);
flush();
@ -90,29 +66,3 @@ TsChannelBase::TsChannelBase(const char *name)
: m_name(name)
{
}
void TsChannelBase::assertPacketSize(size_t size, bool allowLongPackets) {
if (isBigPacket(size) && !allowLongPackets) {
firmwareError(ObdCode::OBD_PCM_Processor_Fault, "[USE PROPER CONSOLE VERSION ] disallowed long packet of size %d", size);
}
}
/**
* Adds size to the beginning of a packet and a crc32 at the end. Then send the packet.
*/
void TsChannelBase::writeCrcPacket(const uint8_t* buf, size_t size, bool allowLongPackets) {
// don't transmit a null buffer...
if (!buf) {
size = 0;
}
assertPacketSize(size, allowLongPackets);
if (isBigPacket(size)) {
// for larger packets we do not use a buffer for CRC calculation meaning data is now allowed to modify while pending
writeCrcPacketLarge(TS_RESPONSE_OK, buf, size);
} else {
// for small packets we use a buffer for CRC calculation
copyAndWriteSmallCrcPacket(TS_RESPONSE_OK, buf, size);
}
}

27
firmware/console/binary/tunerstudio_io.h
View File

@ -36,11 +36,6 @@ public:
// Base functions that use the above virtual implementation
size_t read(uint8_t* buffer, size_t size);
#ifdef EFI_CAN_SERIAL
virtual // CAN device needs this function to be virtual for small-packet optimization
#endif
void writeCrcPacket(const uint8_t* buf, size_t size, bool allowLongPackets = false);
/**
* See 'blockingFactor' in rusefi.ini
*/
@ -50,14 +45,21 @@ public:
return m_name;
}
void assertPacketSize(size_t size, bool allowLongPackets);
uint32_t writePacketHeader(const uint8_t responseCode, const size_t size);
void crcAndWriteBuffer(const uint8_t responseCode, const size_t size);
void copyAndWriteSmallCrcPacket(uint8_t responseCode, const uint8_t* buf, size_t size);
#ifdef EFI_CAN_SERIAL
virtual // CAN device needs this function to be virtual for small-packet optimization
#endif
// Use when buf could change during execution. Makes a copy before computing checksum.
void copyAndWriteSmallCrcPacket(const uint8_t* buf, size_t size);
// Use when buf cannot change during execution. Computes checksum without an extra copy.
void writeCrcPacketLocked(uint8_t responseCode, const uint8_t* buf, size_t size);
inline void writeCrcPacketLocked(const uint8_t* buf, size_t size) {
writeCrcPacketLocked(TS_RESPONSE_OK, buf, size);
}
// Write a response code with no data
void writeCrcResponse(uint8_t responseCode) {
writeCrcPacketLarge(responseCode, nullptr, 0);
inline void writeCrcResponse(uint8_t responseCode) {
writeCrcPacketLocked(responseCode, nullptr, 0);
}
/* When TsChannel is in "not in sync" state tsProcessOne will silently try to find
@ -74,9 +76,6 @@ public:
protected:
const char * const m_name;
private:
void writeCrcPacketLarge(uint8_t responseCode, const uint8_t* buf, size_t size);
};
// This class represents a channel for a physical async serial poart

6
firmware/integration/rusefi_config.txt
View File

@ -86,7 +86,7 @@
! so not forget to change fileVersion in rusefi.ini
! todo: this not needed in light of TS_SIGNATURE but rusEFI console still uses it. Need to migrate
! rusEFI console from TS_FILE_VERSION to TS_SIGNATURE :(
#define TS_FILE_VERSION 20210312
#define TS_FILE_VERSION 20240610
! This is the version of the data stored in flash configuration
! Any time an incompatible change is made to the configuration format stored in flash,
@ -1893,10 +1893,6 @@ end_struct
! Engine Sniffer time stamp unit, in microseconds
#define ENGINE_SNIFFER_UNIT_US 10
! These commands are used exclusively by the FOME console
! 0x74
#define TS_TEST_COMMAND 't'
! High speed logger commands
#define TS_SET_LOGGER_SWITCH 'l'

13
unit_tests/tests/test_tunerstudio.cpp
View File

@ -50,19 +50,14 @@ static void assertCrcPacket(BufferTsChannel& dut) {
TEST(binary, testWriteCrc) {
BufferTsChannel test;
// Let it pick which impl (small vs large) to use
// Small impl
test.reset();
test.writeCrcPacket((const uint8_t*)PAYLOAD, SIZE);
test.copyAndWriteSmallCrcPacket((const uint8_t*)PAYLOAD, SIZE);
assertCrcPacket(test);
// Force the large impl
// Large impl
test.reset();
test.writeCrcPacket((const uint8_t*)PAYLOAD, SIZE);
assertCrcPacket(test);
// Force the small impl
test.reset();
test.writeCrcPacket((const uint8_t*)PAYLOAD, SIZE);
test.writeCrcPacketLocked((const uint8_t*)PAYLOAD, SIZE);
assertCrcPacket(test);
}