fix atomic access to state variables

freemodbus/modbus/ascii/mbascii_m.c

@@ -83,7 +83,6 @@ typedef enum
 /* These Modbus values are shared in ASCII mode*/
 extern volatile UCHAR   ucMasterRcvBuf[];
 extern volatile UCHAR   ucMasterSndBuf[];
-extern volatile eMBMasterTimerMode eMasterCurTimerMode;
 
 /* ----------------------- Static functions ---------------------------------*/
 static UCHAR    prvucMBCHAR2BIN( UCHAR ucCharacter );

freemodbus/modbus/include/mb_m.h

@@ -95,6 +95,19 @@ typedef enum
     MB_MRE_EXE_FUN                  /*!< execute function error. */
 } eMBMasterReqErrCode;
 
+
+/*! \ingroup modbus
+ * \brief Transaction information structure.
+ */
+typedef struct
+{
+    uint64_t xTransId;
+    UCHAR ucDestAddr;
+    UCHAR ucFuncCode;
+    eMBException eException;
+    UCHAR ucFrameError;
+} TransactionInfo_t;
+
 /*! \ingroup modbus
  *  \brief TimerMode is Master 3 kind of Timer modes.
  */
@@ -107,9 +120,7 @@ typedef enum
 
 extern _lock_t xMBMLock; // Modbus lock object
 
-#define MB_ATOMIC_SECTION() CRITICAL_SECTION(xMBMLock)
+#define MB_ATOMIC_SECTION CRITICAL_SECTION(xMBMLock)
-#define MB_ATOMIC_STORE(PTR, DES) CRITICAL_STORE(xMBMLock, PTR, DES)
-#define MB_ATOMIC_LOAD(PTR) CRITICAL_LOAD(xMBMLock, PTR)
 
 /* ----------------------- Function prototypes ------------------------------*/
 /*! \ingroup modbus

freemodbus/modbus/mb_m.c

@@ -38,6 +38,7 @@
 /* ----------------------- System includes ----------------------------------*/
 #include <stdlib.h>
 #include <string.h>
+#include <stdatomic.h>
 
 /* ----------------------- Platform includes --------------------------------*/
 #include "port.h"
@@ -67,32 +68,29 @@
 #define MB_PORT_HAS_CLOSE 1
 #endif
 
-/* ----------------------- Static variables ---------------------------------*/
+/*------------------------ Shared variables ---------------------------------*/
-
-static volatile eMBMasterErrorEventType eMBMasterCurErrorType = EV_ERROR_INIT;
-static volatile USHORT usMasterSendPDULength;
-static volatile eMBMode eMBMasterCurrentMode;
-static uint64_t xCurTransactionId = 0;
 
 _lock_t xMBMLock; // base modbus object lock
+volatile UCHAR ucMasterSndBuf[MB_SERIAL_BUF_SIZE];
+volatile UCHAR ucMasterRcvBuf[MB_SERIAL_BUF_SIZE];
 
+static _Atomic USHORT usMasterSendPDULength = 0;
+static _Atomic eMBMasterErrorEventType eMBMasterCurErrorType = EV_ERROR_INIT;
+static _Atomic BOOL xMBRunInMasterMode = FALSE;
+static _Atomic UCHAR ucMBMasterDestAddress = 0;
+static _Atomic BOOL xFrameIsBroadcast = FALSE;
+
+static _Atomic eMBMasterTimerMode eMasterCurTimerMode;
+
+/* ----------------------- Static variables ---------------------------------*/
+static uint64_t xCurTransactionId = 0;
 static UCHAR *pucMBSendFrame = NULL;
 static UCHAR *pucMBRecvFrame = NULL;
 static UCHAR ucRecvAddress = 0;
+static eMBMode eMBMasterCurrentMode;
 
