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.
