Addressed review comments

Author: evedon

docs/design-documents/drivers/serial/diagrams/retarget.png

@@ -1,2 +1,2 @@
 <?xml version="1.0" encoding="UTF-8"?>
-<mxfile host="www.draw.io" modified="2019-10-04T08:52:28.454Z" agent="Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/13.0.1 Safari/605.1.15" etag="Am0JPjUp-a9ka-FfggNn" version="12.0.2" type="onedrive" pages="1"><diagram id="VeAL7sxWgT67HavtY4N0" name="Page-1">zVbbjpswEP0aHluBgXTzuLl187DaVKnU3b45eAKuHEyNE0i/vnZs7tmrVkqkiIyPZ8bjMxdw/Omu/C5wltxzAsxBLikdf+YgNPaRemrgaIAQuQaIBSUG8hpgTf+BBSu1PSWQdxQl50zSrAtGPE0hkh0MC8GLrtqWs+6pGY5hAKwjzIboL0pkYtAb9K3B74DGSXWyNxqbnR2ulO1N8gQTXrQgf+74U8G5NNKunALT3FW8GLvFM7t1YAJS+RaD4OffafGHbuLlYfLjsA1/j8T8i/VywGxvL+ygEVP+JhslxFqY6tgloVz9M7oRWBwrHXVYrVbbiQrZ8gxSR0e10MwLwKRZRYznYImRx4ptwfcpAR2wpxwUCZWwznCkdwtVXgpL5I7ZbRs6CAnls5x4NdOqQoHvQOrwXWuAbmxybHV6VbKKJtfeyGJJK8+hxbAtr7h23WRACTYJ70gIGiTkfgOat4e1Pm6TS4FVmSNXuZ+cfu4gYwvK4A6nRLF6NlHu7Wp5ddT77qWpD870Qo8k1cOZFrcMyls9XRQXkBIrziKG85xGXa6gpPJRye7X0K6eWjszTYdbLY7VIlUXahnp5VN7rzE7rSo7EzCQwUzrZUVdiu9FBK9PBolFDPIFvfB8lltZDF9IogCGJT10wz2XWXvCitNUtopo3C0iFPSKw1zTWrWHY8+RH/QGwbjnyPAwcHQqtPraH6+98PU5vHpYLx9N479vDnfGcGcKF0J1+dVNgvrdf7FJMLqeSfCJHR2+saPRJTs68HuvhY92dNB/v6DP6mi1bL7ajHrz6evP/wM=</diagram></mxfile>
+<mxfile host="www.draw.io" modified="2019-10-17T16:20:51.525Z" agent="Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/13.0.1 Safari/605.1.15" etag="r95EQ6_d1ASIV7ZjWYtd" version="12.1.3" type="onedrive" pages="1"><diagram id="VeAL7sxWgT67HavtY4N0" name="Page-1">zVbbjpswEP0apPahFYGQTR43t908rDZVKnV33wxMwJXB1DiB9OtrBxswJHuJKm0kJOzjmbF9zsyA5c6S8o6hLH6gIRDLscPScueW4wwGE0+8JHJQiH0zrpCI4VBhDbDBf0EbKnSHQ8gNQ04p4TgzwYCmKQTcwBBjtDDNtpSYu2Yogh6wCRDpo79wyOMKHTs3DX4POIr1zoPRpFpJkDZWN8ljFNKiBbkLy50xSnk1SsoZEMme5qXyW55ZrQ/GIOXvcRj+/DMrfmM/Wu2nP/Zb72XEFt9UlD0iO3VhyxkREW/qi0EkBzN5dh5iKt4E+wyxg7YRm9VmtR/TyJZmkFryVEvJPAMUNrOA0BwUMfyg2WZ0l4YgDzwQAYoYc9hkKJCrhUgwgcU8IWq5T4C+DTAOZQtShNwBTYDL49tq1RkqcXR6arGKRuvaJm7p7CkMqfSK6tCNAmKgRPiAIE5PkAcfJG+PG7mdn3OGRJo7tgg/PT52n/mehktM4B6loeD5pHT27XplOSLkCCWS5dTPs3NxNoecQ3ImBQz3LyoBRPCvVye1a3+21MMTtdchSfSMTA63BMpb2c0EF5CGajgPCMpzHJhcQYn5kxjb3z01e26tzCUdtp4c9CQVF2o5yelze61xO86031ldcrpjAbzdeDhiEfBX7NT3A0KjTfdVbqnovSIiA4I43pvN/ZSyaoc1xSlvJdHETKI6YXSI6t7Kq92MO4HcbuOZdAJVxPQCHROtvvbluee93ffXj5vVU9VoPtb3jbZvdP2CiSq/uk5Q/2t8WicYXU8nuLyivXdWtHNVFT10O5+FSyt6OO4Ecv5XRYtp85dYmTc/2+7iHw==</diagram></mxfile>

docs/design-documents/drivers/serial/diagrams/retarget.xml

