lancaster-university / codal-core

MIT License
11 stars 28 forks source link

Serial::send(ASYNC) should not block #147

Closed martinwork closed 1 year ago

martinwork commented 2 years ago

ASYNC mode should copy as many bytes as possible into the buffer and return immediately with the number of bytes copied. Only the SYNC modes should block until the entire input is buffered. https://github.com/lancaster-university/codal-core/blob/master/source/driver-models/Serial.cpp#L348 ASYNC blocks here: https://github.com/lancaster-university/codal-core/blob/master/source/driver-models/Serial.cpp#L100

microbit-carlos commented 1 year ago

If I understood https://github.com/lancaster-university/codal-microbit-v2/issues/149 correctly this issue was introduced by https://github.com/lancaster-university/codal-core/pull/146 and already fixed in this repo.

@martinwork is that right? Can you check if the right code is currently in the main branch in this repo? If so, I think this issue be closed?

martinwork commented 1 year ago

Yes, I think it's OK. Compared to Dec 2020 https://github.com/lancaster-university/codal-core/tree/e6b73051ed273a288536b523227915d24be01b79 there are now no differences in this area, except these comments https://github.com/lancaster-university/codal-core/compare/e6b7305..a45ede4#diff-9c169617506ceb70851131a3aae500835ba07baa5267976bb92914e4f8b6459aL183

The first difference in Serial.cpp is well below https://github.com/lancaster-university/codal-core/blob/master/source/driver-models/Serial.cpp#L99

git diff e6b7305 HEAD -- source\driver-models\Serial.cpp
diff --git a/source/driver-models/Serial.cpp b/source/driver-models/Serial.cpp
index 462f963..7d95649 100644
--- a/source/driver-models/Serial.cpp
+++ b/source/driver-models/Serial.cpp
@@ -256,7 +256,7 @@ void Serial::circularCopy(uint8_t *circularBuff, uint8_t circularBuffSize, uint8
  *
  *       Buffers aren't allocated until the first send or receive respectively.
  */
-Serial::Serial(Pin& tx, Pin& rx, uint8_t rxBufferSize, uint8_t txBufferSize, uint16_t id) : tx(tx), rx(rx)
+Serial::Serial(Pin& tx, Pin& rx, uint8_t rxBufferSize, uint8_t txBufferSize, uint16_t id) : tx(&tx), rx(&rx)
 {
     this->id = id;

@@ -275,6 +275,9 @@ Serial::Serial(Pin& tx, Pin& rx, uint8_t rxBufferSize, uint8_t txBufferSize, uin

     this->rxBuffHeadMatch = -1;

+    reassignPin(&this->tx, &tx);
+    reassignPin(&this->rx, &rx);
+
     this->status |= DEVICE_COMPONENT_STATUS_IDLE_TICK;
 }

@@ -769,6 +772,8 @@ ManagedString Serial::readUntil(ManagedString delimeters, SerialMode mode)
         eventOn(delimeters, mode);

         foundIndex = rxBuffHead - 1;
+        if (foundIndex < 0)
+            foundIndex += rxBuffSize;

         this->delimeters = ManagedString();
     }
@@ -839,11 +844,17 @@ int Serial::redirect(Pin& tx, Pin& rx)
     lockTx();
     lockRx();

+    reassignPin(&this->tx, &tx);
+    reassignPin(&this->rx, &rx);
+
     if(txBufferedSize() > 0)
         disableInterrupt(TxInterrupt);

     disableInterrupt(RxInterrupt);

+    // To be compatible with V1 behaviour
+    rx.setPull( PullMode::Up );
+
     configurePins(tx, rx);

     enableInterrupt(RxInterrupt);
@@ -883,12 +894,16 @@ int Serial::eventAfter(int len, SerialMode mode)
     if(mode == SYNC_SPINWAIT)
         return DEVICE_INVALID_PARAMETER;

+    // Schedule this fiber to wake on an event from the serial port, if necessary
+    if(mode == SYNC_SLEEP)
+        fiber_wake_on_event(this->id, CODAL_SERIAL_EVT_HEAD_MATCH);
+
     //configure our head match...
     this->rxBuffHeadMatch = (rxBuffHead + len) % rxBuffSize;

-    //block!
+    // Deschedule this fiber, if necessary
     if(mode == SYNC_SLEEP)
-        fiber_wait_for_event(this->id, CODAL_SERIAL_EVT_HEAD_MATCH);
+        schedule();

     return DEVICE_OK;
 }
@@ -980,7 +995,10 @@ int Serial::setRxBufferSize(uint8_t size)
     lockRx();

     // + 1 so there is a usable buffer size, of the size the user requested.
-    this->rxBuffSize = size + 1;
+    if (size != 255)
+        size++;
+
+    this->rxBuffSize = size;

     int result = initialiseRx();

@@ -1005,7 +1023,10 @@ int Serial::setTxBufferSize(uint8_t size)
     lockTx();

     // + 1 so there is a usable buffer size, of the size the user requested.
-    this->txBuffSize = size + 1;
+    if (size != 255)
+        size++;
+
+    this->txBuffSize = size;

     int result = initialiseTx();