Hi,
This is a good idea to better follow posix.
This is typically the kind of stuff that would have deserved a message
on the dev list to say:
Hey friends we are improving terminals, expect bugs because it's hard to
get right, oh btw apps need to be updated too, otherwise strange things
may happen like double echos.
This is not doable in a pull request named "termios improvement" or
something similar, lost in the middle of tens of other requests. It
conveys no meaning and no information that it is a profound change with
wide consequences. It is easy to miss, hard and long to find, but it
would take you seconds to drop a message on the list.
This would help A LOT to have your work understood by the community. It
would also have avoided several github issues. We could contribute by
doing more useful testing of corner cases and come up with more testing
ideas.
Like, proper community behaviour on our nice common project? Isnt that a
normal thing to do?
Sebastien
On 3/8/23 20:26, Xiang Xiao wrote:
On Thu, Mar 9, 2023 at 3:07 AM Gregory Nutt <spudan...@gmail.com> wrote:
I imagine that this is occurring because readline also echos the
input:
https://github.com/apache/nuttx-apps/blob/master/system/readline/readline_common.c#L644
The low-level serial driver should not echo just because /isconsole
/is
true. Console echo is always handled by the application. I would say
that that is a bug.
From the spec:
https://pubs.opengroup.org/onlinepubs/7908799/xsh/termios.h.html
the serial driver should echo the char if ECHO is enabled. Since NuttX
doesn't follow the spec, it makes many POSIX compliant interactive
shells
doesn't work very well.
We have to make the breaking change to fix the wrong initial
implementation.
I still seems like the isconsole does not belong in the conditional:
https://github.com/apache/nuttx/commit/68384e9db42e254b2cf6720bc3380aebdd2b298c
if (dev->isconsole
#ifdef CONFIG_SERIAL_TERMIOS
|| (dev->tc_iflag & ECHO)
#endif
)
{
Currently:
893 if (
894 #ifdef CONFIG_SERIAL_TERMIOS
895 dev->tc_iflag & ECHO
896 #else
897 dev->isconsole
898 #endif
899 )
900 {
If CONFIG_SERIAL_TERMIOS is not set then shouldn't the entire 'if '
condition should be removed?
From my understanding, when CONFIG_SERIAL_TERMIOS isn't set whether
serial
driver do some special terminal handling(e.g. \n<->\r\n and echo) is
controlled by isconsole flag:
1. isconsole equals false, all special handling is disable
2. isconsole equals true, all special handling is enable
So, the check needs to be kept to ensure that non-console uart doesn't
add
the unxpected process.
If CONFIG_SERIAL_TERMIOS is not selected then non-consoles will do
nothing. In order to restore legacy behavior you would have to do this:
+#ifdef CONFIG_SERIAL_TERMIOS
if (
-#ifdef CONFIG_SERIAL_TERMIOS
dev->tc_iflag & ECHO
-#else - dev->isconsole -#endif
)
...
}
+#endif
That should eliminate the double character echo.
So the command line tool has to do the different thing(echo or non echo by
self) based on CONFIG_SERIAL_TERMIOS?
I suppose that you could also eliminate the echo in readline() (and
cle()???) but that would probably mess up the command line editing since
the in the edit commands would also be echoed by the serial driver.
Since the code to handle the special process is very small, is it better to
always allow application change ECHO and OPOST through TCSETS? So, the
special function or program can disable ECHO/OPOST programmatically.
By the way, the reason that this is implemented in a non-standard way like
this with a flag is historical. The serial driver is one of the older
parts of the system. There was no TERMIOS support in NuttX until much,
much later in the development. The serial driver was just a flat,
read/write character device like any other. readline() fakes all the the
necessary operations of raw mode and other termios settings.
Yes, but it's good time to follow the POSIX spec that moves the special
process from readline to serial driver and make TCSETS for ECHO and
OPOST always exist.