@@ -30,7 +30,7 @@
 
 ### Overview and background
 
-Mbed OS contains multiple serial classes and multiple ways of printing information to the console. Serial classes have evolved over the years to add new functinality to existing classes and compensate for limitations. The result is that there are now multiple variants, not always well documented; therefore it is not clear which class shoud be used for what purpose. Futhermore, some of the classes pull substantial dependencies whose benefits are not very clear, for example standard library like objects and abstractions, and parts of the retarget layer.
+Mbed OS contains multiple serial classes and multiple ways of printing information to the console. Serial classes have evolved over the years to add new functionality to existing classes and compensate for limitations. The result is that there are now multiple variants, not always well documented; therefore it is not clear which class should be used for what purpose. Furthermore, some of the classes pull substantial dependencies whose benefits are not very clear, for example standard library like objects and abstractions, and parts of the retarget layer.
 
 This document collects proposals made by various contributors (see [references](#references)) and presents a simplified serial class hierarchy. The aim is to offer two public classes, one for unbuffered I/O and the other for buffered I/O, with a clear definition of which one should be used when. All other serial classes are deprecated and should be removed at a later stage. In addition, the serial classes dependencies will be optimised to only include components that provide valuable functionality.
 
@@ -45,16 +45,17 @@ Its main limitations are:
 - It wastes a lot of CPU time spinning waiting for serial output and it cannot be used reliably for input because it needs interrupt-speed response on read calls but its high-level API cannot be used from interrupts.
 - It lacks buffering so it cannot be used reliably for input and output without flow control.
 - It pulls in the C library stdio system because it uses `Stream`.
+- Although it is a `FileHandle`, its implementation is blocking only and has incorrect read semantics.
 
 **UARTSerial**
 
 `UARTSerial` provides buffered I/O. It can be used reliably for input from non-interrupt context and to avoid excess spinning waiting for transmission buffer space. It also does better blocking and correct POSIX-like `FileHandle` semantics. 
 
-It does not use `Stream` so it has the advantage of not pulling in the C stdio library. The drawback is that it loses the ability to call printf directly.
+It does not use `Stream` so it has the advantage of not pulling in the C stdio library. The drawback is that it doesn't include built-in printf methods, see [Usage scenarios and examples](#usage-scenarios-and-examples) for an example on how to print to a serial port.
 
 **RawSerial**
 
-`RawSerial` provides unbuffered I/O. It has no locks so it is safe to use in interrupts provided that it is the only instance that is using that serial port.
+`RawSerial` provides unbuffered I/O. It has no locks so it is safe to use in interrupts provided that it is the only instance that is using that serial port. Unlike `Serial` and `UARTSerial`, it is not a `FileHandle`.
 
 ### Requirements and assumptions
 
@@ -84,23 +85,50 @@ The below diagram shows the detailed inheritance hierarchy for `BufferedSerial`.
 
 ![BufferedSerial class](./diagrams/buffered-serial.png)
 
+```C
+class BufferedSerial : private SerialBase, public FileHandle, private NonCopyable<BufferedSerial> 
+```
+
+`BufferedSerial` privately inherits from `SerialBase`. It means that applications are forced to use the transmit and receive buffers since they cannot access `SerialBase` member functions directly.
+
 **API description**
 
-This is the new constructor for `BufferedSerial` to pass in the serial buffers size.
+These are the new constructors for `BufferedSerial`. The first constructor creates the buffers dynamically.
 
 ```C
     /** Create a BufferedSerial port, connected to the specified transmit and receive pins, with a particular baud rate.
      *  @param tx Transmit pin
      *  @param rx Receive pin
      *  @param baud The baud rate of the serial port (optional, defaults to MBED_CONF_PLATFORM_DEFAULT_SERIAL_BAUD_RATE)
-     *  @param rx_buffer_size the size of the receive buffer (optional, defaults to MBED_CONF_DRIVERS_UART_SERIAL_RXBUF_SIZE)
-     *  @param tx_buffer_size the size of the transmit buffer (optional, defaults to MBED_CONF_DRIVERS_UART_SERIAL_TXBUF_SIZE)
+     *  @param rx_buffer_size The size of the receive buffer (optional, defaults to MBED_CONF_DRIVERS_UART_SERIAL_RXBUF_SIZE)
+     *  @param tx_buffer_size The size of the transmit buffer (optional, defaults to MBED_CONF_DRIVERS_UART_SERIAL_TXBUF_SIZE)
      */
     BufferedSerial(PinName tx, 
                    PinName rx, 
                    int baud = MBED_CONF_PLATFORM_DEFAULT_SERIAL_BAUD_RATE, 
-                   uint32_t rx_buffer_size = MBED_CONF_DRIVERS_UART_SERIAL_RXBUF_SIZE,
-                   uint32_t tx_buffer_size = MBED_CONF_DRIVERS_UART_SERIAL_TXBUF_SIZE);
+                   size_t rx_buffer_size = MBED_CONF_DRIVERS_UART_SERIAL_RXBUF_SIZE,
+                   size_t tx_buffer_size = MBED_CONF_DRIVERS_UART_SERIAL_TXBUF_SIZE);
+```
+
+The second constructor takes buffer pointers as arguments.
+
+```C
+    /** Create a BufferedSerial port, connected to the specified transmit and receive pins, with a particular baud rate.
+    *  @param tx Transmit pin
+    *  @param rx Receive pin
+    *  @param baud The baud rate of the serial port (optional, defaults to MBED_CONF_PLATFORM_DEFAULT_SERIAL_BAUD_RATE)
+    *  @param rx_buffer The receive buffer 
+    *  @param rx_buffer_size The size of the receive buffer 
+    *  @param tx_buffer The transmit buffer 
+    *  @param tx_buffer_size The size of the transmit buffer 
+    */
+    BufferedSerial(PinName tx, 
+                   PinName rx, 
+                   int baud = MBED_CONF_PLATFORM_DEFAULT_SERIAL_BAUD_RATE, 
+                   void *rx_buffer,
+                   size_t rx_buffer_size,
+                   void *tx_buffer,
+                   size_t tx_buffer_size);
 ```
 
  The rest of the class API is that of `UARTSerial` and is described [here](https://os.mbed.com/docs/mbed-os/v5.14/mbed-os-api-doxy/classmbed_1_1_u_a_r_t_serial.html).
@@ -133,28 +161,6 @@ public:
      */
     UnbufferedSerial(PinName tx, PinName rx, int baud = MBED_CONF_PLATFORM_DEFAULT_SERIAL_BAUD_RATE);
 
-    /** Write a char to the serial port
-     *
-     * @param c The char to write
-     *
-     * @returns The written char or -1 if an error occurred
-     */
-    int putc(int c);
-
-    /** Read a char from the serial port
-     *
-     * @returns The char read from the serial port
-     */
-    int getc();
-
-    /** Write a string to the serial port
-     *
-     * @param str The string to write
-     *
-     * @returns 0 if the write succeeds, EOF for error
-     */
-    int puts(const char *str);
-
     virtual ssize_t write(const void *buffer, size_t size);
     virtual ssize_t read(void *buffer, size_t size);
     virtual off_t seek(off_t offset, int whence = SEEK_SET);
@@ -179,7 +185,7 @@ protected:
 
 ###  Detailed design : MinimalSerial
 
-The `MinimalSerial` class is a minimal abstract base class that provides basics I/O functionality. All other serial classes derive from it.
+The `MinimalSerial` class is a minimal abstract base class that provides basics I/O functionality. It is not part of the public interface. All other serial classes derive from it.
 
 **API description**
 
@@ -217,7 +223,19 @@ The constructor provides arguments to create a serial port connected to the spec
 
 ### Scenario 1 `Printing to stdout`
 
-The below example shows pseudocode for how to print `Hello` to stdout using the new API. Printf cannot be called directly; instead the application first calls _fdopen_ to get a _FILE*_ to use the printf function.
+There is no change in the current APi in regard to printing to stdout.  Applications should call `printf` directly as shown below. The console baud rate can be configured via `platform.stdio-baud-rate` in `mbed_app.json`.
+
+```C
+printf("Hello");
+```
+or
+```C
+fprintf(stdout, "Hello");
+```
+
+### Scenario 2 `Printing to a serial port other than stdout`
+
+The below example shows pseudocode for how to print `Hello` to `serial` port using the new API. `serial.printf` cannot be called directly; instead the application first calls _fdopen_ to get a _FILE*_ to use the printf function.
 
 ```C
 BufferedSerial serial(RX, TX, 115200);
@@ -243,7 +261,7 @@ The relationship of `FileHandle` and other APIs is as follows:
 
 ![Retarget](./diagrams/retarget.png)
 
-The POSIX layer and the static array of `FileHandle*` allocated by the retarget code for file handling consume a lot of memory. A constrained target that only uses a console does not need file handling and should be able to turn this functionality off.
+The POSIX layer, the memory cost associated with the vtable and virtual functions of the `FileHandle` class, and the static array of `FileHandle*` allocated by the retarget code for file handling has a significant impact on memory. A constrained target that only uses a console does not need file handling and should be able to turn this functionality off.
 
  A new configuration parameter is introduced to compile the file handling code out. The application can override the default configuration in mbed_app.json as follows:
 
@@ -290,7 +308,7 @@ Phase 1:
 - [Deprecation of APIs](#Deprecations)
 - Implementation of new class `UnbufferedSerial`
 - Implementation of new class `BufferedSerial`
-- Use `UnbufferedSerial` instead of `DirectSerial` in retarget code
+- Use `UnbufferedSerial` instead of `DirectSerial` in retarget code (as `DirectSerial` is not a public API it can be directly removed)
 
 Phase 2:
 - Removal of deprecated APIs