From 0c2b7b8b3853897c572704cbfa28126c5dd803ad Mon Sep 17 00:00:00 2001 From: johannes Date: Wed, 1 Oct 2025 21:18:33 +0200 Subject: [PATCH 1/5] Here is an untested implementation of timeout mechanism for PutChar to handle #39 Call TriggerTimeout() in a fixed period interval --- include/cansdo.h | 2 ++ src/cansdo.cpp | 22 +++++++++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/include/cansdo.h b/include/cansdo.h index 65efe76..4395b51 100644 --- a/include/cansdo.h +++ b/include/cansdo.h @@ -65,6 +65,7 @@ class CanSdo: CanCallback, public IPutChar SdoFrame* GetPendingUserspaceSdo() { return pendingUserSpaceSdo ? &pendingUserSpaceSdoFrame : 0; } void SendSdoReply(SdoFrame* sdoFrame); void PutChar(char c); + void TriggerTimeout(int callingFrequency); private: CanHardware* canHardware; @@ -78,6 +79,7 @@ class CanSdo: CanCallback, public IPutChar volatile char printBuffer[64]; //Must be a power of 2 for efficient modulo calculation volatile uint32_t printByteIn; volatile uint32_t printByteOut; + volatile int printTimeout; //remaining time to wait Param::PARAM_NUM mapParam; uint32_t mapId; CanMap::CANPOS mapInfo; diff --git a/src/cansdo.cpp b/src/cansdo.cpp index 3241c6d..df3846d 100644 --- a/src/cansdo.cpp +++ b/src/cansdo.cpp @@ -32,6 +32,7 @@ #define PRINT_BUF_ENQUEUE(c) printBuffer[(printByteIn++) & (sizeof(printBuffer) - 1)] = c #define PRINT_BUF_DEQUEUE() printBuffer[(printByteOut++) & (sizeof(printBuffer) - 1)] #define PRINT_BUF_EMPTY() ((printByteOut - printByteIn) == sizeof(printBuffer)) +#define PRINT_TIMEOUT 1000 /** \brief * @@ -204,10 +205,28 @@ void CanSdo::ProcessSDO(uint32_t data[2]) canHardware->Send(0x580 + nodeId, data); } +/** \brief count down PutChar character send timeout + * + * \param callingFrequency in ms. This is subtracted from the remaining wait time + * \return void + * + */ +void CanSdo::TriggerTimeout(int callingFrequency) +{ + if (printTimeout == 0) return; + if (callingFrequency >= printTimeout) + printTimeout -= callingFrequency; + else + printTimeout = 0; +} + void CanSdo::PutChar(char c) { + if (printTimeout == 0) return; //last call to PutChar resulted in a timeout. Do not recover until the next burst + + printTimeout = PRINT_TIMEOUT; //When print buffer is full, wait - while (printByteIn == printByteOut); + while (printByteIn == printByteOut && printTimeout > 0); PRINT_BUF_ENQUEUE(c); printRequest = -1; //We can clear the print start trigger as we've obviously started printing @@ -227,6 +246,7 @@ bool CanSdo::ProcessSpecialSDOObjects(SdoFrame* sdo) { sdo->data = 65535; //this should be the size of JSON but we don't know this in advance. Hmm. sdo->cmd = SDO_RESPONSE_UPLOAD | SDO_SIZE_SPECIFIED; + printTimeout = PRINT_TIMEOUT; printByteIn = 0; printByteOut = sizeof(printBuffer); //both point to the beginning of the physical buffer but virtually they are 64 bytes apart printRequest = sdo->subIndex; From 05598f1b646b4e9dcf12cb1ec49d68aed370f8f4 Mon Sep 17 00:00:00 2001 From: "David J. Fiddes" <35607151+davefiddes@users.noreply.github.com> Date: Thu, 2 Oct 2025 22:05:56 +0100 Subject: [PATCH 2/5] Fix CAN SDO print timeout (#42) The class needs to be properly initialised as the TriggerTimeout() method will likely be called before any incoming SDO frames. On the VCU I observed unitialised member variables being filled with garbage from the stack painting. The previous TriggerTimeout() logic causes timeouts even when no timeout period has elapsed. This corrupts the parameter database download. Overshooting and setting to zero if required seems to work reliably. I don't understand. Tests: - Repeatedly start a db download, abort and immediate retry. This should fail and it does. - Repeatedly start a db download, abort and wait a few seconds to retry. This should succeed and it does. - Use callingFrequency values of 97 and 103 to force over and undershoots. Verify the timeout behaviour works as expected. --- src/cansdo.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/cansdo.cpp b/src/cansdo.cpp index df3846d..c00ca82 100644 --- a/src/cansdo.cpp +++ b/src/cansdo.cpp @@ -41,7 +41,10 @@ * */ CanSdo::CanSdo(CanHardware* hw, CanMap* cm) - : canHardware(hw), canMap(cm), nodeId(1), remoteNodeId(255), printRequest(-1) + : canHardware(hw), canMap(cm), nodeId(1), remoteNodeId(255), printRequest(-1), + printByteIn(0), printByteOut(sizeof(printBuffer)), printTimeout(PRINT_TIMEOUT), + mapParam(Param::PARAM_INVALID), mapId(0), sdoReplyValid(false), sdoReplyData(0), + pendingUserSpaceSdo(false) { canHardware->AddCallback(this); HandleClear(); @@ -213,11 +216,14 @@ void CanSdo::ProcessSDO(uint32_t data[2]) */ void CanSdo::TriggerTimeout(int callingFrequency) { - if (printTimeout == 0) return; - if (callingFrequency >= printTimeout) + if (printTimeout > 0) + { printTimeout -= callingFrequency; - else + } + if (printTimeout < 0) + { printTimeout = 0; + } } void CanSdo::PutChar(char c) From 0beae54646ae0a91a4f77d5b1ee379ce0e9fc3d1 Mon Sep 17 00:00:00 2001 From: "David J. Fiddes" <35607151+davefiddes@users.noreply.github.com> Date: Mon, 6 Oct 2025 21:51:36 +0100 Subject: [PATCH 3/5] Cleanup CanMap, CanSdo and Stm32Can declarations (#43) Make single parameter constructors explicit. Use "override" on all overridden virtual methods. Remove unused timestamp from CanMap. Remove unused GetFlashAddress from Stm32Can. Tests: - Build as part of VCU project --- include/canmap.h | 3 +-- include/cansdo.h | 6 +++--- include/stm32_can.h | 1 - 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/include/canmap.h b/include/canmap.h index a344111..b075b72 100644 --- a/include/canmap.h +++ b/include/canmap.h @@ -59,7 +59,7 @@ class CanMap: CanCallback uint8_t next; }; - CanMap(CanHardware* hw, bool loadFromFlash = true); + explicit CanMap(CanHardware* hw, bool loadFromFlash = true); CanHardware* GetHardware() { return canHardware; } void HandleClear() override; void HandleRx(uint32_t canId, uint32_t data[2], uint8_t dlc) override; @@ -95,7 +95,6 @@ class CanMap: CanCallback CANIDMAP canSendMap[MAX_MESSAGES]; CANIDMAP canRecvMap[MAX_MESSAGES]; CANPOS canPosMap[MAX_ITEMS + 1]; //Last item is a "tail" - uint32_t lastRxTimestamp; void ClearMap(CANIDMAP *canMap); int Add(CANIDMAP *canMap, Param::PARAM_NUM param, uint32_t canId, uint8_t offsetBits, int8_t length, float gain, int8_t offset); diff --git a/include/cansdo.h b/include/cansdo.h index 4395b51..af62027 100644 --- a/include/cansdo.h +++ b/include/cansdo.h @@ -52,9 +52,9 @@ class CanSdo: CanCallback, public IPutChar } __attribute__((packed)); /** Default constructor */ - CanSdo(CanHardware* hw, CanMap* cm = 0); + explicit CanSdo(CanHardware* hw, CanMap* cm = 0); CanHardware* GetHardware() { return canHardware; } - void HandleClear(); + void HandleClear() override; void HandleRx(uint32_t canId, uint32_t data[2], uint8_t dlc) override; void SDOWrite(uint8_t nodeId, uint16_t index, uint8_t subIndex, uint32_t data); void SDORead(uint8_t nodeId, uint16_t index, uint8_t subIndex); @@ -64,7 +64,7 @@ class CanSdo: CanCallback, public IPutChar int GetPrintRequest() { return printRequest; } SdoFrame* GetPendingUserspaceSdo() { return pendingUserSpaceSdo ? &pendingUserSpaceSdoFrame : 0; } void SendSdoReply(SdoFrame* sdoFrame); - void PutChar(char c); + void PutChar(char c) override; void TriggerTimeout(int callingFrequency); private: diff --git a/include/stm32_can.h b/include/stm32_can.h index 0487978..ed47682 100644 --- a/include/stm32_can.h +++ b/include/stm32_can.h @@ -51,7 +51,6 @@ class Stm32Can: public CanHardware void SetFilterBank(int& idIndex, int& filterId, uint16_t* idList); void SetFilterBankMask(int& idIndex, int& filterId, uint16_t* idMaskList); void SetFilterBank29(int& idIndex, int& filterId, uint32_t* idList); - uint32_t GetFlashAddress(); static Stm32Can* interfaces[]; }; From 9fdcdc642a98142d1f0f503e6de460971e901436 Mon Sep 17 00:00:00 2001 From: "David J. Fiddes" <35607151+davefiddes@users.noreply.github.com> Date: Mon, 6 Oct 2025 21:54:37 +0100 Subject: [PATCH 4/5] Allow concurrent sending of CAN frames - partial fix for issue #40 (#41) * Allow listing of errors via CANopen SDO (#37) This exports the list of errors and time at which they occurred as two CANopen SDO arrays. The sub- index is used as an index into the list of errors. Any invalid indices return zero. Clients can use the "lasterr" parameter database spot-value which is present in all libopeninv projects to map the error number to a string. Tests: - Verify with updated OpenInverter CAN Tool that errors can be listed. - Initiate an new error after a period of time and ensure the list tallies with the elapsed wall-clock time. - Check CANopen SDO decodes as expected in Wireshark * Allow concurrent sending of CAN frames - partial fix for issue #40 Stm32Can::Send() can be called from multiple contexts in a typical libopeninv application (main loop, CAN RX interrupt or timer interrupt(s)). To avoid CAN TX frames being lost we should disable interrupts while we manipulate the TX mailbox and queue. Tests: - On a ZombieVerterVCU experiencing issue #40 repeatedly request clean parameter databases with: while true; do oic cache clean && oic -n 3 -t 300 read opmode; done - Verify databases are downloaded correctly every time - Verify ox100 and 0x300 CAN frames sent to VW SBox continue to be sent * Fix hard realtime performance and have reliable CAN send This changes uses the capability of ARM Cortex-M3 CPUs to raise the interrupt priority mask so that low priority interrupts do not interrupt CAN sending. High priority interrupts form PWM, overcurrent and emergency stop in applications like stm32-sine will be processed unimpeded. High priority interrupts, obviously, cannot send CAN messages. The change requires: - CAN_MAX_IRQ_PRIORITY to be defined in hwdefs.h - The priority of all interrupts that can send CAN frames be 1 or lower (bigger number, lower priority) Tests: - Patch code into Stm32-vcu and stm32-sine - Torture test database download with: while true; do oic cache clean && oic read opmode; done - On stm32-sine set canperiod to 10ms - Ensure stm32-sine is in Run. Check jitter on exciter output with a DSO with infinite perrsistence. The new code shows no jitter. The older code shows a jitter of around 5.2 uSec. --- include/cortex.h | 34 ++++++++++++++++++++++++++++++++++ include/errormessage.h | 2 ++ src/cansdo.cpp | 30 ++++++++++++++++++++++++++++++ src/errormessage.cpp | 21 +++++++++++++++++++++ src/stm32_can.cpp | 10 +++++++++- 5 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 include/cortex.h diff --git a/include/cortex.h b/include/cortex.h new file mode 100644 index 0000000..1586c8b --- /dev/null +++ b/include/cortex.h @@ -0,0 +1,34 @@ +/* + * This file is part of the libopeninv project. + * + * Copyright (C) 2025 David J. Fiddes + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#ifndef CORTEX_H +#define CORTEX_H + +#include + +/** Do not mask any interrupts */ +#define CM_BASEPRI_ENABLE_INTERRUPTS 0 + +/** \brief Set the BASEPRI register to the given priority level */ +inline __attribute__((always_inline)) void cm_set_basepriority(uint32_t new_priority) +{ + __asm__ volatile("msr basepri, %0 " ::"r"(new_priority) : "memory"); +} + +#endif // CORTEX_H diff --git a/include/errormessage.h b/include/errormessage.h index b9f0560..76269e8 100644 --- a/include/errormessage.h +++ b/include/errormessage.h @@ -48,6 +48,8 @@ class ErrorMessage static void PrintAllErrors(); static void PrintNewErrors(); static ERROR_MESSAGE_NUM GetLastError(); + static ERROR_MESSAGE_NUM GetErrorNum(uint8_t index); + static uint32_t GetErrorTime(uint8_t index); protected: private: static void PrintError(uint32_t time, ERROR_MESSAGE_NUM err); diff --git a/src/cansdo.cpp b/src/cansdo.cpp index c00ca82..f783ab4 100644 --- a/src/cansdo.cpp +++ b/src/cansdo.cpp @@ -18,6 +18,7 @@ */ #include "cansdo.h" #include "my_math.h" +#include "errormessage.h" #define SDO_REQ_ID_BASE 0x600U #define SDO_REP_ID_BASE 0x580U @@ -28,6 +29,9 @@ #define SDO_INDEX_MAP_RX 0x3001 #define SDO_INDEX_MAP_RD 0x3100 #define SDO_INDEX_STRINGS 0x5001 +#define SDO_INDEX_ERROR_NUM 0x5002 +#define SDO_INDEX_ERROR_TIME 0x5003 + #define PRINT_BUF_ENQUEUE(c) printBuffer[(printByteIn++) & (sizeof(printBuffer) - 1)] = c #define PRINT_BUF_DEQUEUE() printBuffer[(printByteOut++) & (sizeof(printBuffer) - 1)] @@ -200,6 +204,32 @@ void CanSdo::ProcessSDO(uint32_t data[2]) { ReadOrDeleteCanMap(sdo); } + else if (sdo->index == SDO_INDEX_ERROR_NUM) + { + if (sdo->cmd == SDO_READ) + { + sdo->data = ErrorMessage::GetErrorNum(sdo->subIndex); + sdo->cmd = SDO_READ_REPLY; + } + else + { + sdo->cmd = SDO_ABORT; + sdo->data = SDO_ERR_INVIDX; + } + } + else if (sdo->index == SDO_INDEX_ERROR_TIME) + { + if (sdo->cmd == SDO_READ) + { + sdo->data = ErrorMessage::GetErrorTime(sdo->subIndex); + sdo->cmd = SDO_READ_REPLY; + } + else + { + sdo->cmd = SDO_ABORT; + sdo->data = SDO_ERR_INVIDX; + } + } else { if (!ProcessSpecialSDOObjects(sdo)) diff --git a/src/errormessage.cpp b/src/errormessage.cpp index 313fd72..20195a9 100644 --- a/src/errormessage.cpp +++ b/src/errormessage.cpp @@ -106,6 +106,27 @@ ERROR_MESSAGE_NUM ErrorMessage::GetLastError() return lastError; } +ERROR_MESSAGE_NUM ErrorMessage::GetErrorNum(uint8_t index) +{ + if (index < ERROR_BUF_SIZE) + { + if (errorBuffer[index].time > 0) + return errorBuffer[index].msg; + } + + return ERROR_NONE; +} + +uint32_t ErrorMessage::GetErrorTime(uint8_t index) +{ + if (index < ERROR_BUF_SIZE) + { + return errorBuffer[index].time; + } + + return 0; +} + /** Print all errors currently in error memory */ void ErrorMessage::PrintAllErrors() { diff --git a/src/stm32_can.cpp b/src/stm32_can.cpp index 16d0435..f35f4f8 100644 --- a/src/stm32_can.cpp +++ b/src/stm32_can.cpp @@ -28,6 +28,7 @@ #include #include #include "stm32_can.h" +#include "cortex.h" #define MAX_INTERFACES 2 #define IDS_PER_BANK 4 @@ -37,6 +38,10 @@ #define CAN_PERIPH_SPEED 36 #endif // CAN_PERIPH_SPEED +#ifndef CAN_MAX_IRQ_PRIORITY +#warning "CAN_MAX_IRQ_PRIORITY the highest interrupt priority users of the CAN interface may use is not defined" +#endif // CAN_MAX_IRQ_PRIORITY + struct CANSPEED { uint32_t ts1; @@ -198,6 +203,8 @@ void Stm32Can::SetBaudrate(enum baudrates baudrate) */ void Stm32Can::Send(uint32_t canId, uint32_t data[2], uint8_t len) { + cm_set_basepriority(CAN_MAX_IRQ_PRIORITY); + can_disable_irq(canDev, CAN_IER_TMEIE); if (can_transmit(canDev, canId, canId > 0x7FF, false, len, (uint8_t*)data) < 0 && sendCnt < SENDBUFFER_LEN) @@ -214,8 +221,9 @@ void Stm32Can::Send(uint32_t canId, uint32_t data[2], uint8_t len) { can_enable_irq(canDev, CAN_IER_TMEIE); } -} + cm_set_basepriority(CM_BASEPRI_ENABLE_INTERRUPTS); +} Stm32Can* Stm32Can::GetInterface(int index) { From 728953fd9ce9a1e425193c20fb884299e04d08f6 Mon Sep 17 00:00:00 2001 From: "David J. Fiddes" <35607151+davefiddes@users.noreply.github.com> Date: Wed, 8 Oct 2025 12:08:59 +0100 Subject: [PATCH 5/5] Don't force projects to define interrupt priorities to fix CAN send (#44) Realistically only stm32-sine needs to keep PWM timer interrupts running while CAN frames are being queued for sending. To ease updating of libopeninv just disable interrupts for all projects unless CAN_MAX_IRQ_PRIORITY is defined. Tests: - Verify correct operation on an otherwise unmodified Stm32-vcu - Verify correct operation with stm32-sine where CAN_MAX_IRQ_PRIORITY is defined --- src/stm32_can.cpp | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/src/stm32_can.cpp b/src/stm32_can.cpp index f35f4f8..20c709d 100644 --- a/src/stm32_can.cpp +++ b/src/stm32_can.cpp @@ -30,6 +30,13 @@ #include "stm32_can.h" #include "cortex.h" +//Some functions use the "register" keyword which C++ doesn't like +//We can safely ignore that as we don't even use those functions +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wregister" +#include +#pragma GCC diagnostic pop + #define MAX_INTERFACES 2 #define IDS_PER_BANK 4 #define EXT_IDS_PER_BANK 2 @@ -38,8 +45,19 @@ #define CAN_PERIPH_SPEED 36 #endif // CAN_PERIPH_SPEED -#ifndef CAN_MAX_IRQ_PRIORITY -#warning "CAN_MAX_IRQ_PRIORITY the highest interrupt priority users of the CAN interface may use is not defined" +// To allow concurrent sending of CAN frames from different contexts we need to +// disable interrupts. Some projects have hard realtime requirements which mean +// we cannot disable all interrupts. These projects should define the highest +// interrupt priority users of the CAN interface here. If not defined we assume +// that all interrupts can be disabled. +// +// CAN_MAX_IRQ_PRIORITY should match the priority passed to nvic_set_priority() +#ifdef CAN_MAX_IRQ_PRIORITY +#define DISABLE_CAN_USER_INTERRUPTS() cm_set_basepriority(CAN_MAX_IRQ_PRIORITY); +#define ENABLE_CAN_USER_INTERRUPTS() cm_set_basepriority(CM_BASEPRI_ENABLE_INTERRUPTS); +#else +#define DISABLE_CAN_USER_INTERRUPTS() cm_disable_interrupts() +#define ENABLE_CAN_USER_INTERRUPTS() cm_enable_interrupts() #endif // CAN_MAX_IRQ_PRIORITY struct CANSPEED @@ -203,7 +221,7 @@ void Stm32Can::SetBaudrate(enum baudrates baudrate) */ void Stm32Can::Send(uint32_t canId, uint32_t data[2], uint8_t len) { - cm_set_basepriority(CAN_MAX_IRQ_PRIORITY); + DISABLE_CAN_USER_INTERRUPTS(); can_disable_irq(canDev, CAN_IER_TMEIE); @@ -222,7 +240,7 @@ void Stm32Can::Send(uint32_t canId, uint32_t data[2], uint8_t len) can_enable_irq(canDev, CAN_IER_TMEIE); } - cm_set_basepriority(CM_BASEPRI_ENABLE_INTERRUPTS); + ENABLE_CAN_USER_INTERRUPTS(); } Stm32Can* Stm32Can::GetInterface(int index)