RS485 support needs work

MarkRivers commented 8 years ago

I have made a new branch, rs485. This branch started with Florian's pull request. I made the following fixes:

Made it work on vxWorks. The problem was that ioctl was being illegally redefined in default/serial_rs485.h. I removed the definition of ioctl from that file.
Made it work on older Linux systems. These systems don't define the RS485 constants in linux/serial.h, and the ioctl won't work. I test whether RS485 is supported by seeing if serial.h defines SER_RS485_ENABLED. If it does then I assume RS-485 is supported and define the symbol ASYN_RS485_SUPPORTED. In drvAsynSerialPort.c the ioctl for RS485 is only executed if that symbol is defined. It is not defined in default/serial_rs485.h or for older Linux systems.

There is still a problem. I ran the testGpibSerial IOC on a newer Linux system which defines SER_RS485_ENABLED and so I think does support RS485. I get the following errors when I boot the IOC:

drvAsynSerialPortConfigure("L0","/dev/ttyS0",0,0,0)
asynSetOption("L0", -1, "baud", "2400")
setOption failed ioctl TIOCSRS485 failed: Invalid argument
asynSetOption("L0", -1, "bits", "8")
setOption failed ioctl TIOCSRS485 failed: Invalid argument
asynSetOption("L0", -1, "parity", "none")
setOption failed ioctl TIOCSRS485 failed: Invalid argument
asynSetOption("L0", -1, "stop", "1")  
setOption failed ioctl TIOCSRS485 failed: Invalid argument
asynSetOption("L0", -1, "clocal", "Y")
setOption failed ioctl TIOCSRS485 failed: Invalid argument
asynSetOption("L0", -1, "crtscts", "N")
setOption failed ioctl TIOCSRS485 failed: Invalid argument

So every time any option is set it does the ioctl for RS485 and gets an error. Is this because RS485 is only supported on certain types of serial hardware that support RS485? Or is there some other reason for these errors, such as one of the arguments actually being invalid?

tboegi commented 8 years ago

There are 2-3 things needed to be able to use RS485:

(a) The hardware must support it
(b) The Linux kernel must support it
(c) The user-space application must be changed to use (b)

After reading https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/serial/serial-rs485.txt?id=refs/tags/v4.8-rc2

I speculate that your system has (b) and (c), but not (a)

ffeldbauer commented 8 years ago

Due to missing hardware, I can only test the RS485 support on the BeagleBone Black for which I originally implemented it. For my 64-bit linux systems I do not have an RS485 interface. But testing it with one of the /tty/devSx interfaces, which are all RS232, I get the same error as you. In this case it is due to the missing hardware support as mentioned above.

tboegi commented 8 years ago

Another comment: May be the whole rs485 branch can be squashed into a new one, with one commit ? And the commit message states clearly that this works only under Linux, and only on certain boards ? And it may be helpful for others to mention that it has been tested on a BeagleBone Black.

ulrikpedersen commented 8 years ago

May be the whole rs485 branch can be squashed into a new one, with one commit ?

That's relatively easy: git rebase -i 9faa69854ecd96d85f8a1d42b0b72f2dbae548cd. Follow the instructions in the editor to squash all commits into one and edit the new commit message...

It does make for a shorter/cleaner repository history. Always do this by creating a new branch first to avoid having to do a --force push and messing up everyone else's clones.

I have done this on a branch on my fork: dls-controls/asyn/squashed_rs485 commit: 33416cbe6fc23475e51eaf9646884f47e25a53b6

I just squashed the commit messages together and formatted them.

MarkRivers commented 8 years ago

But testing it with one of the /tty/devSx interfaces, which are all RS232, I get the same 
error as you. In this case it is due to the missing hardware support as mentioned above.

Since the ioctl for RS485 is made for every option change, even if not RS485 related (baud, parity, etc.) this is a serious problem. Users will see RS485 error messages every time they set an option if their hardware does not support RS485. I can think of 3 solutions:

