From 32218fe854779362770092be22353c2e1a13f3c2 Mon Sep 17 00:00:00 2001
From: flocci <frank.locci@cern.sh>
Date: Fri, 8 Sep 2017 11:20:33 +0200
Subject: [PATCH] [SIL-310] Refactor the silecs-communication library low-level
 error handling

---
 .../interface/communication/MBConnection.cpp  | 307 ++++++++----------
 .../communication/SNAP7Connection.cpp         |  14 +-
 .../communication/SilecsConnection.cpp        |  13 +-
 3 files changed, 155 insertions(+), 179 deletions(-)

diff --git a/silecs-communication-cpp/src/silecs-communication/interface/communication/MBConnection.cpp b/silecs-communication-cpp/src/silecs-communication/interface/communication/MBConnection.cpp
index d957661..08ee03d 100644
--- a/silecs-communication-cpp/src/silecs-communication/interface/communication/MBConnection.cpp
+++ b/silecs-communication-cpp/src/silecs-communication/interface/communication/MBConnection.cpp
@@ -27,251 +27,228 @@
 namespace Silecs
 {
 
-  MBConnection::MBConnection(PLC* thePLC) : Connection(thePLC)
-  {
+MBConnection::MBConnection(PLC* thePLC) :
+                Connection(thePLC)
+{
     LOG(ALLOC) << "MBConnection (create): " << thePLC->getName();
 
-        //Connection use IP address to limit the naming-server accesses
-        readCtx_ = modbus_new_tcp((char *) thePLC->getIPAddress().c_str(), MODBUS_TCP_DEFAULT_PORT /*=502*/);
-        writeCtx_ = modbus_new_tcp((char *) thePLC->getIPAddress().c_str(), MODBUS_TCP_DEFAULT_PORT /*=502*/);
-
-        /*TODO: To be adjusted with the next stable libmodbus release (>3.1.1)
-          which will fix the current timeout response time issue (see libmodbus forum).
+    //Connection use IP address to limit the naming-server accesses
+    readCtx_ = modbus_new_tcp((char *)thePLC->getIPAddress().c_str(), MODBUS_TCP_DEFAULT_PORT /*=502*/);
+    writeCtx_ = modbus_new_tcp((char *)thePLC->getIPAddress().c_str(), MODBUS_TCP_DEFAULT_PORT /*=502*/);
 
-        Define Modbus response timeout
-        struct timeval response_timeout;
-        response_timeout.tv_sec = 0;
-        response_timeout.tv_usec = 10000;
+    /*TODO: To be adjusted with the next stable libmodbus release (>3.1.1)
+     which will fix the current timeout response time issue (see libmodbus forum).
 
-        modbus_set_response_timeout(readCtx_ , &response_timeout);
-        modbus_set_response_timeout(writeCtx_ , &response_timeout);
-        */
+     Define Modbus response timeout
+     struct timeval response_timeout;
+     response_timeout.tv_sec = 0;
+     response_timeout.tv_usec = 10000;
 
-        modbus_set_slave(readCtx_, 1);
-        //modbus_set_debug(readCtx_, TRUE);
-  }
+     modbus_set_response_timeout(readCtx_ , &response_timeout);
+     modbus_set_response_timeout(writeCtx_ , &response_timeout);
+     */
 
+    modbus_set_slave(readCtx_, 1);
+    //modbus_set_debug(readCtx_, TRUE);
+}
 
-  MBConnection::~MBConnection()
-  {
+MBConnection::~MBConnection()
+{
     //Close the connection before removing resources
     //disable(); must be done before removing resource
-        modbus_free(writeCtx_);
-        modbus_free(readCtx_);
-  }
-
-
-  bool MBConnection::open(PLC* thePLC)
-  {
-      int readErr = modbus_connect(readCtx_);
-        int writeErr = modbus_connect(writeCtx_);
-    return ((readErr != -1) && (writeErr != -1));
-  }
+    modbus_free(writeCtx_);
+    modbus_free(readCtx_);
+}
 
