more improvements to the new read buffer

include/rs232.hpp

@@ -46,11 +46,11 @@ namespace sakurajin {
         std::atomic<bool> stopThread = false;
 
         std::string       readBuffer;
-        std::shared_mutex readBufferMutex;
+        std::timed_mutex  readBufferMutex;
         std::atomic<bool> readBufferHasData = false;
 
         std::string       writeBuffer;
-        std::shared_mutex writeBufferMutex;
+        std::timed_mutex  writeBufferMutex;
         std::atomic<bool> writeBufferHasData = false;
 
         /**
@@ -153,7 +153,7 @@ namespace sakurajin {
          * @return std::string the content of the read buffer
          */
         [[nodiscard]] [[maybe_unused]]
-        std::string&& retrieveReadBuffer();
+        std::string retrieveReadBuffer();
 
         /**
          * @brief load the read buffer and return the first match with a regex
@@ -166,7 +166,7 @@ namespace sakurajin {
          * @return std::string the first match of the read buffer
          */
         [[nodiscard]] [[maybe_unused]]
-        std::string&& retrieveFirstMatch(const std::regex& pattern);
+        std::string retrieveFirstMatch(const std::regex& pattern);
 
         /**
          * @brief print a string to the currently connected device
@@ -178,7 +178,7 @@ namespace sakurajin {
          * @param text the text to send
          */
         [[maybe_unused]]
-        void Print(const std::string& text);
+        void Print(std::string text);
 
 
         /**

samples/interfaceTest.cpp

@@ -3,36 +3,32 @@
 
 using namespace sakurajin;
 
-void printNextBuffer(RS232& h){
-    auto buffer = h.ReadNextMessage();
-
-    if(std::get<1>(buffer) < 0){
-        std::cerr << "Error while reading the buffer" << std::endl;
-    }else{
-        std::cout << std::get<0>(buffer) ;
-    }
+void printNextBuffer(RS232& h) {
+    auto buffer = h.retrieveReadBuffer();
+
+    std::cout << buffer;
 }
 
-int main (int argc, char **argv) {
+int main(int argc, char** argv) {
 
     // get the port form the parameter or set the default value
-    std::string serialPort = "";
+    std::string serialPort;
 
-    if(argc == 2){
+    if (argc == 2) {
         serialPort = argv[1];
-    }else{
-        #if defined(__unix__) || defined(__unix) || (defined(__APPLE__) && defined(__MACH__))
-            serialPort = "/dev/ttyUSB0";
-        #else
-            serialPort = "\\\\.\\COM3";
-        #endif
+    } else {
+#if defined(__unix__) || defined(__unix) || (defined(__APPLE__) && defined(__MACH__))
+        serialPort = "/dev/ttyUSB0";
+#else
+        serialPort = "\\\\.\\COM3";
+#endif
     }
-    
+
     // create the rs232 handle and check if the connection worked
-    RS232 h{serialPort,baud19200};
+    RS232 h{serialPort, baud19200};
 
-    if(!h.IsAvailable()){
-        std::cerr << "Serial port " << h.GetDeviceName() << " is not available!" << std::endl;
+    if (!h.IsAvailable()) {
+        std::cerr << "Serial port " << h.getCurrentDevice()->getDeviceName() << " is not available!" << std::endl;
         return -1;
     }
 
@@ -41,11 +37,11 @@ int main (int argc, char **argv) {
 
     // read the first message
     printNextBuffer(h);
-    
-    //send a message to the arduino
+
+    // send a message to the arduino
     h.Print("Hello from the other side!");
 
-    //get the response from the arduino
+    // get the response from the arduino
     printNextBuffer(h);
 
     // Closing port
@@ -54,4 +50,3 @@ int main (int argc, char **argv) {
 
     return 0;
 }
-

samples/readInfinite.cpp

@@ -36,7 +36,7 @@ int main (int argc, char **argv) {
 
         while (true) {
             //do a small delay to prevent too much locking
-            std::this_thread::sleep_for(std::chrono::milliseconds(100));
+            std::this_thread::sleep_for(std::chrono::milliseconds(10));
 
             //get the first match of the pattern in the read buffer
             auto result = rs232_interface.retrieveFirstMatch(pattern);
@@ -52,7 +52,7 @@ int main (int argc, char **argv) {
     }else{
         while(true){
             //do a small delay to prevent too much locking
-            std::this_thread::sleep_for(std::chrono::milliseconds(1));
+            std::this_thread::sleep_for(std::chrono::milliseconds(10));
 
             //read the string and save the result in a variable
             auto readString = rs232_interface.retrieveReadBuffer();

src/rs232.cpp

@@ -113,51 +113,66 @@ void sakurajin::RS232::work() {
     // if there is something to write to the device, write it
     if (writeBufferHasData) {
         // lock the mutex to prevent the buffer from being changed while it is being moved
+        // try lock is not used here because the buffer is only locked for a short time
+        // both the print function and the work function only do a copy/move operation
         writeBufferMutex.lock();
         auto localWriteBuffer = std::move(writeBuffer);
         writeBufferHasData    = false;
         writeBufferMutex.unlock();
 
         // write the data
-        for (auto c : localWriteBuffer) {
-            if (transferDevice->writeRawData(&c, 1) < 1) {
-                std::cerr << "error writing [" << c << "] to serial port";
-            }
+        auto err = native::Print(transferDevice, localWriteBuffer);
+        if (err < 0) {
+            std::cerr << "Error while writing to the device: " << err << std::endl;
+        }
+    }
+
+    // the read is a bit more complicated because the retrieve functions might block the code for a long time
+    // because of this the read is performed every call to work but first stored into a local static buffer.
+    static std::string queuedBuffer{};
+    char               IOBuf = '\0';
+    if (transferDevice->readRawData(&IOBuf, 1) > 0) {
+        if (queuedBuffer.empty()) {
+            queuedBuffer = std::string(1, IOBuf);
+        } else {
+            queuedBuffer.append(1, IOBuf);
         }
     }
 
-    // read the data
-    char    IOBuf    = '\0';
-    int64_t dataSize = transferDevice->readRawData(&IOBuf, 1);
-    if (dataSize > 0) {
-        // if there is data, add it to the buffer and set the hasData flag
-        readBufferMutex.lock();
+    // if there is data in the local buffer try locking the readBuffer mutex and add the data to the buffer
+    // if it takes too long to lock the mutex, try again during the next call to work
+    // this prevents long blocking of actual write operations while making sure no read data is lost.
+    if (!queuedBuffer.empty() && readBufferMutex.try_lock_for(1ms)) {
+        // if there is data, add it to the buffer
         if (readBufferHasData) {
-            readBuffer.append(1, IOBuf);
+            readBuffer.append(queuedBuffer);
+            queuedBuffer.clear();
         } else {
-            readBuffer = std::string(1, IOBuf);
+            // if there is no data, create a new buffer
+            readBuffer = std::move(queuedBuffer);
             readBuffer.reserve(10);
             readBufferHasData = true;
         }
+
         readBufferMutex.unlock();
     }
 }
 
 // io functions
-void sakurajin::RS232::Print(const std::string& text) {
+void sakurajin::RS232::Print(std::string text) {
     std::scoped_lock lock(writeBufferMutex);
 
     if (writeBufferHasData) {
         writeBuffer.append(text);
     } else {
-        writeBuffer        = text;
+        writeBuffer        = std::move(text);
         writeBufferHasData = true;
     }
 }
 
-std::string&& sakurajin::RS232::retrieveReadBuffer() {
+std::string sakurajin::RS232::retrieveReadBuffer() {
     if (!readBufferHasData) {
-        return std::string();
+        return std::string{};
     }
 
     std::scoped_lock lock(readBufferMutex);
@@ -166,21 +181,20 @@ std::string&& sakurajin::RS232::retrieveReadBuffer() {
     return std::move(readBuffer);
 }
 
-std::string&& sakurajin::RS232::retrieveFirstMatch(const std::regex& pattern) {
+std::string sakurajin::RS232::retrieveFirstMatch(const std::regex& pattern) {
     if (!readBufferHasData) {
-        return std::string();
+        return std::string{};
     }
 
     std::scoped_lock lock(readBufferMutex);
     std::smatch      s_match_result;
     std::regex_search(readBuffer, s_match_result, pattern);
     if (s_match_result.empty()) {
-        return std::string();
+        return std::string{};
     }
 
-    auto result = s_match_result.str();
-    readBuffer  = s_match_result.suffix();
-    return std::move(result);
+    readBuffer = s_match_result.suffix();
+    return s_match_result.str();
 }
 
 // device access functions