stdio_uart: add stdio_uart_onlcr option to convert LF -> CRLF on output #15619

jimparis · 2020-12-11T19:12:18Z

This adds an option, enabled by USE_MODULE += "stdio_uart_onlcr", to automatically convert LF to CRLF on stdio UART output. (\n to \r\n). This is named after the "onlcr" stty flag, which does the same thing on a Unix system.

It's an easy global fix for something which has come up a lot before, e.g.:
Wiki
PR #4899
Issue #3040
Issue #12540

This approach of converting on output is used by many other embedded OSes and lets RIOT output work without needing specific configuration on the other end's terminal software, or any changes to strings throughout the OS and application.

Add USE_MODULE += "stdio_uart_onlcr" to enable it. This is named after the "onlcr" stty flag, which does the same thing.

jimparis · 2020-12-17T04:33:41Z

Updated to fix whitespace

stale · 2022-03-02T13:18:29Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

jimparis · 2022-03-02T18:42:42Z

Any feedback?

jmkim · 2022-06-30T07:01:37Z

Any updates?

maribu · 2022-08-25T08:52:57Z

Sorry for having to wait so long for getting a review.

Given that the effort to enable the conversion of "\n" to "\r\n" on the RIOT side (adding one line to the Makefile) is typically the same as configuring the receiver side to use "\n" for line endings, I do not really see the benefit of it.

To me, the trade-off looks like this:

👍 no need to add a line to the serial program's config used. (Note that if you open the serial via make term, RIOT will provide the correct symbol rate, the correct line ending encoding, and magic signals via DTR / RTS as needed. If your favorite program is not supported, it certainly could be added.)
👎 a module has to be selected in addition (IMO the same work as it safes)
👎 it increases the size of the .text section by a few bytes
👎 it adds a few CPU cycles and one byte per line of overhead

If you insist that this will be useful to you, I can review it. But to me the alternative of configuring the receiver side to except "\n" as line endings just looks more appealing.

maribu

I have comment inline regarding the implementation.

If you say that this indeed is a better solution than receiver side conversion for your use case, we can push this forward.

maribu · 2022-09-14T07:02:32Z

sys/stdio_uart/stdio_uart.c

+  #ifdef MODULE_STDIO_UART_ONLCR
+    const uint8_t *buf = (const uint8_t *)buffer;
+    const uint8_t cr = '\r';
+    size_t rem = len;
+    while (rem--) {
+            if (*buf == '\n')
+                    uart_write(STDIO_UART_DEV, &cr, 1);
+            uart_write(STDIO_UART_DEV, buf, 1);
+            buf++;
+    }
+  #else
    uart_write(STDIO_UART_DEV, (const uint8_t *)buffer, len);
+  #endif
 #endif
    return len;


Suggested change

#ifdef MODULE_STDIO_UART_ONLCR

const uint8_t *buf = (const uint8_t *)buffer;

const uint8_t cr = '\r';

size_t rem = len;

while (rem--) {

if (*buf == '\n')

uart_write(STDIO_UART_DEV, &cr, 1);

uart_write(STDIO_UART_DEV, buf, 1);

buf++;

}

#else

uart_write(STDIO_UART_DEV, (const uint8_t *)buffer, len);

#endif

#endif

return len;

ssize_t result = len;

if (IS_USED(MODULE_STDIO_UART_CRLF)) {

static const uint8_t crlf[2] = { (uint8_t)'\r', (uint8_t)'\n' };

while (len) {

size_t chunk_len = 0;

const uint8_t *buf = buffer;

while ((chunk_len < len) && (buf[chunk_len] != (uint8_t)'\n')) {

chunk_len++;

}

uart_write(STDIO_UART_DEV, buf, chunk_len);

buf += chunk_len;

len -= chunk_len;

if (chunk_len) {

uart_write(STDIO_UART_DEV, crlf, sizeof(crlf));

buf++;

len--;

}

}

}

else {

uart_write(STDIO_UART_DEV, (const uint8_t *)buffer, len);

}

return result;

Three things:

The current implementation that writes one char at a time. However, some UART drivers do support DMA. In that case this would keep the CPU busy all the time, while without CRLF conversion the CPU would only need to setup the DMA transfer to the UART peripheral (and like attend to an IRQ once it is done). The suggested change will be more efficient, as it will result in two DMA transfers per line rather than in one DMA transfer per character

There is no need for preprocessor conditionals here, you can just use a C level if. That way, the compile tests will check syntactical correctness for both branches. And with any optimization level better than -O0 the compiler will eliminate the dead branch, so that there is no overhead to pay

I am personally not super happy with name stdio_uart_onlcr, as it is not immediately clear to me what onlcr stands for. How about something like stdio_uart_crlf instead?

mcr · 2022-09-29T07:21:17Z

Hacking the terminal emulator to do the wrong thing is ... just that... the wrong thing.
This means that RIOT-OS can not be used easily with common systems, plugged into Windows systems, or actual terminals.
. o O ( telnet towel.blinkenlights.nl )

If you are worried about efficiency of looking for \n and sending \r\n, then maybe serial communications is not for you.

maribu · 2022-09-29T11:41:23Z

Hacking the terminal emulator to do the wrong thing is ... just that... the wrong thing.

This is not technically correct. UART is a different thing than the old terminal interfaces. We have particulate matter sensors, MP3 players, GPS receivers etc. interfacing via UART. These devices use a binary request-response protocol using UART as physical layer.

There is no such thing "plug and play" for terminals with an UART interface. In addition to symbol rate, parity, and stop bits one needs to agree on that the interface is actually used as serial console. Yes, you can use UART for a serial console, but this doesn't make UART a "terminal interface". One can also use USB (e.g. via CDC ACM), SPI, I2C, or any network interface as a transport for a serial console as well.

