search

eclipse-archived / smarthome

Eclipse SmartHome™ project
https://www.eclipse.org/smarthome/
Eclipse Public License 2.0
862 stars 786 forks source link

Serial abstraction should hide implementations receive-threshold / receive-timeout #6280

Open martinvw opened 5 years ago

martinvw commented 5 years ago

CC: @msteigenberger

In the DSMR binding we do a call to the ESH serial abstraction:

https://github.com/openhab/openhab2-addons/blob/7ce13315c1f8ce640ad9822014feaa3e91126ef5/addons/binding/org.openhab.binding.dsmr/src/main/java/org/openhab/binding/dsmr/internal/device/connector/DSMRSerialConnector.java#L154

            serialPort.enableReceiveThreshold(SERIAL_TIMEOUT_MILLISECONDS);
            serialPort.enableReceiveTimeout(SERIAL_TIMEOUT_MILLISECONDS);

However this is implemented NrJavaSerial (in serial over ip) as:

        throw new UnsupportedCommOperationException();

https://github.com/NeuronRobotics/nrjavaserial/blob/9a407b820606ff15504244c17adec2d57ba60897/src/main/java/gnu/io/rfc2217/TelnetSerialPort.java#L900

So because the binding calls a method from the abstraction it cannot use tcp-over-ip anymore, if this is desirable we should maybe throw a very specific exception or otherwise handle it in somewhere or maybe even propose a fix in NrJavaSerial

msteigenberger commented 5 years ago

Hmm... From the docs it says this method throws a UnsupporteeCommException if the underlying driver doesn't support it: https://docs.oracle.com/cd/E17802_01/products/products/javacomm/reference/api/javax/comm/CommPort.html#enableReceiveThreshold(int)

So anyhow it has to be handled by the binding either by making it optional (catching the exception and move on) or stopping the binding to work. But imho it should be better documented in the ESH SerialPort.

Hilbrand commented 5 years ago

Reading the docs I was inclined to follow you, however I think it isn't the whole story for the following to reasons: Firstly the ESH is not a 1-on-1 copy of the javax interface. For example it catches the NoSuchPortException and thus deviates from the specification. The second reason is. While the javax interface itself is a generic interface to com ports, a developer programming directly to a specific comport implementation actively controls the driver used and knows what that driver can or can't do. With the ESH abstraction layer the developer has no control over which driver is actually used. This depends on what the actual user has installed or in this case what kind of connection is configured. And that means that the binding using the ESH abstraction layer can't know if the exception thrown is a problem or not because it doesn't know what actual driver is used. So it can't know how to handle it, it can't know if it should stop or can continue. In this case the binding stopped because it gets a error and doesn't know it was not a problem in this case because it can't know even though it was not a problem in this case. Therefor the ESH wrapper should take care of this and throw an exception when it's a problem and not when it's not a problem if the method is not supported imho. P.s. even the docs seem to be ambigue. It speaks about calling another method after calling the enable method to see if it's supported. So why would one need to call a test method if it would simply throw an exception? It suggests somehow no exceptions might be thrown even when it's not supported.

msteigenberger commented 5 years ago

I partly agree to your first point, however I disagree to your second.

  1. The ESH abstraction is not a copy, so it can handle things different.
  2. The ESH is just an abstraction to serial implementations but it's still bound to serial ports. Therefore it must handle everything serial related and provide the necessary functionally. How can the abstraction know how to handle the case if some driver is not supporting something? Only the binding can know if the specific functionally is required or not and can handle it by giving up or ignoring it. Btw. In the case when someone is creating an application with the direct serial implementation he also does not know on which system with which serial device it is executed...

P.S. Also the javadoc is not ambiguous imho. It's not a good API design to have a method which is going to fail under specific (known) circumstances, It should rather provide a possiblity to check whether the circumstances are met or not (and additionally fail the method if it is called when it's not met).

Hilbrand commented 5 years ago

How can the abstraction know how to handle the case if some driver is not supporting something?

In this case for each driver supported there is code that wraps and handles that specific driver. This means the abstraction knows exactly what the specific driver supports or not. In this specific case it's known it simply throws an exception because that's how they decided to implement it. So the abstraction can handle this specific case and wrap it to make the abstraction work seemingly the same for everyone.