+bool MBConnection::open(PLC* thePLC)
+{
+    int readErr = modbus_connect(readCtx_);
+    int writeErr = modbus_connect(writeCtx_);
+    return ( (readErr != -1) && (writeErr != -1));
+}
 
-  bool MBConnection::close(PLC* thePLC)
-  {
+bool MBConnection::close(PLC* thePLC)
+{
     modbus_close(readCtx_);
-        modbus_close(writeCtx_);
+    modbus_close(writeCtx_);
     return true;
-  }
-
-
-  /*----------------------------------------------------------*/
-  int MBConnection::mbWriteFrames(modbus_t* ctx, uint16_t start_addr, uint16_t count, uint8_t* data)
-  {
-      uint16_t *srcp = (uint16_t*)data;
-      int word_count, byte_count;
-      int rc;
+}
 
-      while(count)
-      {
-          word_count = (count > MAX_WRITE_DATA_SIZE) ? MAX_WRITE_DATA_SIZE/2 : (count/2)+(count%2);
-          byte_count = (word_count * 2);
+/*----------------------------------------------------------*/
+int MBConnection::mbWriteFrames(modbus_t* ctx, long start_addr, uint16_t count, uint8_t* data)
+{
+    uint16_t *srcp = (uint16_t*)data;
+    int word_count, byte_count;
 
-          if ((rc = modbus_write_registers(ctx, start_addr, word_count, srcp)) != word_count)
-          {
-            return rc;
-          }
+    while (count)
+    {
+        word_count = (count > MAX_WRITE_DATA_SIZE) ? MAX_WRITE_DATA_SIZE / 2 : (count / 2) + (count % 2);
+        byte_count = (word_count * 2);
 
-          //srcp += byte_count-(count%2);
-          srcp += word_count;
-          start_addr += word_count;
-          count -= (byte_count - (count%2));
-      }
+        if (modbus_write_registers(ctx, start_addr, word_count, srcp) != word_count)
+        {
+            return -1;
+        }
 
-      return rc;
-  }
+        //srcp += byte_count-(count%2);
+        srcp += word_count;
+        start_addr += word_count;
+        count -= (byte_count - (count % 2));
+    }
 
+    return 0;
+}
 
