Review request changes

Author: hugueskamba

TESTS/mbed_drivers/unbuffered_serial/main.cpp

@@ -41,25 +41,27 @@ using namespace utest::v1;
 #define MSG_KEY_ECHO_MESSAGE                                     "echo_message"
 #define MSG_VALUE_HELLO_WORLD                                   "Hello, world!"
 
-#define STRING_TO_SEND "{{" MSG_KEY_ECHO_MESSAGE ";" MSG_VALUE_HELLO_WORLD "}}\r\n"
 #define EXPECTED_ECHOED_STRING "{{" MSG_KEY_ECHO_MESSAGE ";" MSG_VALUE_HELLO_WORLD "}}"
+// The target is expected to transmit Greentea messages with \n (or \r\n) or they are not detected by the host
+#define STRING_TO_SEND EXPECTED_ECHOED_STRING "\n"
+
 
 static UnbufferedSerial unbuffered_serial_obj(
     USBTX, USBRX, MBED_CONF_PLATFORM_STDIO_BAUD_RATE
 );
 
-ssize_t greentea_unbuffered_serial_get_kv(void *buffer, ssize_t length)
+static ssize_t unbuffered_serial_read(void *buffer, ssize_t length)
 {
     if (length == 0) {
         return 0;
     }
 
-    // Ignore the character previously sent to the host that was not read
+    // Ignore the `\n` character previously sent to the host in the previous
+    // key-value pair that was not removed from the FIFO.
     unsigned char *buf = static_cast<unsigned char *>(buffer);
     unbuffered_serial_obj.read(buf, 1);
 
     // Get the message sent by the host
-    // unsigned char *buf = static_cast<unsigned char *>(buffer);
     for (ssize_t i = 0; i < length; i++) {
         TEST_ASSERT_EQUAL_UINT(1, unbuffered_serial_obj.read(buf+i, 1));
     }
@@ -68,6 +70,13 @@ ssize_t greentea_unbuffered_serial_get_kv(void *buffer, ssize_t length)
 }
 
 
+// Test that data sent using an UnbufferedSerial object is correctly sent.
+// The test case sends a Greentea key-value pair message from the target to the
+// host using an UnbufferedSerial object and expects the message
+// to be echoed back by the host. The host response is received via the Greentea
+// framework usual route using greentea_parse_kv(). Success is determined upon
+// reception of the echoed message which indicates that the message was received
+// by the host as it was sent by the target.
 static void test_serial_write()
 {
     char tx_msg[] = STRING_TO_SEND;
@@ -85,6 +94,13 @@ static void test_serial_write()
     TEST_ASSERT_EQUAL_STRING(MSG_VALUE_HELLO_WORLD, rx_value);
 }
 
+
+// Test that data received using an UnbufferedSerial object is correctly received.
+// The test case sends a Greentea key-value pair message from the target to the
+// host via the Greentea framework usual route using greentea_send_kv().
+// It expects the message to be echoed back to the target. An UnbufferedSerial
+// object is used to handle the received message. Succes is determined upon
+// reception of a key-value pair matching the key-value pair sent by the target.
 static void test_serial_read()
 {
     greentea_send_kv(MSG_KEY_ECHO_MESSAGE, MSG_VALUE_HELLO_WORLD);
@@ -93,11 +109,12 @@ static void test_serial_read()
     // Exclude the null terminator which is not read
     ssize_t expected_rx_msg_length = sizeof(EXPECTED_ECHOED_STRING)-1;
 
-    greentea_unbuffered_serial_get_kv(rx_msg, expected_rx_msg_length);
+    unbuffered_serial_read(rx_msg, expected_rx_msg_length);
 
     TEST_ASSERT_EQUAL_STRING(EXPECTED_ECHOED_STRING, rx_msg);
 }
 
