[PATCH] -FT230X: Readout, decode and encode the RS232 inverson configuration bits.

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] -FT230X: Readout, decode and encode the RS232 inverson configuration bits.

Uwe Bonnes

--
Uwe Bonnes                [hidden email]

Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
--------- Tel. 06151 162516 -------- Fax. 06151 164321 ----------
From e56d3d7b8f25ceb903c5339c76da62807377724f Mon Sep 17 00:00:00 2001
From: Uwe Bonnes <[hidden email]>
Date: Thu, 14 Aug 2014 14:34:38 +0200
Subject: FT230X: Readout, decode and encode the RS232 inverson configuration
 bits.

---
 src/ftdi.c   | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/ftdi.h   |  1 +
 src/ftdi_i.h |  5 +++++
 3 files changed, 68 insertions(+)

diff --git a/src/ftdi.c b/src/ftdi.c
index c62428d..978342b 100644
--- a/src/ftdi.c
+++ b/src/ftdi.c
@@ -3065,6 +3065,7 @@ int ftdi_eeprom_build(struct ftdi_context *ftdi)
             {
                 output[0x1a + j] = eeprom->cbus_function[j];
             }
+            output[0x0b] = eeprom->rs232_inversion;
             break;
     }
 
@@ -3389,6 +3390,8 @@ int ftdi_eeprom_decode(struct ftdi_context *ftdi, int verbose)
         eeprom->group1_drive   = (buf[0x0c] >> 4) & 0x03;
         eeprom->group1_schmitt = (buf[0x0c] >> 4) & IS_SCHMITT;
         eeprom->group1_slew    = (buf[0x0c] >> 4) & SLOW_SLEW;
+
+        eeprom->rs232_inversion = buf[0xb];
     }
 
     if (verbose)
@@ -3512,6 +3515,57 @@ int ftdi_eeprom_decode(struct ftdi_context *ftdi, int verbose)
                 if (eeprom->cbus_function[i]<= CBUSH_AWAKE)
                     fprintf(stdout,"CBUS%d Function: %s\n", i, cbush_mux[eeprom->cbus_function[i]]);
             }
+            if(eeprom->rs232_inversion ) {
+                int i = 0;
+                fprintf(stdout,"Inversion on ");
+                if (eeprom->rs232_inversion & INVERT_TXD) {
+                    fprintf(stdout,"TXD");
+                    i = 1;
+                }
+                if (eeprom->rs232_inversion & INVERT_RXD) {
+                    if (i)
+                        fprintf(stdout,", ");
+                    fprintf(stdout,"RXD");
+                    i = 1;
+                }
+                if (eeprom->rs232_inversion & INVERT_RTS) {
+                    if (i)
+                        fprintf(stdout,", ");
+                    fprintf(stdout,"RTS");
+                    i = 1;
+                }
+                if (eeprom->rs232_inversion & INVERT_CTS) {
+                    if (i)
+                        fprintf(stdout,", ");
+                    fprintf(stdout,"CTS");
+                    i = 1;
+                }
+                if (eeprom->rs232_inversion & INVERT_DTR) {
+                    if (i)
+                        fprintf(stdout,", ");
+                    fprintf(stdout,"DTR");
+                    i = 1;
+                }
+                if (eeprom->rs232_inversion & INVERT_DSR) {
+                    if (i)
+                        fprintf(stdout,", ");
+                    fprintf(stdout,"DSR");
+                    i = 1;
+                }
+                if (eeprom->rs232_inversion & INVERT_DCD) {
+                    if (i)
+                        fprintf(stdout,", ");
+                    fprintf(stdout,"DCD");
+                    i = 1;
+                }
+                if (eeprom->rs232_inversion & INVERT_RI) {
+                    if (i)
+                        fprintf(stdout,", ");
+                    fprintf(stdout,"RI");
+                    i = 1;
+                }
+                fprintf(stdout," Pin(s)\n");
+            }
         }
 
         if (ftdi->type == TYPE_R)
@@ -3734,6 +3788,9 @@ int ftdi_get_eeprom_value(struct ftdi_context *ftdi, enum ftdi_eeprom_value valu
         case CHIP_SIZE:
             *value = ftdi->eeprom->size;
             break;
+        case RS232_INVERSION:
+            *value = ftdi->eeprom->rs232_inversion;
+            break;
         default:
             ftdi_error_return(-1, "Request for unknown EEPROM value");
     }