Use a system call to determine if the port supports RS485. I don't know if such a call exists.
Add a configuration flag to drvAsynIPPortConfigure to tell the driver that the port supports RS485. This is difficult to do in a backwards compatible manner.
Only do the ioctl call if an RS485 option is set. This seems quite straightforward. With this method the user will only see an error message if they attempt to set an RS485 option on hardware that does not support RS485.

ffeldbauer commented 8 years ago

Wait. You are right. Sorry. This error I only got, when using one of the RS485 specific options. I will check!

MarkRivers commented 8 years ago

I merged master into rs485 and pushed back to Github. Florian you should test with this updated branch.

ffeldbauer commented 8 years ago

I'm not sure why, but there is an if-clause missing in the applyOptions method. Maybe I added it while testing on the BeagleBone but forgot to add it in git? The corresponding block should be

#ifdef ASYN_RS485_SUPPORTED
    if( tty->rs485.flags & SER_RS485_ENABLED ) {
      if( ioctl( tty->fd, TIOCSRS485, &tty->rs485 ) < 0 ) {
          epicsSnprintf(pasynUser->errorMessage, pasynUser->errorMessageSize,
                                "ioctl TIOCSRS485 failed: %s", strerror(errno));
          return asynError;
      }
  }
#endif

In this case the the ioctl command is only called, if the rs485_enable option was set.

MarkRivers commented 8 years ago

But doesn't that prevent the user from disabling RS485 mode? If they do

asynSetOption("rs485_enable", "N")

it won't do the ioctl call, and perhaps they need to do that.

ffeldbauer commented 8 years ago

well, yes....but does it make sense to disable it, when the hardware is a RS485 interface? I guess I have to think about it some more to come up with a good solution. Maybe one has to split the applyOptions method? the second doing only the RS485 ioctl call, which will only be called if at least on of the options was called?

MarkRivers commented 8 years ago

Maybe one has to split the applyOptions method? the second doing only the RS485 ioctl call, 
which will only be called if at least on of the options was called?

That was my thought. I will implement that (already partly done) and push to github.

MarkRivers commented 8 years ago

I have pushed a new version. On my Linux system where the serial ports don't support RS485 I now don't get any errors when setting options like baud. I do get an error if I set rs485_enable, which is expected.

ffeldbauer commented 8 years ago

I will test your new push tomorrow on my beagle bone!

MarkRivers commented 8 years ago

Did you have a chance to test this? I'd like to include it in asyn R4-30, to be released soon.

ffeldbauer commented 8 years ago

Yes, and after I fixed a broken cable it worked without any problems on the beaglebone.

MarkRivers commented 8 years ago

Thanks.

MarkRivers commented 8 years ago

I just merged this branch with master (I did not rebase, since I was not clear on how to do it correctly).

Fortunately the APS Jenkins build server ran on the new version of master and found an error before I officially released R4-30. Here is the error

/usr/bin/gcc -c  -D_GNU_SOURCE -D_DEFAULT_SOURCE            -D_X86_64_  -DUNIX  -Dlinux     -O3 -g   -Wall       -m64     -fPIC -MMD -I. -I../O.Common -I. -I../../asyn/drvAsynSerial/os/Linux -I../../asyn/drvAsynSerial/os/default -I.. -I../../asyn/asynDriver -I../../asyn/asynGpib -I../../asyn/drvAsynSerial -I../../asyn/interfaces -I../../asyn/miscellaneous -I../../asyn/asynPortDriver/exceptions -I../../asyn/asynPortDriver -I../../asyn/asynPortClient -I../../asyn/devEpics -I../../asyn/asynRecord -I../../asyn/vxi11 -I../../asyn/gsIP488 -I../../asyn/ni1014 -I../../asyn/devGpib -I../../include/os/Linux -I../../include  -I/Jenkins/jenkins/jobs/epics-ipac/workspace/include  -I/Jenkins/jenkins/jobs/epics-seq/workspace/include -I/Jenkins/jenkins/jobs/epics-base-3.14/workspace/include/os/Linux -I/Jenkins/jenkins/jobs/epics-base-3.14/workspace/include        ../../asyn/drvAsynSerial/drvAsynSerialPort.c 
../../asyn/drvAsynSerial/drvAsynSerialPort.c: In function ‘getOption’:
../../asyn/drvAsynSerial/drvAsynSerialPort.c:213: error: ‘struct serial_rs485’ has no member named ‘delay_rts_after_send’
../../asyn/drvAsynSerial/drvAsynSerialPort.c: In function ‘setOption’:
../../asyn/drvAsynSerial/drvAsynSerialPort.c:550: error: ‘struct serial_rs485’ has no member named ‘delay_rts_after_send’
make[2]: *** [drvAsynSerialPort.o] Error 1