+
 utest::v1::status_t greentea_setup(const size_t number_of_cases)
 {
     GREENTEA_SETUP(12, "serial_comms");

drivers/SerialBase.h

@@ -38,8 +38,8 @@ namespace mbed {
  * @{
  */
 
-/** An abstract base class for serial port implementations
- * Can't be instantiated in sub-classess
+/** A base class for serial port implementations
+ * Can't be instantiated directly (use UnbufferedSerial or UARTSerial)
  *
  * @note Synchronization level: Set by subclass
  */

drivers/UnbufferedSerial.h

@@ -21,11 +21,14 @@
 
 #if DEVICE_SERIAL || defined(DOXYGEN_ONLY)
 
+#include <cstdarg>
+
 #include "drivers/SerialBase.h"
 #include "platform/FileHandle.h"
 #include "platform/mbed_toolchain.h"
 #include "platform/NonCopyable.h"
-#include <cstdarg>
+#include "platform/PlatformMutex.h"
+
 
 namespace mbed {
 
@@ -64,36 +67,28 @@ class UnbufferedSerial:
 
     /** Write the contents of a buffer to a file
      *
-     *  Follows POSIX semantics:
-     *
-     * * if blocking, block until all data is written
-     * * if no data can be written, and non-blocking set, return -EAGAIN
-     * * if some data can be written, and non-blocking set, write partial
+     * Blocks until all data is written
      *
      *  @param buffer   The buffer to write from
-     *  @param length   The number of bytes to write
-     *  @return         The number of bytes written, negative error on failure
+     *  @param size     The number of bytes to write
+     *  @return         The number of bytes written
      */
     virtual ssize_t write(const void *buffer, size_t size);
 
     /** Read the contents of a file into a buffer
      *
-     *  Follows POSIX semantics:
-     *
-     *  * if no data is available, and non-blocking set return -EAGAIN
-     *  * if no data is available, and blocking set, wait until data is available
-     *  * If any data is available, call returns immediately
+     *  Blocks and reads exactly one character
      *
      *  @param buffer   The buffer to read in to
-     *  @param length   The number of bytes to read
-     *  @return         The number of bytes read, 0 at end of file, negative error on failure
+     *  @param size     The number of bytes to read
+     *  @return         The number of bytes read
      */
     virtual ssize_t read(void *buffer, size_t size);
 
     /** Move the file position to a given offset from from a given location
      *
-     * Not valid for a device type FileHandle like UARTSerial.
-     * In case of UARTSerial, returns ESPIPE
+     * Not valid for a device type FileHandle like UnbufferedSerial.
+     * In case of UnbufferedSerial, returns ESPIPE
      *
      *  @param offset   The offset from whence to move to
      *  @param whence   The start of where to seek
@@ -136,12 +131,17 @@ class UnbufferedSerial:
         return 0;
     }
 
-    /**
-     * Equivalent to POSIX poll(). Derived from FileHandle.
-     * Provides a mechanism to multiplex input/output over a set of file
-     * handles.
+
+    /** Check for poll event flags
+     * Check the events listed in events to see if data can be read or written
+     * without blocking.
+     * Call is nonblocking - returns state of events.
+     *
+     * @param events        bitmask of poll events we're interested in - POLLIN/POLLOUT etc.
+     *
+     * @returns             bitmask of poll events that have occurred.
      */
-    virtual short poll(short events) const;
+    virtual short poll(short events);
 
 #if !(DOXYGEN_ONLY)
 protected:
@@ -152,6 +152,17 @@ class UnbufferedSerial:
     /* Release exclusive access to this serial port
      */
     virtual void unlock(void);
+
+private:
+
+    /** Acquire mutex */
+    virtual void api_lock(void);
+
+    /** Release mutex */
+    virtual void api_unlock(void);
+
+
+    PlatformMutex _mutex;
 #endif // !(DOXYGEN_ONLY)
 };
 

drivers/source/UnbufferedSerial.cpp

@@ -15,13 +15,12 @@
  * limitations under the License.
  */
 
-#include "hal/serial_api.h"
-
 #include "drivers/UnbufferedSerial.h"
 
-
 #if DEVICE_SERIAL
 
+#include "platform/mbed_critical.h"
+
 namespace mbed {
 
 UnbufferedSerial::UnbufferedSerial(
@@ -36,9 +35,29 @@ UnbufferedSerial::UnbufferedSerial(
 ssize_t UnbufferedSerial::write(const void *buffer, size_t size)
 {
     const unsigned char *buf = static_cast<const unsigned char *>(buffer);
+
+    if (size == 0) {
+        return 0;
+    }
+
+    bool lock_api = false;
+
+    if (!core_util_in_critical_section()) {
+        lock_api = true;
+    }
+
+    if (lock_api) {
+        api_lock();
+    }
+
     for (size_t i = 0; i < size; i++) {
-        serial_putc(&_serial, buf[i]);
+        _base_putc(buf[i]);
+    }
+
+    if (lock_api) {
+        api_unlock();
     }
+
     return size;
 }
 
@@ -48,17 +67,17 @@ ssize_t UnbufferedSerial::read(void *buffer, size_t size)
     if (size == 0) {
         return 0;
     }
-    buf[0] = serial_getc(&_serial);
+    buf[0] = _base_getc();
     return 1;
 }
 
-short UnbufferedSerial::poll(short events) const
+short UnbufferedSerial::poll(short events)
 {
     short revents = 0;
-    if ((events & POLLIN) && serial_readable((serial_t*)&_serial)) {
+    if ((events & POLLIN) && SerialBase::readable()) {
         revents |= POLLIN;
     }
-    if ((events & POLLOUT) && serial_writable((serial_t*)&_serial)) {
+    if ((events & POLLOUT) && SerialBase::writeable()) {
         revents |= POLLOUT;
     }
     return revents;
@@ -78,6 +97,16 @@ void UnbufferedSerial::unlock()
     // No lock used - external synchronization required
 }
 
+void UnbufferedSerial::api_lock(void)
+{
+    _mutex.lock();
+}
+
+void UnbufferedSerial::api_unlock(void)
+{
+    _mutex.unlock();
+}
+
 } // namespace mbed
 
 #endif // #if DEVICE_SERIAL

features/unsupported/tests/mbed/ticker/main.cpp

@@ -1,7 +1,7 @@
 #include "mbed.h"
 #include "test_env.h"
 
-UnbufferedSerial pc(USBTX, USBRX);
+RawSerial pc(USBTX, USBRX);
 
 Ticker flipper_1;
 DigitalOut led1(LED1);