@@ -3923,6 +3980,11 @@ int ftdi_set_eeprom_value(struct ftdi_context *ftdi, enum ftdi_eeprom_value valu
             break;
         case CHIP_SIZE:
             ftdi_error_return(-2, "EEPROM Value can't be changed");
+            break;
+        case RS232_INVERSION:
+            ftdi->eeprom->rs232_inversion = value;
+            break;
+
         default :
             ftdi_error_return(-1, "Request to unknown EEPROM value");
     }
diff --git a/src/ftdi.h b/src/ftdi.h
index 07fcd71..e192a66 100644
--- a/src/ftdi.h
+++ b/src/ftdi.h
@@ -332,6 +332,7 @@ enum ftdi_eeprom_value
     CHANNEL_C_RS485    = 53,
     CHANNEL_D_RS485    = 54,
     RELEASE_NUMBER     = 55,
+    RS232_INVERSION    = 56,
 };
 
 /**
diff --git a/src/ftdi_i.h b/src/ftdi_i.h
index 19d8dd5..066790b 100644
--- a/src/ftdi_i.h
+++ b/src/ftdi_i.h
@@ -123,6 +123,11 @@ struct ftdi_eeprom
     int data_order;
     int flow_control;
 
+    /* FT-X Device and Peripheral control
+     * Fixme: Decode byte[0xa]
+     */
+    int rs232_inversion;
+
     /** eeprom size in bytes. This doesn't get stored in the eeprom
         but is the only way to pass it to ftdi_eeprom_build. */
     int size;
--
1.8.4.5


--
libftdi - see http://www.intra2net.com/en/developer/libftdi for details.
To unsubscribe send a mail to [hidden email]  

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] -FT230X: Readout, decode and encode the RS232 inverson configuration bits.

Rogier Wolff
On Thu, Aug 14, 2014 at 03:43:01PM +0200, Uwe Bonnes wrote:

> +                fprintf(stdout,"Inversion on ");
> +                if (eeprom->rs232_inversion & INVERT_TXD) {
> +                    fprintf(stdout,"TXD");
> +                    i = 1;
> +                }
> +                if (eeprom->rs232_inversion & INVERT_RXD) {
> +                    if (i)
> +                        fprintf(stdout,", ");
> +                    fprintf(stdout,"RXD");
> +                    i = 1;
> +                }

I would write this a bit different:

Already at the first call of the helper function, the total code is
less. When you reuse the helper, you start having /much/ less code...

Oh, and my code does "one pin" / "two pins" correctly...  :-)

        Roger.

------

#include <stdio.h>
#include <stdlib.h>

#define INVERT_TXD 1
#define INVERT_RXD 2
#define INVERT_RTS 4
#define INVERT_CTS 8
#define INVERT_DTR 16
#define INVERT_DSR 32
#define INVERT_DCD 64
#define INVERT_RI 128

struct bitnames {
  int mask;
  char *name;
};

struct bitnames invbitlist[] = {
  {INVERT_TXD, "TXD"},
  {INVERT_RXD, "RXD"},
  {INVERT_RTS, "RTS"},
  {INVERT_CTS, "CTS"},
  {INVERT_DTR, "DTR"},
  {INVERT_DSR, "DSR"},
  {INVERT_DCD, "DCD"},
  {INVERT_RI, "RI"},
  {0, NULL},
};

void print_bitlist (char *pre, char *post, int bits, struct bitnames *bitnames)
{
  int i, n;
  if (!bits) return;

  printf ("%s", pre);

  for (i=0, n=0;bitnames[i].mask;i++) {
    if(bits & bitnames[i].mask) {
      if (n++) printf (",");
      printf (" %s", bitnames[i].name);
    }
  }
  printf (post, (n==1)?"":"s");
}


int main (int argc, char **argv)
{
  int i;

  i = atoi (argv[1]);
  print_bitlist ("Inversion on", " pin%s\n", i, invbitlist);
  exit (0);
}
 


--
** [hidden email] ** http://www.BitWizard.nl/ ** +31-15-2600998 **
**    Delftechpark 26 2628 XH  Delft, The Netherlands. KVK: 27239233    **
*-- BitWizard writes Linux device drivers for any device you may have! --*
The plan was simple, like my brother-in-law Phil. But unlike
Phil, this plan just might work.

--
libftdi - see http://www.intra2net.com/en/developer/libftdi for details.
To unsubscribe send a mail to [hidden email]  

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Re: [PATCH] -FT230X: Readout, decode and encode the RS232 inverson configuration bits.

Thomas Jarosch
Hi Uwe,

On Monday, 18. August 2014 09:31:54 Rogier Wolff wrote:

> On Thu, Aug 14, 2014 at 03:43:01PM +0200, Uwe Bonnes wrote:
> > +                fprintf(stdout,"Inversion on ");
> > +                if (eeprom->rs232_inversion & INVERT_TXD) {
> > +                    fprintf(stdout,"TXD");
> > +                    i = 1;
> > +                }
> > +                if (eeprom->rs232_inversion & INVERT_RXD) {
> > +                    if (i)
> > +                        fprintf(stdout,", ");
> > +                    fprintf(stdout,"RXD");
> > +                    i = 1;
> > +                }
>
> I would write this a bit different:
>
> Already at the first call of the helper function, the total code is
> less. When you reuse the helper, you start having /much/ less code...
>
> Oh, and my code does "one pin" / "two pins" correctly...  :-)
>
> Roger.

any comment on Roger's comment? Any plans to update your patch?

Cheers,
Thomas


--
libftdi - see http://www.intra2net.com/en/developer/libftdi for details.
To unsubscribe send a mail to [hidden email]  

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Re: [PATCH] -FT230X: Readout, decode and encode the RS232 inverson configuration bits.

Uwe Bonnes

    Thomas> any comment on Roger's comment? Any plans to update your patch?
Reworked patch appended, using array for the strings, but not extra
function.
--
Uwe Bonnes                [hidden email]

Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
--------- Tel. 06151 162516 -------- Fax. 06151 164321 ----------
From f2ca4c0288416076494c4d0b16cae878d9e1f83d Mon Sep 17 00:00:00 2001
From: Uwe Bonnes <[hidden email]>
Date: Thu, 14 Aug 2014 14:34:38 +0200
Subject: FT230X: Readout, decode and encode the RS232 inverson configuration
 bits.

---
 src/ftdi.c   | 38 ++++++++++++++++++++++++++++++++++++++
 src/ftdi.h   |  1 +
 src/ftdi_i.h |  5 +++++
 3 files changed, 44 insertions(+)

diff --git a/src/ftdi.c b/src/ftdi.c
index c62428d..08130c2 100644
--- a/src/ftdi.c
+++ b/src/ftdi.c
@@ -3065,6 +3065,7 @@ int ftdi_eeprom_build(struct ftdi_context *ftdi)
             {
                 output[0x1a + j] = eeprom->cbus_function[j];
             }
+            output[0x0b] = eeprom->rs232_inversion;
             break;
     }
 
@@ -3389,6 +3390,8 @@ int ftdi_eeprom_decode(struct ftdi_context *ftdi, int verbose)
         eeprom->group1_drive   = (buf[0x0c] >> 4) & 0x03;
         eeprom->group1_schmitt = (buf[0x0c] >> 4) & IS_SCHMITT;
         eeprom->group1_slew    = (buf[0x0c] >> 4) & SLOW_SLEW;
+
+        eeprom->rs232_inversion = buf[0xb];
     }
 
     if (verbose)
@@ -3512,6 +3515,33 @@ int ftdi_eeprom_decode(struct ftdi_context *ftdi, int verbose)
                 if (eeprom->cbus_function[i]<= CBUSH_AWAKE)
                     fprintf(stdout,"CBUS%d Function: %s\n", i, cbush_mux[eeprom->cbus_function[i]]);
             }
+            if(eeprom->rs232_inversion ) {
+                struct bitnames {
+                    int mask;
+                    char *name;
+                };
+
+                struct bitnames invbitlist[] = {
+                    {INVERT_TXD, "TXD"},
+                    {INVERT_RXD, "RXD"},
+                    {INVERT_RTS, "RTS"},
+                    {INVERT_CTS, "CTS"},
+                    {INVERT_DTR, "DTR"},
+                    {INVERT_DSR, "DSR"},
+                    {INVERT_DCD, "DCD"},
+                    {INVERT_RI, "RI"},
+                    {0, NULL},
+                };
+                int i, n;
+                printf("Inversion on ");
+                for (i=0, n=0; invbitlist[i].mask;i++) {
+                    if(eeprom->rs232_inversion & invbitlist[i].mask) {
+                        if (n++) printf (",");
+                        printf (" %s", invbitlist[i].name);
+                    }
+                }
+                printf (" Pin%s\n",(n==1)?"":"s");
+            }
         }
 
         if (ftdi->type == TYPE_R)
@@ -3734,6 +3764,9 @@ int ftdi_get_eeprom_value(struct ftdi_context *ftdi, enum ftdi_eeprom_value valu
         case CHIP_SIZE:
             *value = ftdi->eeprom->size;
             break;
+        case RS232_INVERSION:
+            *value = ftdi->eeprom->rs232_inversion;
+            break;
         default:
             ftdi_error_return(-1, "Request for unknown EEPROM value");
     }