telnet towel.blinkenlights.nl

With e.g. telnet it is actually specified that the line endings are \r\n. RIOT can provide access to the shell via telnet btw. and it does follow the standard and consequently translates \n to \r\n there.

But in any case it doesn't IMO matter to engage in a theoretical discussion here. It seems there are people that do have a use case for \n --> \r\n conversion on the device side. As OS developers we IMO should strive to enable users to address use case. So let's get this in.

(Still, rebasing and addressing the review comments is still needed to merge this.)

mcr · 2022-09-29T12:30:02Z

I haven't looked at the details of this pull request yet, but I don't want to put the fix in the UART code for all the reasons above.
It needs to go into the puts() or equivalent lower-level function. I hope to fix my ignorance of this code today.

mcr · 2022-10-06T16:39:04Z

Any feedback?

I have argued that your fix is right, and I've rebased it against master.

kaspar030 · 2022-10-07T08:22:17Z

@jimparis would you like to continue with this PR? there's consensus now that we want this. Or should we go with @mcr's rebased version?

jimparis · 2022-10-08T07:14:19Z

I'm not able to work on this currently, and so I'm happy to go with @mcr 's version (thanks for picking this up!)

maribu · 2022-10-12T20:52:57Z

OK, let's get this in for the next release. With the feature freeze kicking in at the end of today, I picked this up and addressed the remaining issues: #18731

stdio_uart: add stdio_uart_onlcr option to convert LF -> CRLF on output

9164072

Add USE_MODULE += "stdio_uart_onlcr" to enable it. This is named after the "onlcr" stty flag, which does the same thing.

jimparis force-pushed the master branch from 61540ef to 9164072 Compare December 16, 2020 20:05

jeandudey added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: sys Area: System labels Mar 20, 2021

MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021

MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022

jmkim mentioned this pull request Jun 30, 2022

Implement the c_oflag: POSIX terminal interface output modes #18286

Closed

maribu reviewed Sep 14, 2022

View reviewed changes

mcr mentioned this pull request Oct 6, 2022

stdio_uart: add stdio_uart_onlcr option to convert LF -> CRLF on output #18703

Closed

maribu mentioned this pull request Oct 12, 2022

sys/stdio_uart: add stdio_uart_onlcr (pseudo-) module #18731

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stdio_uart: add stdio_uart_onlcr option to convert LF -> CRLF on output #15619

stdio_uart: add stdio_uart_onlcr option to convert LF -> CRLF on output #15619

jimparis commented Dec 11, 2020 •

edited

Loading

jimparis commented Dec 17, 2020

stale bot commented Mar 2, 2022

jimparis commented Mar 2, 2022

jmkim commented Jun 30, 2022

maribu commented Aug 25, 2022

maribu left a comment

maribu Sep 14, 2022

mcr commented Sep 29, 2022

maribu commented Sep 29, 2022

mcr commented Sep 29, 2022

mcr commented Oct 6, 2022

kaspar030 commented Oct 7, 2022

jimparis commented Oct 8, 2022 •

edited

Loading

maribu commented Oct 12, 2022

-  #ifdef MODULE_STDIO_UART_ONLCR
-    const uint8_t *buf = (const uint8_t *)buffer;
-    const uint8_t cr = '\r';
-    size_t rem = len;
-    while (rem--) {
-            if (*buf == '\n')
-                    uart_write(STDIO_UART_DEV, &cr, 1);
-            uart_write(STDIO_UART_DEV, buf, 1);
-            buf++;
-    }
-  #else
-    uart_write(STDIO_UART_DEV, (const uint8_t *)buffer, len);
-  #endif
-#endif
-    return len;
+    ssize_t result = len;
+    if (IS_USED(MODULE_STDIO_UART_CRLF)) {
+        static const uint8_t crlf[2] = { (uint8_t)'\r', (uint8_t)'\n' };
+        while (len) {
+            size_t chunk_len = 0;
+            const uint8_t *buf = buffer;
+            while ((chunk_len < len) && (buf[chunk_len] != (uint8_t)'\n')) {
+                chunk_len++;
+            }
+            uart_write(STDIO_UART_DEV, buf, chunk_len);
+            buf += chunk_len;
+            len -= chunk_len;
+            if (chunk_len) {
+                uart_write(STDIO_UART_DEV, crlf, sizeof(crlf));
+                buf++;
+                len--;
+            }
+        }
+    }
+    else {
+        uart_write(STDIO_UART_DEV, (const uint8_t *)buffer, len);
+    }
+    return result;

stdio_uart: add stdio_uart_onlcr option to convert LF -> CRLF on output #15619

Are you sure you want to change the base?

stdio_uart: add stdio_uart_onlcr option to convert LF -> CRLF on output #15619

Conversation

jimparis commented Dec 11, 2020 • edited Loading

jimparis commented Dec 17, 2020

stale bot commented Mar 2, 2022

jimparis commented Mar 2, 2022

jmkim commented Jun 30, 2022

maribu commented Aug 25, 2022

maribu left a comment

Choose a reason for hiding this comment

maribu Sep 14, 2022

Choose a reason for hiding this comment

mcr commented Sep 29, 2022

maribu commented Sep 29, 2022

mcr commented Sep 29, 2022

mcr commented Oct 6, 2022

kaspar030 commented Oct 7, 2022

jimparis commented Oct 8, 2022 • edited Loading

maribu commented Oct 12, 2022

jimparis commented Dec 11, 2020 •

edited

Loading

jimparis commented Oct 8, 2022 •

edited

Loading