-static BOOL xMBRunInMasterMode =FALSE;
+/* The transaction information structure which keep last processing state */
-static UCHAR ucMBMasterDestAddress = 0;
+static TransactionInfo_t xTransactionInfo = {0};
-static UCHAR ucLastFunctionCode = 0;
-static UCHAR usLastFrameError = 0;
-static eMBException eLastException = MB_EX_NONE;
-static uint64_t xLastTransactionId = 0;
-
-/*------------------------ Shared variables ---------------------------------*/
-
-volatile UCHAR ucMasterSndBuf[MB_SERIAL_BUF_SIZE];
-volatile UCHAR ucMasterRcvBuf[MB_SERIAL_BUF_SIZE];
-volatile eMBMasterTimerMode eMasterCurTimerMode;
-volatile BOOL xFrameIsBroadcast = FALSE;
 
 static enum
 {
@@ -182,9 +180,21 @@ eMBMasterTCPInit( USHORT ucTCPPort )
         peMBMasterFrameSendCur = eMBMasterTCPSend;
         pxMBMasterPortCBTimerExpired = xMBMasterTCPTimerExpired;
         pvMBMasterFrameCloseCur = MB_PORT_HAS_CLOSE ? vMBMasterTCPPortClose : NULL;
-        ucMBMasterDestAddress = MB_TCP_PSEUDO_ADDRESS;
         eMBMasterCurrentMode = MB_TCP;
         eMBState = STATE_DISABLED;
+        ucRecvAddress = MB_TCP_PSEUDO_ADDRESS;
+        xCurTransactionId = 0;
+        xTransactionInfo.xTransId = 0;
+        xTransactionInfo.ucDestAddr = 0;
+        xTransactionInfo.ucFuncCode = 0;
+        xTransactionInfo.eException = MB_EX_NONE;
+        xTransactionInfo.ucFrameError = 0;
+        
+        /* initialize the state values. */
+        atomic_init(&usMasterSendPDULength, 0);
+        atomic_init(&eMBMasterCurErrorType, EV_ERROR_INIT);
+        atomic_init(&xMBRunInMasterMode, FALSE);
+        atomic_init(&ucMBMasterDestAddress, MB_TCP_PSEUDO_ADDRESS);
 
         // initialize the OS resource for modbus master.
         vMBMasterOsResInit();
@@ -192,13 +202,7 @@ eMBMasterTCPInit( USHORT ucTCPPort )
         {
             eStatus = MB_EPORTERR;
         }
-        /* initialize the state values. */
+
-        ucRecvAddress = MB_TCP_PSEUDO_ADDRESS;
-        ucLastFunctionCode = 0;
-        usLastFrameError = 0;
-        eLastException = MB_EX_NONE;
-        xCurTransactionId = 0;
-        eMBMasterCurErrorType = EV_ERROR_INIT;
     }
     return eStatus;
 }
@@ -256,13 +260,19 @@ eMBMasterSerialInit( eMBMode eMode, UCHAR ucPort, ULONG ulBaudRate, eMBParity eP
         else
         {
             eMBState = STATE_DISABLED;
-            /* initialize the state values. */
+            ucRecvAddress = 0;
-            ucRecvAddress = MB_TCP_PSEUDO_ADDRESS;
-            ucLastFunctionCode = 0;
-            usLastFrameError = 0;
-            eLastException = MB_EX_NONE;
             xCurTransactionId = 0;
-            eMBMasterCurErrorType = EV_ERROR_INIT;
+            xTransactionInfo.xTransId = 0;
+            xTransactionInfo.ucDestAddr = 0;
+            xTransactionInfo.ucFuncCode = 0;
+            xTransactionInfo.eException = MB_EX_NONE;
+            xTransactionInfo.ucFrameError = 0;
+            
+            /* initialize the state values. */
+            atomic_init(&usMasterSendPDULength, 0);
+            atomic_init(&eMBMasterCurErrorType, EV_ERROR_INIT);
+            atomic_init(&xMBRunInMasterMode, FALSE);
+            atomic_init(&ucMBMasterDestAddress, MB_TCP_PSEUDO_ADDRESS);
         }
         /* initialize the OS resource for modbus master. */
         vMBMasterOsResInit();
@@ -338,9 +348,9 @@ eMBMasterPoll( void )
     int                     j;
     eMBErrorCode            eStatus = MB_ENOERR;
     xMBMasterEventType      xEvent;
-    eMBMasterErrorEventType errorType;
+    eMBMasterErrorEventType errorType = EV_ERROR_INIT;
-    eMBException            eException = MB_EX_NONE;
+    static eMBException     eException = MB_EX_NONE;
-    UCHAR                   ucFunctionCode = 0;
+    static UCHAR            ucFunctionCode = 0;
     static USHORT           usRecvLength = 0;
 
     /* Check if the protocol stack is ready. */
@@ -371,7 +381,6 @@ eMBMasterPoll( void )
                     ESP_LOGE( MB_PORT_TAG, "%" PRIu64 ":Frame send error = %d", xEvent.xTransactionId, (unsigned)eStatus );
                 }
                 xCurTransactionId = xEvent.xTransactionId;
