[Xenomai-core] new features for 16550A driver

[Xenomai-core] new features for 16550A driver

[Guillaume Gaudonville]

[-- Attachment #1: Type: text/plain, Size: 596 bytes --]

Hello,

I'm working on a project which aims at porting an application
running under VxWorks to Linux. This application uses the serial
port.

I would like to add an ioctl or a field in the config structure
to permit to strip or not the parity bit of each byte on reception. This
feature is implemented on Linux and VxWorks.

Is it something I can do and then submit to Xenomai or your driver is only
an example and will never be improved?

I think that I will have more feature to add to the driver, that's why I'm
asking this.

-- 
Guillaume Gaudonville
E-mail: guillaume.gaudonville@domain.hid

[-- Attachment #2: Type: text/html, Size: 800 bytes --]

Re: [Xenomai-core] new features for 16550A driver

[Jan Kiszka]

Guillaume Gaudonville wrote:
> Hello,
> 
> I'm working on a project which aims at porting an application
> running under VxWorks to Linux. This application uses the serial
> port.
> 
> I would like to add an ioctl or a field in the config structure
> to permit to strip or not the parity bit of each byte on reception. This
> feature is implemented on Linux and VxWorks.
> 
> Is it something I can do and then submit to Xenomai or your driver is only
> an example and will never be improved?

The driver is far from being just an example, it's used in real 
applications.

If your feature is useful and doesn't turn the existing code upside 
down, it will surely be considered for merging.

> 
> I think that I will have more feature to add to the driver, that's why I'm
> asking this.

I don't want to exclude anything (specifically as long as I haven't seen 
some patch yet), but I also don't want to overload the driver over the 
time with corner-case features. We need a good balance.

But we first of all need more details about what you plan to add, what 
application scenario is behind it, why you need the driver to implement 
the feature, and what impact it may have on the code. You are welcome to 
elaborate on this or, even better, post some patch drafts here.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] new features for 16550A driver

[Guillaume Gaudonville]

[-- Attachment #1: Type: text/plain, Size: 2526 bytes --]

2007/9/26, Jan Kiszka <jan.kiszka@domain.hid>:
>
> Guillaume Gaudonville wrote:
> > Hello,
> >
> > I'm working on a project which aims at porting an application
> > running under VxWorks to Linux. This application uses the serial
> > port.
> >
> > I would like to add an ioctl or a field in the config structure
> > to permit to strip or not the parity bit of each byte on reception. This
> > feature is implemented on Linux and VxWorks.
> >
> > Is it something I can do and then submit to Xenomai or your driver is
> only
> > an example and will never be improved?
>
> The driver is far from being just an example, it's used in real
> applications.


It was just to be sure... I'm using it for a real application.

If your feature is useful and doesn't turn the existing code upside
> down, it will surely be considered for merging.
>
> >
> > I think that I will have more feature to add to the driver, that's why
> I'm
> > asking this.
>
> I don't want to exclude anything (specifically as long as I haven't seen
> some patch yet), but I also don't want to overload the driver over the
> time with corner-case features. We need a good balance.
>
> But we first of all need more details about what you plan to add, what
> application scenario is behind it, why you need the driver to implement
> the feature, and what impact it may have on the code. You are welcome to
> elaborate on this or, even better, post some patch drafts here.


I don't  know  which  features  I'll need. For the ISTRIP feature the goal
is only to
remove the eigth'th bit of every byte received: I've got a VxWorks
application which communicate
with an external clock on the serial port. The clock uses the parity bit but
my application
do not. In the original code an ioctl was done to say the serial driver to
remove the parity bit on
reception. This feature is also offered under Linux and seems to be posix
(man tcsetattr, search ISTRIP).
In order to reduce the code modifications, I would like to implement it in
the driver.

If so, I think the better way would be to add a field in the rtser_config
struct, do you agree? And then depending
upon this field to apply a mask (0x7f) to the character in the interrupt
handler rt_16550_rx_interrupt() or only
to the characters passed to user space depending upon what is done in other
driver.
to reproduce the same behaviour).

Jan
>
> --
> Siemens AG, Corporate Technology, CT SE 2
> Corporate Competence Center Embedded Linux
>