So on this version of Linux the serial_rs485 structure is defined, but it does not contain the delay_rts_before_send or delay_rts_after_send members. I don't know how to test for this. Is there a way to determine which Linux kernel version introduced these members in the structure, so we can restrict building to those kernel versions and higher?

anjohnson commented 8 years ago

The Jenkins master currently runs on a RHEL 6.8 box, which has the before_send member but not the after_send one:

struct serial_rs485 {
    __u32   flags;          /* RS485 feature flags */
#define SER_RS485_ENABLED       (1 << 0)
#define SER_RS485_RTS_ON_SEND       (1 << 1)
#define SER_RS485_RTS_AFTER_SEND    (1 << 2)
    __u32   delay_rts_before_send;  /* Milliseconds */
    __u32   padding[6];     /* Memory is cheap, new structs
                       are a royal PITA .. */
};

RHEL7.2 has both:

struct serial_rs485 {
    __u32   flags;          /* RS485 feature flags */
#define SER_RS485_ENABLED       (1 << 0)    /* If enabled */
#define SER_RS485_RTS_ON_SEND       (1 << 1)    /* Logical level for
                               RTS pin when
                               sending */
#define SER_RS485_RTS_AFTER_SEND    (1 << 2)    /* Logical level for
                               RTS pin after sent*/
#define SER_RS485_RX_DURING_TX      (1 << 4)
    __u32   delay_rts_before_send;  /* Delay before send (milliseconds) */
    __u32   delay_rts_after_send;   /* Delay after send (milliseconds) */
    __u32   padding[5];     /* Memory is cheap, new structs
                       are a royal PITA .. */
};

There is an additional macro defined there, SER_RS485_RX_DURING_TX, but it would not be wise to use its presence to determine whether the after_send member should be present, and in fact the older macros aren't really meant to indicate whether the field is present anyway.

The commit log of changes to the kernel's linux/serial.h header can be found here.

Sorry, no answers, just more code archeology...

ffeldbauer commented 8 years ago

I will try to find out.

On 08/19/2016 04:50 PM, Mark Rivers wrote:

I just merged this branch with master (I did not rebase, since I was not clear on how to do it correctly).

Fortunately the APS Jenkins build server ran on the new version of master and found an error before I officially released R4-30. Here is the error

