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

Hauke Petersen notifications at github.com
Tue Jan 29 14:08:23 CET 2019


haukepetersen requested changes on this pull request.

some small findings left, closing in...

> @@ -292,6 +329,19 @@ void uart_poweroff(uart_t uart)
 #endif
 }
 
+static uint8_t _data_mask(uart_t uart)

I am still strongly in favor of spending one byte RAM per UART device for pre-calculating this mask instead of having these ~10 instructions overhead in each ISR call!

> @@ -67,6 +76,59 @@ static int parse_dev(char *arg)
     return dev;
 }
 
+#ifdef MODULE_PERIPH_UART_MODECFG
+static uart_data_bits_t parse_data_bits(char *arg)
+{
+    int data_bits = atoi(arg) - 5;
+
+    if (data_bits >= 0 && data_bits < data_bits_lut_len)
+        return data_bits_lut[data_bits];

parenthesis missing

> +{
+    int data_bits = atoi(arg) - 5;
+
+    if (data_bits >= 0 && data_bits < data_bits_lut_len)
+        return data_bits_lut[data_bits];
+
+    printf("Error: Invalid number of data_bits (%i).\n", data_bits + 5);
+    return -1;
+}
+
+static uart_stop_bits_t parse_stop_bits(char *arg)
+{
+    int stop_bits = atoi(arg) - 1;
+
+    if (stop_bits >= 0 && stop_bits < stop_bits_lut_len)
+        return stop_bits_lut[stop_bits];

parents missing here as well

> +{
+    int dev;
+    uart_data_bits_t data_bits;
+    uart_parity_t  parity;
+    uart_stop_bits_t  stop_bits;
+
+    if (argc < 5) {
+        printf("usage: %s <dev> <data bits> <parity> <stop bits>\n", argv[0]);
+        return 1;
+    }
+
+    dev = parse_dev(argv[1]);
+    if (dev < 0) {
+        return 1;
+    }
+    data_bits = parse_data_bits(argv[2]);

this does potentially break: there is no guarantee what the actual value returned by this function might look like (it might be defined freely on a per-CPU basis). So the check in the line below might not work reliable for any platform...

I suggest simply to remove the static function and put the data bit parsing code here directly!

> +    uart_stop_bits_t  stop_bits;
+
+    if (argc < 5) {
+        printf("usage: %s <dev> <data bits> <parity> <stop bits>\n", argv[0]);
+        return 1;
+    }
+
+    dev = parse_dev(argv[1]);
+    if (dev < 0) {
+        return 1;
+    }
+    data_bits = parse_data_bits(argv[2]);
+    if (data_bits < 0) {
+        return 1;
+    }
+    parity = parse_parity(argv[3]);

same here

-- 
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-197538084
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190129/a8c018ea/attachment-0001.html>


More information about the notifications mailing list