@@ -3923,6 +3956,11 @@ int ftdi_set_eeprom_value(struct ftdi_context *ftdi, enum ftdi_eeprom_value valu
             break;
         case CHIP_SIZE:
             ftdi_error_return(-2, "EEPROM Value can't be changed");
+            break;
+        case RS232_INVERSION:
+            ftdi->eeprom->rs232_inversion = value;
+            break;
+
         default :
             ftdi_error_return(-1, "Request to unknown EEPROM value");
     }
diff --git a/src/ftdi.h b/src/ftdi.h
index 07fcd71..e192a66 100644
--- a/src/ftdi.h
+++ b/src/ftdi.h
@@ -332,6 +332,7 @@ enum ftdi_eeprom_value
     CHANNEL_C_RS485    = 53,
     CHANNEL_D_RS485    = 54,
     RELEASE_NUMBER     = 55,
+    RS232_INVERSION    = 56,
 };
 
 /**
diff --git a/src/ftdi_i.h b/src/ftdi_i.h
index 19d8dd5..066790b 100644
--- a/src/ftdi_i.h
+++ b/src/ftdi_i.h
@@ -123,6 +123,11 @@ struct ftdi_eeprom
     int data_order;
     int flow_control;
 
+    /* FT-X Device and Peripheral control
+     * Fixme: Decode byte[0xa]
+     */
+    int rs232_inversion;
+
     /** eeprom size in bytes. This doesn't get stored in the eeprom
         but is the only way to pass it to ftdi_eeprom_build. */
     int size;
--
1.8.4.5


--
libftdi - see http://www.intra2net.com/en/developer/libftdi for details.
To unsubscribe send a mail to [hidden email]  

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] -FT230X: Readout, decode and encode the RS232 inverson configuration bits.

Thomas Jarosch
On Tuesday, 2. September 2014 13:50:32 Uwe Bonnes wrote:
>     Thomas> any comment on Roger's comment? Any plans to update your
> patch? Reworked patch appended, using array for the strings, but not
> extra function.

Applied, thanks Uwe! I've slightly tweaked the commit message.

Thomas


--
libftdi - see http://www.intra2net.com/en/developer/libftdi for details.
To unsubscribe send a mail to [hidden email]  

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] -FT230X: Readout, decode and encode the RS232 inverson configuration bits.

Rogier Wolff
On Mon, Sep 08, 2014 at 04:57:50PM +0200, Thomas Jarosch wrote:
> On Tuesday, 2. September 2014 13:50:32 Uwe Bonnes wrote:
> >     Thomas> any comment on Roger's comment? Any plans to update your
> > patch? Reworked patch appended, using array for the strings, but not
> > extra function.

My question is: Why NOT the extra function? Performance? Is this a
critical path? Is it called very often? I don't think so.

I think it's much more likely that a "bunch of names for bitfields"
need printing elsewhere, so that the function would be reused.

Now, if someone finds himself needing to print a bunch of names for
bitfields, he'll look for an example, and find your "using array for
the string, but not the extra function" code, and copy that. This is
the easiest way out.

This started with

  if (bitfield & 1) printf ("someflag");

which got copied around. That was the easiest (least thinking-work
required) solution.


So I'd say:
 ALWAYS create a separate function.
   except:
      if the code is NEVER going to be called from somewhere else
    and
      the code is more readable with the function "inline"

An exception can be made for a performance critical piece of code
where inlining through a compiler directive is ineffective.

> Applied, thanks Uwe! I've slightly tweaked the commit message.

I don't have the time right now to rework it as I think it should be
done. :-(

I hope to be able to influence your programming styles for the future,
and "fixing" this can be done "when convenient"... (if you're
convinced by my arguments... :-) )

        Roger.

--
** [hidden email] ** http://www.BitWizard.nl/ ** +31-15-2600998 **
**    Delftechpark 26 2628 XH  Delft, The Netherlands. KVK: 27239233    **
*-- BitWizard writes Linux device drivers for any device you may have! --*
The plan was simple, like my brother-in-law Phil. But unlike
Phil, this plan just might work.

--
libftdi - see http://www.intra2net.com/en/developer/libftdi for details.
To unsubscribe send a mail to [hidden email]  

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

variable shadowing

Michel Zou

hi,

in ftdi_stream.c:155, the return code declaration at beginnign of ftdi_readstream
int err = 0;
is overriden by at line 243 by:

do
    {
        FTDIProgressInfo  *progress = &state.progress;
        const double progressInterval = 1.0;
        struct timeval timeout = { 0, ftdi->usb_read_timeout };
        struct timeval now;

        int err = libusb_handle_events_timeout(ftdi->usb_ctx, &timeout);
        if (err ==  LIBUSB_ERROR_INTERRUPTED)
            /* restart interrupted events */
            err = libusb_handle_events_timeout(ftdi->usb_ctx, &timeout);


if the final return code should depend on the return code of libusb_handle_events_timeout,
it's a bug, if not a different name should be chosen.

here's a patch for a similar fix for a more trivial case in ftdi.c.

xan.


libftdi - see http://www.intra2net.com/en/developer/libftdi for details.
To unsubscribe send a mail to [hidden email]



0001-Fix-declaration-shadow-in-ftdi.c.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: variable shadowing

Thomas Jarosch
Hi Xan,

On Sunday, 14. September 2014 10:38:36 xantares 09 wrote:

> in ftdi_stream.c:155, the return code declaration at beginnign of
> ftdi_readstream int err = 0;
> is overriden by at line 243 by:
>
> do
>     {
>         FTDIProgressInfo  *progress = &state.progress;
>         const double progressInterval = 1.0;
>         struct timeval timeout = { 0, ftdi->usb_read_timeout };
>         struct timeval now;
>
>         int err = libusb_handle_events_timeout(ftdi->usb_ctx, &timeout);
>         if (err ==  LIBUSB_ERROR_INTERRUPTED)
>             /* restart interrupted events */
>             err = libusb_handle_events_timeout(ftdi->usb_ctx, &timeout);
>
>
> if the final return code should depend on the return code of
> libusb_handle_events_timeout, it's a bug, if not a different name should
> be chosen.

thanks for reporting this issue, I've applied your patch.

I think you are right, we should probably change "int err ="
to just "err =".

@Uwe: Do you agree? It's your code :)


Also, looking closely at that code, it calls libusb_handle_events_timeout()
again on LIBUSB_ERROR_INTERRUPTED. But it does that only *once*.
Is that intentional? What if the system is temporarily really busy?


Thanks to the Shellshock bash bug my time schedules were a bit messed up.
So I guess we won't have a libftdi release at the end of September.
(I'll also be AFK for a few days, too)

Cheers,
Thomas


--
libftdi - see http://www.intra2net.com/en/developer/libftdi for details.
To unsubscribe send a mail to [hidden email]  

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: variable shadowing

Uwe Bonnes
>>>>> "Thomas" == Thomas Jarosch <[hidden email]> writes:

    Thomas> Hi Xan, On Sunday, 14. September 2014 10:38:36 xantares 09
    Thomas> wrote:
    >> in ftdi_stream.c:155, the return code declaration at beginnign of
    >> ftdi_readstream int err = 0; is overriden by at line 243 by:
    >>
    >> do { FTDIProgressInfo *progress = &state.progress; const double
    >> progressInterval = 1.0; struct timeval timeout = { 0,
    >> ftdi->usb_read_timeout }; struct timeval now;
    >>
    >> int err = libusb_handle_events_timeout(ftdi->usb_ctx, &timeout); if
    >> (err == LIBUSB_ERROR_INTERRUPTED) /* restart interrupted events */
    >> err = libusb_handle_events_timeout(ftdi->usb_ctx, &timeout);
    >>
    >>
    >> if the final return code should depend on the return code of
    >> libusb_handle_events_timeout, it's a bug, if not a different name
    >> should be chosen.

    Thomas> thanks for reporting this issue, I've applied your patch.

    Thomas> I think you are right, we should probably change "int err =" to
    Thomas> just "err =".

    Thomas> @Uwe: Do you agree? It's your code :)

Your solution sounds sensible.

    Thomas> Also, looking closely at that code, it calls
    Thomas> libusb_handle_events_timeout() again on
    Thomas> LIBUSB_ERROR_INTERRUPTED. But it does that only *once*.  Is that
    Thomas> intentional? What if the system is temporarily really busy?

What to do in case of error, especially in errors your experiments did't
show, is always problematic. I think our default  for timeout is 5
Milliseconds. With two times that timeout and at a rate of 20 MByte per
second, after 10 ms we have 200 kbyte outstanding. Mostly the buffer on the
ftdi side will hold less. So probably I thought that one retry is a good
compromise.

But if people have other experience, the code is not fixed in stone...

Bye
--
Uwe Bonnes                [hidden email]

Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
--------- Tel. 06151 162516 -------- Fax. 06151 164321 ----------

--
libftdi - see http://www.intra2net.com/en/developer/libftdi for details.
To unsubscribe send a mail to [hidden email]  

Loading...