-- 
Guillaume Gaudonville
E-mail: guillaume.gaudonville@domain.hid

[-- Attachment #2: Type: text/html, Size: 3359 bytes --]

Re: [Xenomai-core] new features for 16550A driver

[Guillaume Gaudonville]

[-- Attachment #1.1: Type: text/plain, Size: 2959 bytes --]

I've attached the patch drafts. It works fine for me. it prepare the work to
implement the other iflag values as describe in man tcsetattr.

2007/9/26, Guillaume Gaudonville <guillaume.gaudonville@domain.hid>:
>
>
>
> 2007/9/26, Jan Kiszka <jan.kiszka@domain.hid>:
> >
> > Guillaume Gaudonville wrote:
> > > Hello,
> > >
> > > I'm working on a project which aims at porting an application
> > > running under VxWorks to Linux. This application uses the serial
> > > port.
> > >
> > > I would like to add an ioctl or a field in the config structure
> > > to permit to strip or not the parity bit of each byte on reception.
> > This
> > > feature is implemented on Linux and VxWorks.
> > >
> > > Is it something I can do and then submit to Xenomai or your driver is
> > only
> > > an example and will never be improved?
> >
> > The driver is far from being just an example, it's used in real
> > applications.
>
>
> It was just to be sure... I'm using it for a real application.
>
> If your feature is useful and doesn't turn the existing code upside
> > down, it will surely be considered for merging.
> >
> > >
> > > I think that I will have more feature to add to the driver, that's why
> > I'm
> > > asking this.
> >
> > I don't want to exclude anything (specifically as long as I haven't seen
> > some patch yet), but I also don't want to overload the driver over the
> > time with corner-case features. We need a good balance.
> >
> > But we first of all need more details about what you plan to add, what
> > application scenario is behind it, why you need the driver to implement
> > the feature, and what impact it may have on the code. You are welcome to
> >
> > elaborate on this or, even better, post some patch drafts here.
>
>
> I don't  know  which  features  I'll need. For the ISTRIP feature the goal
> is only to
> remove the eigth'th bit of every byte received: I've got a VxWorks
> application which communicate
> with an external clock on the serial port. The clock uses the parity bit
> but my application
> do not. In the original code an ioctl was done to say the serial driver to
> remove the parity bit on
> reception. This feature is also offered under Linux and seems to be posix
> (man tcsetattr, search ISTRIP).
> In order to reduce the code modifications, I would like to implement it in
> the driver.
>
> If so, I think the better way would be to add a field in the rtser_config
> struct, do you agree? And then depending
> upon this field to apply a mask (0x7f) to the character in the interrupt
> handler rt_16550_rx_interrupt() or only
> to the characters passed to user space depending upon what is done in
> other driver.
> to reproduce the same behaviour).
>
> Jan
> >
> > --
> > Siemens AG, Corporate Technology, CT SE 2
> > Corporate Competence Center Embedded Linux
> >
>
>
>
> --
> Guillaume Gaudonville
> E-mail: guillaume.gaudonville@domain.hid
>



-- 
Guillaume Gaudonville
E-mail: guillaume.gaudonville@domain.hid

