[riot-notifications] [RIOT-OS/RIOT] cpu/cc26x0: implement uart_mode() (#11612)

MrKevinWeiss notifications at github.com
Tue Jun 4 09:49:08 CEST 2019


MrKevinWeiss commented on this pull request.

The implementation is good and tested.  The test has a few comments to address.

> +#include <stdlib.h>
+
+#include "periph/uart.h"
+#include "xtimer.h"
+
+/* UART_DEV is always 0 since this test is for device with 1 UART */
+#define _UART_DEV (UART_DEV(0))
+
+
+static void _print_status(int8_t status, uart_data_bits_t data_bits,
+                          uart_parity_t parity, uart_stop_bits_t stop_bits) {
+    char mode[4];
+
+    /* If status < 0, print a header indicating an error */
+    if (status < 0) {
+        printf("Unsupported Mode: ");

Could you make it not print anything if it is supported, it becomes pretty hard to read on the scope.

> +        UART_STOP_BITS_1,
+        UART_STOP_BITS_2
+    };
+
+    /* Test each permutation */
+    for (int didx = 0; didx < 4; ++didx) {
+        for (int pidx = 0; pidx < 5; ++pidx) {
+            for (int sidx = 0; sidx < 2; ++sidx) {
+                int8_t status = uart_mode(_UART_DEV, data_bits[didx],
+                                          parity[pidx], stop_bits[sidx]);
+
+                _print_status(status, data_bits[didx],
+                              parity[pidx], stop_bits[sidx]);
+
+                /* Short sleep to make results easier to parse */
+                xtimer_usleep(50000);

Maybe get rid of the delay if it is not supported so it cycles through only usable values.

> +    };
+
+    uart_parity_t parity[5] = {
+        UART_PARITY_NONE,
+        UART_PARITY_EVEN,
+        UART_PARITY_ODD,
+        UART_PARITY_MARK,
+        UART_PARITY_SPACE
+    };
+
+    uart_stop_bits_t stop_bits[2] = {
+        UART_STOP_BITS_1,
+        UART_STOP_BITS_2
+    };
+
+    /* Test each permutation */

If you could print the supported values in the beginning that would be nice before any changes to the mode.

> +        UART_PARITY_EVEN,
+        UART_PARITY_ODD,
+        UART_PARITY_MARK,
+        UART_PARITY_SPACE
+    };
+
+    uart_stop_bits_t stop_bits[2] = {
+        UART_STOP_BITS_1,
+        UART_STOP_BITS_2
+    };
+
+    /* Test each permutation */
+    for (int didx = 0; didx < 4; ++didx) {
+        for (int pidx = 0; pidx < 5; ++pidx) {
+            for (int sidx = 0; sidx < 2; ++sidx) {
+                int8_t status = uart_mode(_UART_DEV, data_bits[didx],

Though this implementation is not affected by this, in the api it says to always init before changing mode so you will have to call `uart_init(_UART_DEV, <BAUDRATE>, NULL, NULL);`

> +        UART_STOP_BITS_1,
+        UART_STOP_BITS_2
+    };
+
+    /* Test each permutation */
+    for (int didx = 0; didx < 4; ++didx) {
+        for (int pidx = 0; pidx < 5; ++pidx) {
+            for (int sidx = 0; sidx < 2; ++sidx) {
+                int8_t status = uart_mode(_UART_DEV, data_bits[didx],
+                                          parity[pidx], stop_bits[sidx]);
+
+                _print_status(status, data_bits[didx],
+                              parity[pidx], stop_bits[sidx]);
+
+                /* Short sleep to make results easier to parse */
+                xtimer_usleep(50000);

Also maybe have the number settable with an overwrittable define.

-- 
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/11612#pullrequestreview-245274310
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190604/9763ce88/attachment.html>


More information about the notifications mailing list