-                MB_ATOMIC_STORE(&(xLastTransactionId), xCurTransactionId);
                 break;
             case EV_MASTER_FRAME_SENT:
                 if (xCurTransactionId == xEvent.xTransactionId) {
@@ -418,7 +427,6 @@ eMBMasterPoll( void )
                     MB_PORT_CHECK(pucMBRecvFrame, MB_EILLSTATE, "receive buffer initialization fail.");
                     ESP_LOGD(MB_PORT_TAG, "%" PRIu64 ":EV_MASTER_EXECUTE", xEvent.xTransactionId);
                     ucFunctionCode = pucMBRecvFrame[MB_PDU_FUNC_OFF];
-                    MB_ATOMIC_STORE(&(ucLastFunctionCode), ucFunctionCode);
                     eException = MB_EX_ILLEGAL_FUNCTION;
                     /* If receive frame has exception. The receive function code highest bit is 1.*/
                     if (ucFunctionCode & MB_FUNC_ERROR) {
@@ -450,7 +458,6 @@ eMBMasterPoll( void )
                             }
                         }
                     }
-                    MB_ATOMIC_STORE(&(eLastException), eException);
                     /* If master has exception, will send error process event. Otherwise the master is idle.*/
                     if ( eException != MB_EX_NONE ) {
                         vMBMasterSetErrorType( EV_ERROR_EXECUTE_FUNCTION );
@@ -478,28 +485,24 @@ eMBMasterPoll( void )
                             vMBMasterErrorCBRespondTimeout( xEvent.xTransactionId,
                                                             ucMBMasterGetDestAddress( ),
                                                             pucMBSendFrame, usMBMasterGetPDUSndLength( ) );
-                            MB_ATOMIC_STORE(&(usLastFrameError), errorType);
                             break;
                         case EV_ERROR_RECEIVE_DATA:
                             vMBMasterErrorCBReceiveData( xEvent.xTransactionId,
                                                             ucMBMasterGetDestAddress( ),
                                                             pucMBRecvFrame, usRecvLength,
                                                             pucMBSendFrame, usMBMasterGetPDUSndLength( ) );
-                            MB_ATOMIC_STORE(&(usLastFrameError), errorType);
                             break;
                         case EV_ERROR_EXECUTE_FUNCTION:
                             vMBMasterErrorCBExecuteFunction( xEvent.xTransactionId,
                                                             ucMBMasterGetDestAddress( ),
                                                             pucMBRecvFrame, usRecvLength,
                                                             pucMBSendFrame, usMBMasterGetPDUSndLength( ) );
-                            MB_ATOMIC_STORE(&(usLastFrameError), errorType);
                             break;
                         case EV_ERROR_OK:
                             vMBMasterCBRequestSuccess( xEvent.xTransactionId,
                                                         ucMBMasterGetDestAddress( ),
                                                         pucMBRecvFrame, usRecvLength,
                                                         pucMBSendFrame, usMBMasterGetPDUSndLength( ) );
-                            MB_ATOMIC_STORE(&(usLastFrameError), errorType);
                             break;
                         default:
                             ESP_LOGE( MB_PORT_TAG, "%" PRIu64 ":incorrect error type = %d.", xEvent.xTransactionId, (int)errorType);
@@ -509,6 +512,13 @@ eMBMasterPoll( void )
                 vMBMasterPortTimersDisable( );
                 uint64_t xProcTime = xCurTransactionId ? ( xEvent.xPostTimestamp - xCurTransactionId ) : 0;
                 ESP_LOGD( MB_PORT_TAG, "Transaction (%" PRIu64 "), processing time(us) = %" PRId64, xCurTransactionId, xProcTime );
+                MB_ATOMIC_SECTION {
+                    xTransactionInfo.xTransId = xCurTransactionId;
+                    xTransactionInfo.ucDestAddr = atomic_load(&ucMBMasterDestAddress);
+                    xTransactionInfo.ucFuncCode = ucFunctionCode;
+                    xTransactionInfo.eException = eException;
+                    xTransactionInfo.ucFrameError = errorType;
+                }
                 xCurTransactionId = 0;
                 vMBMasterSetErrorType( EV_ERROR_INIT );
                 vMBMasterRunResRelease( );
@@ -528,37 +538,37 @@ eMBMasterPoll( void )
 // Get whether the Modbus Master is run in master mode.
 BOOL xMBMasterGetCBRunInMasterMode( void )
 {
-    return MB_ATOMIC_LOAD( &xMBRunInMasterMode);
+    return atomic_load(&xMBRunInMasterMode);
 }
 
 // Set whether the Modbus Master is run in master mode.
 void vMBMasterSetCBRunInMasterMode( BOOL IsMasterMode )
 {
-    MB_ATOMIC_STORE(&(xMBRunInMasterMode), IsMasterMode);
+    atomic_store(&xMBRunInMasterMode, IsMasterMode);
 }
 
 // Get Modbus Master send destination address.
 UCHAR ucMBMasterGetDestAddress( void )
 {
-    return MB_ATOMIC_LOAD( &ucMBMasterDestAddress);
+    return atomic_load(&ucMBMasterDestAddress);
 }
 
 // Set Modbus Master send destination address.
 void vMBMasterSetDestAddress( UCHAR Address )
 {
-    MB_ATOMIC_STORE(&(ucMBMasterDestAddress), Address);
+    atomic_store(&ucMBMasterDestAddress, Address);
 }
 
 // Get Modbus Master current error event type.
 eMBMasterErrorEventType inline eMBMasterGetErrorType( void )
 {
-    return MB_ATOMIC_LOAD(&eMBMasterCurErrorType);
+    return atomic_load(&eMBMasterCurErrorType);
 }
 
 // Set Modbus Master current error event type.
 void IRAM_ATTR vMBMasterSetErrorType( eMBMasterErrorEventType errorType )
 {
-    MB_ATOMIC_STORE(&(eMBMasterCurErrorType), errorType);
+    atomic_store(&eMBMasterCurErrorType, errorType);
 }
 
 /* Get Modbus Master send PDU's buffer address pointer.*/
@@ -570,37 +580,37 @@ void vMBMasterGetPDUSndBuf( UCHAR ** pucFrame )
 /* Set Modbus Master send PDU's buffer length.*/
 void vMBMasterSetPDUSndLength( USHORT SendPDULength )
 {
-    MB_ATOMIC_STORE(&(usMasterSendPDULength), SendPDULength);
+    atomic_store(&usMasterSendPDULength, SendPDULength);
 }
 
 /* Get Modbus Master send PDU's buffer length.*/
 USHORT usMBMasterGetPDUSndLength( void )
 {
-    return MB_ATOMIC_LOAD(&usMasterSendPDULength);
+    return atomic_load(&usMasterSendPDULength);
 }
 
 /* Set Modbus Master current timer mode.*/
 void vMBMasterSetCurTimerMode( eMBMasterTimerMode eMBTimerMode )
 {
-    MB_ATOMIC_STORE(&(eMasterCurTimerMode), eMBTimerMode);
+    atomic_store(&eMasterCurTimerMode, eMBTimerMode);
 }
 
 /* Get Modbus Master current timer mode.*/
 eMBMasterTimerMode MB_PORT_ISR_ATTR xMBMasterGetCurTimerMode( void )
 {
-    return MB_ATOMIC_LOAD(&eMasterCurTimerMode);
+    return atomic_load(&eMasterCurTimerMode);
 }
 
 /* The master request is broadcast? */
 BOOL MB_PORT_ISR_ATTR xMBMasterRequestIsBroadcast( void )
 {
-    return MB_ATOMIC_LOAD( &xFrameIsBroadcast);
+    return atomic_load(&xFrameIsBroadcast);
 }
 
 /* The master request is broadcast? */
 void vMBMasterRequestSetType( BOOL xIsBroadcast )
 {
-    MB_ATOMIC_STORE(&(xFrameIsBroadcast), xIsBroadcast);
+    atomic_store(&xFrameIsBroadcast, xIsBroadcast);
 }
 
 // Get Modbus Master communication mode.
@@ -616,13 +626,13 @@ BOOL xMBMasterGetLastTransactionInfo( uint64_t *pxTransId, UCHAR *pucDestAddress
 {
     BOOL xState = (eMBState == STATE_ENABLED);
     if (xState && pxTransId && pucDestAddress && pucFunctionCode
-        && pucException && pusErrorType) {
+            && pucException && pusErrorType) {
-        MB_ATOMIC_SECTION() {
+        MB_ATOMIC_SECTION {
-            *pxTransId = xLastTransactionId;
+            *pxTransId = xTransactionInfo.xTransId;
-            *pucDestAddress = ucMBMasterDestAddress;
+            *pucDestAddress = xTransactionInfo.ucDestAddr;
-            *pucFunctionCode = ucLastFunctionCode;
+            *pucFunctionCode = xTransactionInfo.ucFuncCode;
-            *pucException =  eLastException;
+            *pucException =  xTransactionInfo.eException;
-            *pusErrorType = usLastFrameError;
+            *pusErrorType = xTransactionInfo.ucFrameError;
         }
     }
     return xState;

freemodbus/port/port.h

@@ -154,28 +154,6 @@ void unlock_obj(_lock_t *plock);
 
 #define CRITICAL_SECTION(lock) for (int st = lock_obj((_lock_t *)&lock); (st > 0); unlock_obj((_lock_t *)&lock), st = -1)
 
-#define CRITICAL_STORE(LOCK, PTR, DES) \
-__extension__ \
-({  \
-    __auto_type __atomic_ptr = (PTR); \
-    __typeof__ ((void)0, *__atomic_ptr) __atomic_tmp = (DES); \
-    lock_obj((_lock_t *)&LOCK); \
-    *__atomic_ptr = __atomic_tmp; \
-    unlock_obj((_lock_t *)&LOCK); \
-    (__atomic_tmp); \
-})
-
-#define CRITICAL_LOAD(LOCK, PTR) \
-__extension__ \
-({  \
-    __auto_type __atomic_ptr = (PTR); \
-    __typeof__ ((void)0, *__atomic_ptr) __atomic_tmp; \
-    lock_obj((_lock_t *)&LOCK); \
-    __atomic_tmp = (*__atomic_ptr); \
-    unlock_obj((_lock_t *)&LOCK); \
-    (__atomic_tmp); \
-})
-
 #ifdef __cplusplus
 PR_BEGIN_EXTERN_C
 #endif /* __cplusplus */

freemodbus/port/portevent_m.c

@@ -35,6 +35,7 @@
  */
 
 /* ----------------------- Modbus includes ----------------------------------*/
+#include <stdatomic.h>
 
 #include "freertos/FreeRTOS.h"
 #include "freertos/task.h"
@@ -64,7 +65,7 @@ static EventGroupHandle_t xEventGroupMasterHdl;
 static EventGroupHandle_t xEventGroupMasterConfirmHdl;
 static QueueHandle_t xQueueMasterHdl;
 
-static uint64_t xTransactionID = 0;
+static _Atomic uint64_t xTransactionID = 0;
 
 /* ----------------------- Start implementation -----------------------------*/
 
@@ -78,7 +79,7 @@ xMBMasterPortEventInit( void )
     xQueueMasterHdl = xQueueCreate(MB_EVENT_QUEUE_SIZE, sizeof(xMBMasterEventType));
     MB_PORT_CHECK(xQueueMasterHdl, FALSE, "mb stack event group creation error.");
     vQueueAddToRegistry(xQueueMasterHdl, "MbMasterPortEventQueue");
-    xTransactionID = 0;
+    atomic_init(&xTransactionID, 0);
     return TRUE;
 }
 
@@ -91,7 +92,7 @@ xMBMasterPortEventPost( eMBMasterEventEnum eEvent)
     xEvent.xPostTimestamp = esp_timer_get_time();
     
     if (eEvent & EV_MASTER_TRANS_START) {
-        MB_ATOMIC_STORE(&(xTransactionID), xEvent.xPostTimestamp);
+        atomic_store(&(xTransactionID), xEvent.xPostTimestamp);
     }
     xEvent.eEvent = (eEvent & ~EV_MASTER_TRANS_START);
 
@@ -118,7 +119,7 @@ xMBMasterPortEventGet(xMBMasterEventType *peEvent)
     BOOL xEventHappened = FALSE;
 
     if (xQueueReceive(xQueueMasterHdl, peEvent, portMAX_DELAY) == pdTRUE) {
-        peEvent->xTransactionId = MB_ATOMIC_LOAD(&xTransactionID);
+        peEvent->xTransactionId = atomic_load(&xTransactionID);
         // Set event bits in confirmation group (for synchronization with port task)
         xEventGroupSetBits(xEventGroupMasterConfirmHdl, peEvent->eEvent);
         peEvent->xGetTimestamp = esp_timer_get_time();
@@ -145,7 +146,7 @@ xMBMasterPortFsmWaitConfirmation( eMBMasterEventEnum eEventMask, ULONG ulTimeout
 
 uint64_t xMBMasterPortGetTransactionId( )
 {
-    return MB_ATOMIC_LOAD(&xTransactionID);
+    return atomic_load(&xTransactionID);
 }
 
 // This function is initialize the OS resource for modbus master.