|/usr/bin/gcc -c -D_GNU_SOURCE -D_DEFAULT_SOURCE -D_X8664 -DUNIX -Dlinux -O3 -g -Wall -m64 -fPIC -MMD -I. -I../O.Common -I. -I../../asyn/drvAsynSerial/os/Linux -I../../asyn/drvAsynSerial/os/default -I.. -I../../asyn/asynDriver -I../../asyn/asynGpib -I../../asyn/drvAsynSerial -I../../asyn/interfaces -I../../asyn/miscellaneous -I../../asyn/asynPortDriver/exceptions -I../../asyn/asynPortDriver -I../../asyn/asynPortClient -I../../asyn/devEpics -I../../asyn/asynRecord -I../../asyn/vxi11 -I../../asyn/gsIP488 -I../../asyn/ni1014 -I../../asyn/devGpib -I../../include/os/Linux -I../../include -I/Jenkins/jenkins/jobs/epics-ipac/workspace/include -I/Jenkins/jenkins/jobs/epics-seq/workspace/include -I/Jenkins/jenkins/jobs/epics-base-3.14/workspace/include/os/Linux -I/Jenkins/jenkins/jobs/epics-base-3.14/workspace/include ../../asyn/drvAsynSerial/drvAsynSerialPort.c ../../asyn/drvAsynSerial/drvAsynSerialPort.c: In function ‘getOption’: ../../asyn/drvAsynSerial/drvAsynSerialPort.c:213: error: ‘struct serial_rs485’ has no member named ‘delay_rts_after_send’ ../../asyn/drvAsynSerial/drvAsynSerialPort.c: In function ‘setOption’: ../../asyn/drvAsynSerial/drvAsynSerialPort.c:550: error: ‘struct serial_rs485’ has no member named ‘delay_rts_after_send’ make[2]: *\ [drvAsynSerialPort.o] Error 1 |

So on this version of Linux the serial_rs485 structure is defined, but it does not contain the delay_rts_before_send or delay_rts_after_send members. I don't know how to test for this. Is there a way to determine which Linux kernel version introduced these members in the structure, so we can restrict building to those kernel versions and higher?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/epics-modules/asyn/issues/29#issuecomment-241039643, or mute the thread https://github.com/notifications/unsubscribe-auth/ACBHzbkf79jENIp8MUxgzY9hFsas98T-ks5qhcK_gaJpZM4Jl8mm.

Dr. Florian Feldbauer
Helmholtz-Institut Mainz /
Johannes Gutenberg-Universität Mainz
Johann-Joachim-Becher-Weg 36
D-55128 Mainz

Office: SB1 / 00-213

| Phone: (+49)6131 / 39-29605 |

ffeldbauer commented 8 years ago

It seems, the delay_rts_after_send was introduced with this patch: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=1b6331848b69d1ed165a6bdc75c4046d68767563

According to the source tree, this patch was included in kernel version 2.6.35

The patch also introduced the flag SER_RS485_RTS_BEFORE_SEND.

Dr. Florian Feldbauer
Helmholtz-Institut Mainz /
Johannes Gutenberg-Universität Mainz
Johann-Joachim-Becher-Weg 36
D-55128 Mainz

Office: SB1 / 00-213

| Phone: (+49)6131 / 39-29605 |

MarkRivers commented 8 years ago

I have changed os/Linux/serial_rs485.h so it only defines ASYN_RS485_SUPPORTED for kernel 2.6.35 and greater. There were problems with conflicting definitions of the serial_rs485 structure in older versions of Linux, so now all references to RS485 support in drvAsynSerialPort.c are only compiled if ASYN_RS485_DEFINED.is true. This builds on all systems I have tested, including the APS Jenkins build server. @ffeldbauer please test the latest Github master on Beaglebone.

ffeldbauer commented 8 years ago

The latest master is working on the beagle bone!

tboegi commented 8 years ago

2 Minor comments after a code-review:

The flags are unsigned: https://github.com/EuropeanSpallationSource/asyn/commit/baa3e6082ba30c7a36ad44d7f24979b57bd6666c
The struct serial_rs485 is not necessarily future-proof: https://github.com/EuropeanSpallationSource/asyn/commit/4fd1c14fa5fffdf9120ed9aa0e0242df5087d44b

Both commits are here: https://github.com/EuropeanSpallationSource/asyn/tree/rs485-2

MarkRivers commented 8 years ago

I have made the change to use %u rather than %d in the format strings.

The second change is no longer needed because the dummy serial_rs485 structure is no longer defined in os/Linux/serial_rs485.h.

MarkRivers commented 8 years ago

Closing this. R4-30 has the RS-485 support and builds OK on all platforms tested.

epics-modules / asyn

RS485 support needs work #29

| Phone: (+49)6131 / 39-29605 |

| Phone: (+49)6131 / 39-29605 |