[riot-notifications] [RIOT-OS/RIOT] RFC: UART enhanced settings (#10743)

MrKevinWeiss notifications at github.com
Thu Jan 24 12:17:42 CET 2019


MrKevinWeiss commented on this pull request.

Hmm here is a suggestion on how to fix the problem of needing to differentiate the bits in order to change the have the parity alter the databits.

> @@ -154,6 +154,43 @@ int uart_init(uart_t uart, uint32_t baudrate, uart_rx_cb_t rx_cb, void *arg)
     return UART_OK;
 }
 
+#ifdef MODULE_PERIPH_UART_MODECFG
+int uart_mode(uart_t uart, uart_data_bits_t data_bits, uart_parity_t parity, uart_stop_bits_t stop_bits)

can you break it down to two lines
```c
int uart_mode(uart_t uart, uart_data_bits_t data_bits, uart_parity_t parity,
              uart_stop_bits_t stop_bits)
```

> @@ -154,6 +154,43 @@ int uart_init(uart_t uart, uint32_t baudrate, uart_rx_cb_t rx_cb, void *arg)
     return UART_OK;
 }
 
+#ifdef MODULE_PERIPH_UART_MODECFG
+int uart_mode(uart_t uart, uart_data_bits_t data_bits, uart_parity_t parity, uart_stop_bits_t stop_bits)
+{
+    assert(uart < UART_NUMOF);
+
+    uint32_t data_bits_holder = data_bits;

I see now, if you make the data_bits type 32bits (as mentioned above) it should work.

> @@ -370,6 +370,52 @@ typedef enum {
     STM32_LPUART,           /**< STM32 Low-power UART (LPUART) module type */
 } uart_type_t;
 
+#ifdef MODULE_PERIPH_UART_MODECFG
+#define UART_INVALID_MODE 0xFF

If you just make it a bit this will force the uint32_t for the enum 
`#define UART_INVALID_MODE 0x8000000`
(please add a comment why)

> @@ -370,6 +370,52 @@ typedef enum {
     STM32_LPUART,           /**< STM32 Low-power UART (LPUART) module type */
 } uart_type_t;
 
+#ifdef MODULE_PERIPH_UART_MODECFG
+#define UART_INVALID_MODE 0xFF
+/**
+ * @brief   Override parity values
+ * @{
+ */
+#define HAVE_UART_PARITY_T
+typedef enum {
+   UART_PARITY_NONE = 0,                               /**< no parity */
+   UART_PARITY_EVEN = USART_CR1_PCE,                   /**< even parity */
+   UART_PARITY_ODD = (USART_CR1_PCE | USART_CR1_PS),   /**< odd parity */
+   UART_PARITY_MARK = UART_INVALID_MODE,               /**< not supported */

To be able to differentiate you should set the invalid bit and a unique number.
If you don't have this then when 6e1 and 7e1 appear the same (since the compare is always UART_INVALID_MODE)
```c
#define HAVE_UART_PARITY_T
typedef enum {
   UART_PARITY_NONE = 0,                               /**< no parity */
   UART_PARITY_EVEN = USART_CR1_PCE,                   /**< even parity */
   UART_PARITY_ODD = (USART_CR1_PCE | USART_CR1_PS),   /**< odd parity */
   UART_PARITY_MARK = UART_INVALID_MODE | 4,               /**< not supported */
   UART_PARITY_SPACE = UART_INVALID_MODE  | 5               /**< not supported */
} uart_parity_t;
/** @} */

/**
 * @brief   Override data bits length values
 * @{
 */
#define HAVE_UART_DATA_BITS_T
typedef enum {
   UART_DATA_BITS_8 = 0,                    /**< 8 data bits */
    UART_DATA_BITS_5 = UART_INVALID_MODE | 1,   /**< not supported */
    UART_DATA_BITS_6 = UART_INVALID_MODE | 2,   /**< not supported unless parity is set */
#if defined(USART_CR1_M1)
    UART_DATA_BITS_7 = USART_CR1_M1,        /**< 7 data bits */
#else
    UART_DATA_BITS_7 = UART_INVALID_MODE | 3,   /**< not supported unless parity is set */
#endif
    UART_DATA_BITS_9 = UART_INVALID_MODE | 4    /**< not supported */
} uart_data_bits_t;
/** @} */
```

> @@ -67,6 +67,87 @@ static int parse_dev(char *arg)
     return dev;
 }
 
+#ifdef MODULE_PERIPH_UART_MODECFG
+static int parse_data_bits(char *arg)
+{
+    unsigned data_bits = atoi(arg);
+    int res;

return type should be data_bits_t

> +            break;
+        case 8:
+            res = UART_DATA_BITS_8;
+            break;
+        default:
+            printf("Error: Invalid number of data_bits (%u).\n", data_bits);
+            res = -1;
+    }
+
+    return res;
+}
+
+static int parse_stop_bits(char *arg)
+{
+    unsigned stop_bits = atoi(arg);
+    int res;

return type should be stop_bits_t

> +            res = UART_STOP_BITS_1;
+            break;
+        case 2:
+            res = UART_STOP_BITS_2;
+            break;
+        default:
+            printf("Error: Invalid number of stop bits (%u).\n", stop_bits);
+            res = -1;
+    }
+
+    return res;
+}
+
+static int parse_parity(char *arg)
+{
+    int res;

I think you see where I am going with this.

> +            res = UART_DATA_BITS_7;
+            break;
+        case 8:
+            res = UART_DATA_BITS_8;
+            break;
+        default:
+            printf("Error: Invalid number of data_bits (%u).\n", data_bits);
+            res = -1;
+    }
+
+    return res;
+}
+
+static int parse_stop_bits(char *arg)
+{
+    unsigned stop_bits = atoi(arg);

please use int not unsigned

> +}
+
+static int parse_stop_bits(char *arg)
+{
+    unsigned stop_bits = atoi(arg);
+    int res;
+
+    switch (stop_bits) {
+        case 1:
+            res = UART_STOP_BITS_1;
+            break;
+        case 2:
+            res = UART_STOP_BITS_2;
+            break;
+        default:
+            printf("Error: Invalid number of stop bits (%u).\n", stop_bits);

switch this to int

> @@ -67,6 +67,87 @@ static int parse_dev(char *arg)
     return dev;
 }
 
+#ifdef MODULE_PERIPH_UART_MODECFG
+static int parse_data_bits(char *arg)
+{
+    unsigned data_bits = atoi(arg);

use int because that is what atoi does

> +
+    switch (data_bits) {
+        case 5:
+            res = UART_DATA_BITS_5;
+            break;
+        case 6:
+            res = UART_DATA_BITS_6;
+            break;
+        case 7:
+            res = UART_DATA_BITS_7;
+            break;
+        case 8:
+            res = UART_DATA_BITS_8;
+            break;
+        default:
+            printf("Error: Invalid number of data_bits (%u).\n", data_bits);

update to %i

> +
+    uint32_t data_bits_holder = data_bits;
+
+    if (parity) {
+        if (data_bits_holder == UART_DATA_BITS_8) {
+            data_bits_holder = USART_CR1_M;
+        }
+        else if (data_bits_holder == UART_DATA_BITS_7) {
+            data_bits_holder = UART_DATA_BITS_8;
+        }
+        else if (data_bits_holder == UART_DATA_BITS_6) {
+            data_bits_holder = UART_DATA_BITS_7;
+        }
+    }
+
+    if (data_bits_holder == UART_INVALID_MODE || parity == UART_INVALID_MODE) {

just mask for the invalid bit
`if ((data_bits & UART_INVALID_MODE) || (parity & UART_INVALID_MODE)) `
or even
`if ((data_bits | parity)  & UART_INVALID_MODE)) `

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/10743#pullrequestreview-195915853
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190124/f819c674/attachment-0001.html>


More information about the notifications mailing list