[-- Attachment #1.2: Type: text/html, Size: 4256 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 16550A_istrip_iflag.patch --]
[-- Type: text/x-diff; name="16550A_istrip_iflag.patch", Size: 2022 bytes --]

Index: ksrc/drivers/serial/16550A.c
===================================================================
--- ksrc/drivers/serial/16550A.c	(révision 2765)
+++ ksrc/drivers/serial/16550A.c	(copie de travail)
@@ -153,7 +153,10 @@ static inline int rt_16550_rx_interrupt(
 	do {
 		c = rt_16550_reg_in(mode, base, RHR);	/* read input char */
 
-		ctx->in_buf[ctx->in_tail] = c;
+		if (testbits(ctx->config.iflags, RTSER_IFLAG_ISTRIP))
+			ctx->in_buf[ctx->in_tail] = c & 0x7f;
+		else
+			ctx->in_buf[ctx->in_tail] = c;
 		if (ctx->in_history)
 			ctx->in_history[ctx->in_tail] = *timestamp;
 		ctx->in_tail = (ctx->in_tail + 1) & (IN_BUFFER_SIZE - 1);
@@ -426,6 +429,10 @@ static int rt_16550_set_config(struct rt
 		rtdm_lock_put_irqrestore(&ctx->lock, lock_ctx);
 	}
 
+	if (testbits(config->config_mask, RTSER_SET_IFLAGS)) {
+		ctx->config.iflags = config->iflags;
+	}
+
 	return err;
 }
 
Index: include/rtdm/rtserial.h
===================================================================
--- include/rtdm/rtserial.h	(révision 2765)
+++ include/rtdm/rtserial.h	(copie de travail)
@@ -184,6 +184,7 @@
 #define RTSER_SET_TIMEOUT_EVENT		0x0400
 #define RTSER_SET_TIMESTAMP_HISTORY	0x0800
 #define RTSER_SET_EVENT_MASK		0x1000
+#define RTSER_SET_IFLAGS		0x2000
 /** @} */
 
 
@@ -238,6 +239,15 @@
 #define RTSER_BREAK_SET			0x01
 
 
+/*!
+ * @anchor RTSER_IFLAG_xxx   @name RTSER_IFLAG_xxx
+ * Input behaviour (when used every bits will be set by the ioctl command)
+ * @{ */
+#define RTSER_IFLAG_NONE		0x00
+#define RTSER_IFLAG_ISTRIP		0x01
+#define RTSER_IFLAG_DEFAULT		0x00
+
+
 /**
  * Serial device configuration
  */
@@ -280,6 +290,11 @@ typedef struct rtser_config {
 	/** event mask to be used with @ref RTSER_RTIOC_WAIT_EVENT, see
 	 *  @ref RTSER_EVENT_xxx */
 	int		event_mask;
+
+	/** input flags, see @ref RTSER_IFLAG_xxx, RTSER_IFLAG_ISTRIP is
+	 *  the only one supported for the moment (when used every bits
+	 *  must be set according to the desired configuration)*/
+	int		iflags;
 } rtser_config_t;
 
 /**

Re: [Xenomai-core] new features for 16550A driver

[Jan Kiszka]

Guillaume Gaudonville wrote:
> I've attached the patch drafts. It works fine for me. it prepare the work to
> implement the other iflag values as describe in man tcsetattr.
> 

...

> 
> Index: ksrc/drivers/serial/16550A.c
> ===================================================================
> --- ksrc/drivers/serial/16550A.c	(révision 2765)
> +++ ksrc/drivers/serial/16550A.c	(copie de travail)
> @@ -153,7 +153,10 @@ static inline int rt_16550_rx_interrupt(
>  	do {
>  		c = rt_16550_reg_in(mode, base, RHR);	/* read input char */
>  
> -		ctx->in_buf[ctx->in_tail] = c;
> +		if (testbits(ctx->config.iflags, RTSER_IFLAG_ISTRIP))
> +			ctx->in_buf[ctx->in_tail] = c & 0x7f;
> +		else
> +			ctx->in_buf[ctx->in_tail] = c;

OK, things become clearer.

Question: Switching parity checking on in the hardware does not work/is 
no option for you? Shouldn't that clear the parity information as well?

>  		if (ctx->in_history)
>  			ctx->in_history[ctx->in_tail] = *timestamp;
>  		ctx->in_tail = (ctx->in_tail + 1) & (IN_BUFFER_SIZE - 1);
> @@ -426,6 +429,10 @@ static int rt_16550_set_config(struct rt
>  		rtdm_lock_put_irqrestore(&ctx->lock, lock_ctx);
>  	}
>  
> +	if (testbits(config->config_mask, RTSER_SET_IFLAGS)) {
> +		ctx->config.iflags = config->iflags;
> +	}
> +
>  	return err;
>  }
>  
> Index: include/rtdm/rtserial.h
> ===================================================================
> --- include/rtdm/rtserial.h	(révision 2765)
> +++ include/rtdm/rtserial.h	(copie de travail)
> @@ -184,6 +184,7 @@
>  #define RTSER_SET_TIMEOUT_EVENT		0x0400
>  #define RTSER_SET_TIMESTAMP_HISTORY	0x0800
>  #define RTSER_SET_EVENT_MASK		0x1000
> +#define RTSER_SET_IFLAGS		0x2000
>  /** @} */
>  
>  
> @@ -238,6 +239,15 @@
>  #define RTSER_BREAK_SET			0x01
>  
>  
> +/*!
> + * @anchor RTSER_IFLAG_xxx   @name RTSER_IFLAG_xxx
> + * Input behaviour (when used every bits will be set by the ioctl command)
> + * @{ */
> +#define RTSER_IFLAG_NONE		0x00
> +#define RTSER_IFLAG_ISTRIP		0x01
> +#define RTSER_IFLAG_DEFAULT		0x00
> +
> +
>  /**
>   * Serial device configuration
>   */
> @@ -280,6 +290,11 @@ typedef struct rtser_config {
>  	/** event mask to be used with @ref RTSER_RTIOC_WAIT_EVENT, see
>  	 *  @ref RTSER_EVENT_xxx */
>  	int		event_mask;
> +
> +	/** input flags, see @ref RTSER_IFLAG_xxx, RTSER_IFLAG_ISTRIP is
> +	 *  the only one supported for the moment (when used every bits
> +	 *  must be set according to the desired configuration)*/
> +	int		iflags;
>  } rtser_config_t;
>  
>  /**

The patch looks very clean, no question! Assuming my attempt above to 
use the hardware for this is nonsense, I then wonder if the feature is 
worth the additional conditional branch + another potential cache miss 
(for ctx->config.iflags) inside the critical reception path.

This is precisely that question of balance: Is the extension of some 
broader interest? Do we need it inside the driver (I see no reason yet 
why the application cannot do the masking, and we are not forced into 
compatibility with termios)? And does the extension impact critical paths?

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] new features for 16550A driver

[Guillaume Gaudonville]

[-- Attachment #1: Type: text/plain, Size: 6455 bytes --]

2007/9/26, Jan Kiszka <jan.kiszka@domain.hid>:
>
> Guillaume Gaudonville wrote:
> > I've attached the patch drafts. It works fine for me. it prepare the
> work to
> > implement the other iflag values as describe in man tcsetattr.
> >
>
> ...
>
> >
> > Index: ksrc/drivers/serial/16550A.c
> > ===================================================================
> > --- ksrc/drivers/serial/16550A.c      (révision 2765)
> > +++ ksrc/drivers/serial/16550A.c      (copie de travail)
> > @@ -153,7 +153,10 @@ static inline int rt_16550_rx_interrupt(
> >       do {
> >               c = rt_16550_reg_in(mode, base, RHR);   /* read input char
> */
> >
> > -             ctx->in_buf[ctx->in_tail] = c;
> > +             if (testbits(ctx->config.iflags, RTSER_IFLAG_ISTRIP))
> > +                     ctx->in_buf[ctx->in_tail] = c & 0x7f;
> > +             else
> > +                     ctx->in_buf[ctx->in_tail] = c;
>
> OK, things become clearer.
>
> Question: Switching parity checking on in the hardware does not work/is
> no option for you? Shouldn't that clear the parity information as well?

It's not an option for me. I'm not sure if that clear the parity information
but I don't want to have parity error checking. I think this option exist
to permit to manage a remote hardware which communicates over
the serial line with parity enabled when we don't mind of the parity
information and don't want to see parity error.


>               if (ctx->in_history)
> >                       ctx->in_history[ctx->in_tail] = *timestamp;
> >               ctx->in_tail = (ctx->in_tail + 1) & (IN_BUFFER_SIZE - 1);
> > @@ -426,6 +429,10 @@ static int rt_16550_set_config(struct rt
> >               rtdm_lock_put_irqrestore(&ctx->lock, lock_ctx);
> >       }
> >
> > +     if (testbits(config->config_mask, RTSER_SET_IFLAGS)) {
> > +             ctx->config.iflags = config->iflags;
> > +     }
> > +
> >       return err;
> >  }
> >
> > Index: include/rtdm/rtserial.h
> > ===================================================================
> > --- include/rtdm/rtserial.h   (révision 2765)
> > +++ include/rtdm/rtserial.h   (copie de travail)
> > @@ -184,6 +184,7 @@
> >  #define RTSER_SET_TIMEOUT_EVENT              0x0400
> >  #define RTSER_SET_TIMESTAMP_HISTORY  0x0800
> >  #define RTSER_SET_EVENT_MASK         0x1000
> > +#define RTSER_SET_IFLAGS             0x2000
> >  /** @} */
> >
> >
> > @@ -238,6 +239,15 @@
> >  #define RTSER_BREAK_SET                      0x01
> >
> >
> > +/*!
> > + * @anchor RTSER_IFLAG_xxx   @name RTSER_IFLAG_xxx
> > + * Input behaviour (when used every bits will be set by the ioctl
> command)
> > + * @{ */
> > +#define RTSER_IFLAG_NONE             0x00
> > +#define RTSER_IFLAG_ISTRIP           0x01
> > +#define RTSER_IFLAG_DEFAULT          0x00
> > +
> > +
> >  /**
> >   * Serial device configuration
> >   */
> > @@ -280,6 +290,11 @@ typedef struct rtser_config {
> >       /** event mask to be used with @ref RTSER_RTIOC_WAIT_EVENT, see
> >        *  @ref RTSER_EVENT_xxx */
> >       int             event_mask;
> > +
> > +     /** input flags, see @ref RTSER_IFLAG_xxx, RTSER_IFLAG_ISTRIP is
> > +      *  the only one supported for the moment (when used every bits
> > +      *  must be set according to the desired configuration)*/
> > +     int             iflags;
> >  } rtser_config_t;
> >
> >  /**
>
> The patch looks very clean, no question! Assuming my attempt above to
> use the hardware for this is nonsense, I then wonder if the feature is
> worth the additional conditional branch + another potential cache miss
> (for ctx->config.iflags) inside the critical reception path.


This is precisely that question of balance: Is the extension of some
> broader interest? Do we need it inside the driver (I see no reason yet
> why the application cannot do the masking, and we are not forced into
> compatibility with termios)? And does the extension impact critical paths?


I agree with you if the goal was just to manage the ISTRIP option. But we
could
then add some of the other input flag :

       * IGNBRK Ignore BREAK condition on input.
       * BRKINT If  IGNBRK  is set, a BREAK is ignored. If it is not set but
BRKINT is set, then a BREAK causes the input and output queues to be
flushed, and if the terminal is the controlling terminal of a foreground
process group, it will cause a SIGINT to be sent to this foreground process
group.  When neither IGNBRK nor BRKINT are set, a BREAK reads as a null byte
('\0'), except when  PARMRK is set, in which case it reads as the sequence
\377 \0 \0.
       * IGNPAR Ignore framing errors and parity errors.
       * PARMRK If  IGNPAR is not set, prefix a character with a parity
error or framing error with \377 \0.  If neither IGNPAR nor PARMRK is set,
read a character with a parity error or framing error as \0.
       * INPCK  Enable input parity checking.
       * ISTRIP Strip off eighth bit.
       * INLCR  Translate NL to CR on input.
       * IGNCR  Ignore carriage return on input.
       * ICRNL  Translate carriage return to newline on input (unless IGNCR
is set).

Maybe I could implement some of them. Number depending upon the time I have.

This flag could be very useful to Xenomai users. I know some of them are
implemented
both in termios and in the tyLib of VxWorks. So, I see two use case:

           * peoples who will have to port a Linux application or a VxWorks
application
              under Xenomai: those features would permit them to limits the
modifications
              in the original code (which I think is a big constraint of
this type of work).
           * peoples who will have to code an application which need those
features. This features
              seems to be implemented by a lot of OSes, I don't think this
is done for nothing. This
              limits the development work in User Space while not adding a
big coomplexity in the driver.

Just a precision: Under Linux this is not done in the low-level driver but
in the serial line discipline code. But
your driver manage already the communication with User Space which is more
than what Linux serial
drivers do.


Jan
>
> --
> Siemens AG, Corporate Technology, CT SE 2
> Corporate Competence Center Embedded Linux
>



-- 
Guillaume Gaudonville
E-mail: guillaume.gaudonville@domain.hid

[-- Attachment #2: Type: text/html, Size: 10297 bytes --]