I think the implementation in the telnet driver was a bad design decision. For example the exception is also thrown when setSerialPortParams is called with an unsupported baudrate. That make sense to me, because in such a case the driver can't work. However calling enableReceiveThreshold won't have any effect on the operation of the driver, but it does throw the exception in the telnet driver. So in my binding I have to handle the exception differently. When setSerialPortParams throws the exception I know I can't continue because it would not work. This is what I would assume the exception means. But when enableReceiveThreshold throws the exception I can ignore it. Well actually I don't know if I can ignore it. All I know is that when the driver is the telnet driver and this exception is thrown I can ignore it. But I don't (want to) know if the driver is the telnet driver and that is why I think the abstraction should handle this. I rest my case :smile:

msteigenberger commented 5 years ago

IMO it depends on what you see as the driver. The concrete serial implementation (rfc2217) is IMO already part of the driver. It either supports enableReceiveThreshold or not.

So in my binding I have to handle the exception differently

Why? The only thing you know is, that it is not supported. That can happen with rxtx implementation as well (because the actual driver doesn't support it). You would handle it the same way. (either you need the functionality and hence give up or decide to move on)

But I don't (want to) know if the driver is the telnet driver and that is why I think the abstraction should handle this.

Full ack. As said I think it is handled correctly from what the implementation can do from what the rfc is supporting.

lolodomo commented 5 years ago

I encountered the same problem with the new Sony projector binding.

2019-01-26 12:32:12.483 [DEBUG] [s.SonyProjectorSerialConnector:122  ] - open Serial failed: Unsupported Comm Operation Exception: gnu.io.UnsupportedCommOperationExceptionorg.eclipse.smarthome.io.transport.serial.UnsupportedCommOperationException: gnu.io.UnsupportedCommOperationException
    at org.eclipse.smarthome.io.transport.serial.rxtx.RxTxSerialPort.enableReceiveThreshold(RxTxSerialPort.java:157)
    at org.openhab.binding.sonyprojector.internal.communication.serial.SonyProjectorSerialConnector.open(SonyProjectorSerialConnector.java:93)
    at org.openhab.binding.sonyprojector.internal.handler.SonyProjectorHandler.pollProjector(SonyProjectorHandler.java:327)
    at org.openhab.binding.sonyprojector.internal.handler.SonyProjectorHandler.lambda$0(SonyProjectorHandler.java:296)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
    at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:308)
    at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:180)
    at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:294)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:748)
Caused by: gnu.io.UnsupportedCommOperationException: null
    at gnu.io.rfc2217.TelnetSerialPort.enableReceiveThreshold(TelnetSerialPort.java:948)
    at org.eclipse.smarthome.io.transport.serial.rxtx.RxTxSerialPort.enableReceiveThreshold(RxTxSerialPort.java:155)
    at org.openhab.binding.sonyprojector.internal.communication.serial.SonyProjectorSerialConnector.open(SonyProjectorSerialConnector.java:93)
    at org.openhab.binding.sonyprojector.internal.handler.SonyProjectorHandler.pollProjector(SonyProjectorHandler.java:327)
    at org.openhab.binding.sonyprojector.internal.handler.SonyProjectorHandler.lambda$0(SonyProjectorHandler.java:296)
    at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
    at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:308)
    at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:180)
    at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:294)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)

What is the default considered when enableReceiveThreshold is not supported ?

lolodomo commented 5 years ago

Without the ability to call enableReceiveThreshold and enableReceiveTimeout, the behaviour I notice is that the first call to dataIn.read(dataBuffer) is returning only one byte which is the expected start code of my frame but unfortunately the second call to dataIn.read(dataBuffer) is just blocking indefinitely waiting for data ! Help would be appreciated. @martinvw : was a solution found for the DSMR binding ?

martinvw commented 5 years ago

I run a home compiled version with just that call disabled it works fine

Hilbrand commented 5 years ago

@lolodomo 'without the calls' is that when you use it as serial or when used with the remote configuration?

lolodomo commented 5 years ago

With remote configuration.

lolodomo commented 5 years ago

Bingo, I solved my problem which was due to the usual RXTX library CPU load workaround we apply, consisting in a sleep forever when data availability is notified. So when serial over IP (RFC2217) is used, this workaround has to avoided. I will try and fix the Powermax binding to be compatible with serial over IP.

maggu2810 commented 5 years ago

Hi @lolodomo, are there still some open questions? I have added RFC2217 to a proprietary EnOcean binding some time and could check my code.

lolodomo commented 5 years ago

No open questions.

lolodomo commented 5 years ago

Finally, the lack of receive timeout is a real problem. Several bindings assume the read is not blocking. Having a blocking read for serial over IP make these bindings not really compatible with serial over IP.

Is there a technical reason to not implement reading timeout (not blocking read) for serial over IP ?

With an OFF device, the current implementation allows opening a serial over IP port without errors, allows even sending data to the device without errors, and blocks the thead in case of reading.