-  /*----------------------------------------------------------*/
-  int MBConnection::mbReadFrames(modbus_t* ctx, uint16_t start_addr, uint16_t count, uint8_t* data)
-  {
+/*----------------------------------------------------------*/
+int MBConnection::mbReadFrames(modbus_t* ctx, long start_addr, uint16_t count, uint8_t* data)
+{
     uint16_t *destp = (uint16_t*)data;
     int word_count, byte_count;
-    int rc;
 
-    while(count) {
+    while (count)
+    {
 
         if (count > MAX_READ_DATA_SIZE)
         {
-           /*'word_count' is a word counter*/
-           word_count = MAX_READ_DATA_SIZE/2;
+            /*'word_count' is a word counter*/
+            word_count = MAX_READ_DATA_SIZE / 2;
         }
         else
         {
-          /*'word_count' is a word counter*/
-          word_count = (count/2) + (count%2);
+            /*'word_count' is a word counter*/
+            word_count = (count / 2) + (count % 2);
         }
 
         byte_count = (word_count * 2);
 
-        if ((rc = modbus_read_registers(ctx, start_addr, word_count, destp)) != word_count)
+        if (modbus_read_registers(ctx, start_addr, word_count, destp) != word_count)
         {
-            return rc;
+            return -1;
         }
 
         //destp += (byte_count-(count%2));
         destp += word_count;
         start_addr += word_count;
-        count -= (byte_count - (count%2));
+        count -= (byte_count - (count % 2));
     }
 
-    return rc;
-  }
-
-
-  int MBConnection::readData(PLC* thePLC, long address, unsigned long offset, unsigned long size, unsigned char* pBuffer)
-  {
-      int rc = 0;
-      int error = -1;
+    return 0;
+}
 
-      if (address < 0)
-      {
-          LOG(COMM) << "Invalid address: " << address;
-          throw SilecsException(__FILE__, __LINE__, PARAM_INCORRECT_BLOCK_ADDRESS, StringUtilities::toString(address));
-      }
+int MBConnection::readData(PLC* thePLC, long address, unsigned long offset, unsigned long size, unsigned char* pBuffer)
+{
+    int err = 0;
 
     //Schneider uses 16bit alignment memory. Block address is expressed in bytes, must be an even value!
-    if (address % 2) throw SilecsException(__FILE__, __LINE__, PARAM_INCORRECT_BLOCK_ADDRESS, StringUtilities::toString(address));
+    if (address % 2)
+        throw SilecsException(__FILE__, __LINE__, PARAM_INCORRECT_BLOCK_ADDRESS, StringUtilities::toString(address));
 
-     /* . There is one read-channel per PLC connection. It must be protected against concurrent access.
-    * . The write-channel is independent and can be accessed in parallel.
-    * . The global action (open,close,etc.) must be protected from any concurrent access.
-    * Attention!
-    * Mutexes are defined with recursive option. It allows doOpen method re-calling readData
-    * method by executing register synchronization if required.
-    */
+    /* . There is one read-channel per PLC connection. It must be protected against concurrent access.
+     * . The write-channel is independent and can be accessed in parallel.
+     * . The global action (open,close,etc.) must be protected from any concurrent access.
+     * Attention!
+     * Mutexes are defined with recursive option. It allows doOpen method re-calling readData
+     * method by executing register synchronization if required.
+     */
 
     //(re)connect the PLC if needed and (re)synchronize the retentive registers
     if (doOpen(thePLC))
     {
-      //connection is established then acquire data
-      readLock();
+        //connection is established then acquire data
+        readLock();
         //Schneider uses 16bit alignment memory. Block address is expressed in bytes (==> /2)
-        unsigned short addr = ((unsigned short)address + (unsigned short)offset)/2;
+        unsigned short addr = ((unsigned short)address + (unsigned short)offset) / 2;
 
         //DATA topic makes sense with RECV one
-        if (RECV & Log::topics_) LOG(DATA) << "Read data, address: %MW" << addr << ", byte-size: " << size;
-
-        rc = mbReadFrames(readCtx_, addr, (unsigned short)size, pBuffer);
-
-        error = checkError(thePLC, rc, false) ? rc : 0;
+        if (RECV & Log::topics_)
+            LOG(DATA) << "Read data, address: %MW" << addr << ", byte-size: " << size;
 
-      readUnlock();
+        err = mbReadFrames(readCtx_, addr, (unsigned short)size, pBuffer);
+        checkError(thePLC, err, false); // close the connection, will try again at the next access
+        readUnlock();
     }
-    return error;
-  }
-
-
-  int MBConnection::writeData(PLC* thePLC, long address, unsigned long offset, unsigned long size, unsigned char* pBuffer)
-  {
-      int rc = 0;
-      int error = -1;
+    return err;
+}
 
-      if (address < 0)
-      {
-          LOG(COMM) << "Invalid address: " << address;
-          throw SilecsException(__FILE__, __LINE__, PARAM_INCORRECT_BLOCK_ADDRESS, StringUtilities::toString(address));
-      }
+int MBConnection::writeData(PLC* thePLC, long address, unsigned long offset, unsigned long size, unsigned char* pBuffer)
+{
+    int err = 0;
 
     //Schneider uses 16bit alignment memory. Block address is expressed in bytes, must be an even value!
-    if (address % 2) throw SilecsException(__FILE__, __LINE__, PARAM_INCORRECT_BLOCK_ADDRESS, StringUtilities::toString(address));
+    if (address % 2)
+        throw SilecsException(__FILE__, __LINE__, PARAM_INCORRECT_BLOCK_ADDRESS, StringUtilities::toString(address));
 
-     /* . There is one read-channel per PLC connection. It must be protected against concurrent access.
-    * . The write-channel is independent and can be accessed in parallel.
-    * . The global action (open,close,etc.) must be protected from any concurrent access.
-    * Attention!
-    * Mutexes are defined with recursive option. It allows doOpen method re-calling sendData
-    * method by executing register synchronization if required.
-    */
+    /* . There is one read-channel per PLC connection. It must be protected against concurrent access.
+     * . The write-channel is independent and can be accessed in parallel.
+     * . The global action (open,close,etc.) must be protected from any concurrent access.
+     * Attention!
+     * Mutexes are defined with recursive option. It allows doOpen method re-calling sendData
+     * method by executing register synchronization if required.
+     */
 
     //(re)connect the PLC if needed and (re)synchronize the retentive registers
     if (doOpen(thePLC))
     {
-      //connection is established then send data
-      writeLock();
+        //connection is established then send data
+        writeLock();
         //Schneider uses 16bit alignment memory. Block address is expressed in bytes (==> /2)
-        unsigned short addr = ((unsigned short)address + (unsigned short)offset)/2;
+        unsigned short addr = ((unsigned short)address + (unsigned short)offset) / 2;
 
         //DATA topic makes sense with SEND one
-        if (SEND & Log::topics_) LOG(DATA) << "Write data, address: %MW" << addr << ", byte-size: " << size;
-
-        rc = mbWriteFrames(writeCtx_, (unsigned short)addr, (unsigned short)size, pBuffer);
-        error = checkError(thePLC, rc, false) ? rc : 0;
+        if (SEND & Log::topics_)
+            LOG(DATA) << "Write data, address: %MW" << addr << ", byte-size: " << size;
 
-      writeUnlock();
+        err = mbWriteFrames(writeCtx_, (unsigned short)addr, (unsigned short)size, pBuffer);
+        checkError(thePLC, err, false); // close the connection, will try again at the next access
+        writeUnlock();
     }
-    return error;
-  }
-
+    return err;
+}
 
-  int MBConnection::readMemory(PLC* thePLC, long address, unsigned long offset, unsigned long size, unsigned char* pBuffer)
-  {
+int MBConnection::readMemory(PLC* thePLC, long address, unsigned long offset, unsigned long size, unsigned char* pBuffer)
+{
     return readData(thePLC, address, offset, size, pBuffer);
-  }
-
+}
 
-  int MBConnection::writeMemory(PLC* thePLC, long address, unsigned long offset, unsigned long size, unsigned char* pBuffer)
-  {
+int MBConnection::writeMemory(PLC* thePLC, long address, unsigned long offset, unsigned long size, unsigned char* pBuffer)
+{
     return writeData(thePLC, address, offset, size, pBuffer);
-  }
-
+}
 
-  int MBConnection::readIO(PLC* thePLC, long address, unsigned long offset, unsigned long size, unsigned char* pBuffer)
-  {
+int MBConnection::readIO(PLC* thePLC, long address, unsigned long offset, unsigned long size, unsigned char* pBuffer)
+{
     return readData(thePLC, address, offset, size, pBuffer);
-  }
+}
 
-
-  int MBConnection::writeIO(PLC* thePLC, long address, unsigned long offset, unsigned long size, unsigned char* pBuffer)
-  {
+int MBConnection::writeIO(PLC* thePLC, long address, unsigned long offset, unsigned long size, unsigned char* pBuffer)
+{
     return writeData(thePLC, address, offset, size, pBuffer);
-  }
+}
 
+//-------------------------------------------------------------------------------------------------------------------
+bool MBConnection::checkError(PLC* thePLC, int err, bool retry)
+{
 
-  //-------------------------------------------------------------------------------------------------------------------
-  bool MBConnection::checkError(PLC* thePLC, int rc, bool retry)
-  {
-
-    if (rc == -1) {
-      LOG(COMM) << "Transaction failure with PLC: " << thePLC->getName() << " " << modbus_strerror(errno);
+    if (err != 0)
+    {
+        LOG(COMM) << "Transaction failure with the controller: " << thePLC->getName() << ". MODBUS[" << errno << "]: " << modbus_strerror(errno);
 
-      if (retry)
-      {
-        LOG(COMM) << "Try to reconnect the PLC: " << thePLC->getName();
-        if (reOpen(thePLC))
-        {   // can repeat the request after the connection was successfully reopened
-          return true;
+        if (retry)
+        {
+            LOG(COMM) << "Try to reconnect the controller: " << thePLC->getName();
+            if (reOpen(thePLC))
+            { // can repeat the request after the connection was successfully reopened
+                return true;
+            }
+            // reconnection has failed again, just close the connection
+            LOG(COMM) << "Unable to reconnect the controller: " << thePLC->getName();
         }
-        // reconnection has failed again, just close the connection
-        LOG(COMM) << "Unable to reconnect the PLC: " << thePLC->getName();
-      }
-      // no retry, we just want to close (use default case)
-      doClose(thePLC, /*withLock =*/true);
+        // no retry, we just want to close (use default case)
+        doClose(thePLC, /*withLock =*/true);
     }
     return false;
-  }
+}
 
 } // namespace
+
 #endif //MODBUS_SUPPORT_ENABLED
diff --git a/silecs-communication-cpp/src/silecs-communication/interface/communication/SNAP7Connection.cpp b/silecs-communication-cpp/src/silecs-communication/interface/communication/SNAP7Connection.cpp
index aff5c15..6bd09d3 100644
--- a/silecs-communication-cpp/src/silecs-communication/interface/communication/SNAP7Connection.cpp
+++ b/silecs-communication-cpp/src/silecs-communication/interface/communication/SNAP7Connection.cpp
@@ -215,7 +215,7 @@ int SNAP7Connection::readMemory(PLC* thePLC, long DBn, unsigned long offset, uns
     //connection is established then acquire data
     Lock lock(readMux_);
     //DATA topic makes sense with RECV one
-    if (RECV & Log::topics_) LOG(DATA) << "Read data, DBn: " << DBn << ", ofs: " << offset << ", byte-size: " << size;
+    if (RECV & Log::topics_) LOG(DATA) << "Read memory data, DBn: " << DBn << ", ofs: " << offset << ", byte-size: " << size;
 
     err = Cli_DBRead(recvClient_, (int)DBn, (int)offset, (int)size, (void *)pBuffer);
     checkError(thePLC, err, false); // close the connection, will try again at the next access
@@ -238,7 +238,7 @@ int SNAP7Connection::writeMemory(PLC* thePLC, long DBn, unsigned long offset, un
           //connection is established then send data
           Lock lock(writeMux_);
     //DATA topic makes sense with SEND one
-    if (SEND & Log::topics_) LOG(DATA) << "Write data, DBn: " << DBn << ", ofs: " << offset << ", byte-size: " << size;
+    if (SEND & Log::topics_) LOG(DATA) << "Write memory data, DBn: " << DBn << ", ofs: " << offset << ", byte-size: " << size;
 
     err = Cli_DBWrite(sendClient_, (int)DBn, (int)offset, (int)size, (void *)pBuffer);
     checkError(thePLC, err, false); // close the connection, will try again at the next access
@@ -261,7 +261,7 @@ int SNAP7Connection::readIO(PLC* thePLC, long address, unsigned long offset, uns
     //connection is established then acquire data
       Lock lock(readMux_);
     //DATA topic makes sense with RECV one
-    if (RECV & Log::topics_) LOG(DATA) << "Read data, address: " << address << ", ofs: " << offset << ", byte-size: " << size;
+    if (RECV & Log::topics_) LOG(DATA) << "Read IO data, address: " << address << ", ofs: " << offset << ", byte-size: " << size;
 
     address += offset;
     err = Cli_EBRead(recvClient_, (int)address, (int)size, (void *)pBuffer);
@@ -285,7 +285,7 @@ int SNAP7Connection::writeIO(PLC* thePLC, long address, unsigned long offset, un
             //connection is established then send data
             Lock lock(writeMux_);
             //DATA topic makes sense with SEND one
-            if (SEND & Log::topics_) LOG(DATA) << "Write data, address: " << address << ", ofs: " << offset << ", byte-size: " << size;
+            if (SEND & Log::topics_) LOG(DATA) << "Write IO data, address: " << address << ", ofs: " << offset << ", byte-size: " << size;
 
             address += offset;
             err = Cli_ABWrite(sendClient_, (int)address, (int)size, (void *)pBuffer);
@@ -299,17 +299,17 @@ bool SNAP7Connection::checkError(PLC* thePLC, int err, bool retry)
 {
     if (err != 0)
     {
-        LOG(COMM) << "Transaction failure with PLC: " << thePLC->getName() << " " << getErrorMessage(err);
+        LOG(COMM) << "Transaction failure with the controller: " << thePLC->getName() << " " << getErrorMessage(err);
 
         if (retry)
         {
-            LOG(COMM) << "Try to reconnect the PLC: " << thePLC->getName();
+            LOG(COMM) << "Try to reconnect the controller: " << thePLC->getName();
             if (reOpen(thePLC))
             { // can repeat the request after the connection was successfully reopened
                 return true;
             }
             // reconnection has failed again, just close the connection
-            LOG(COMM) << "Unable to reconnect the PLC: " << thePLC->getName();
+            LOG(COMM) << "Unable to reconnect the controller: " << thePLC->getName();
         }
         // no retry, we just want to close (use default case)
         doClose(thePLC, /*withLock =*/true);
diff --git a/silecs-communication-cpp/src/silecs-communication/interface/communication/SilecsConnection.cpp b/silecs-communication-cpp/src/silecs-communication/interface/communication/SilecsConnection.cpp
index 93cd441..5597a7b 100644
--- a/silecs-communication-cpp/src/silecs-communication/interface/communication/SilecsConnection.cpp
+++ b/silecs-communication-cpp/src/silecs-communication/interface/communication/SilecsConnection.cpp
@@ -442,7 +442,7 @@ namespace Silecs
 	  /*----------------------------------------------------------*/
 	  int rfcPing(char *ip, long ts)
 	  {
-	    int err, s, val = 1;
+	    int s, val = 1;
 	    struct protoent *pent;
 	    struct sockaddr_in rsock;
 
@@ -452,19 +452,19 @@ namespace Silecs
 	    if ((s=socket(AF_INET,SOCK_STREAM,0)) == -1)
 	    {
 	        LOG(ERROR) << "Can't create socket: " << strerror(errno);
-	        return RFC_SYS_ERROR;
+	        return -1;
 	    }
 
 	    /* Set TCP_NODELAY option to force immediate TCP/IP acknowledgement */
 	    if ((pent=getprotobyname("TCP")) == NULL)
 	    {
 	        LOG(ERROR) << "Can't configure socket: " << strerror(errno);
-	        return RFC_SYS_ERROR;
+	        return -1;
 	    }
 	    if (setsockopt(s,pent->p_proto,TCP_NODELAY,&val,4) == -1)
 	    {
 	        LOG(ERROR) << "Can't configure socket: " << strerror(errno);
-	        return RFC_SYS_ERROR;
+	        return -1;
 	    }
 
 	    rsock.sin_addr.s_addr=inet_addr(ip);
@@ -472,17 +472,16 @@ namespace Silecs
 	    /*check any port to detect if the host is OFF*/
 	    rsock.sin_port=htons(102);
 
-	    err = 0;
 	    if (connect_nonb(s,(struct sockaddr *)(&rsock),sizeof(rsock), ts) == -1)
 	    {
 	      /*if hostname is OFF, connect() fails on TIMEOUT*/
 	        if ((errno == ETIMEDOUT) || (errno == EHOSTDOWN) || (errno == EHOSTUNREACH)) {
-	        err = RFC_TIMEOUT_ERROR;
+	            return -1;
 	      }
 	    }
 
 	        close(s);
-	    return err;
+	        return 0; //controller was reachable
 	  }
 
 	  /*..........................................................*/
-- 
GitLab