search

dronefleet / mavlink

A Java API for MAVLink communication
MIT License
110 stars 67 forks source link

Connection.next() should timeout or be preemptable #8

Closed Enlade closed 6 years ago

Enlade commented 6 years ago

In some instances it might be benificial for connection.next() to timeout or to be able to be forced to return/exit. Even in the case where a user opens up a connection to some COM port they might determine that that port was the wrong COM port and thus they would want to stop connection.next() from continuing. However, in my case I am writting code that performs an auto connect by walking through each COM port looking for a connected MAVLink device (UAV). In my instance, if the COM port is not connected to a MAVLink device the code will lock up waiting for the next message from that port when it will never come. So, it would be nice if there was some way of telling connection.next() to return if it does not recieve a message within some timeout, or to implement the code in a way that allows the user to stop the process themselves (where they can implement thier own timeout timer).

benbarkay commented 6 years ago

Most communication implementations provide timeout mechanisms (as they should). Socket#setSoTimeout, or DatagramSocket#setSoTimeout, for instance. Whatever provides your InputStream should have such mechanism. Adding another timeout mechanism would be redundant and adding unnecessary complexity. You can add such redundancy in your app by closing the connection or the inputstream on your own terms from within the scope of your application.

Enlade commented 6 years ago

Yea, the problem is with serial connections. There are a number of serial libraries that one can use under Windows with JAVA, but in general support of serial is a bit messy. I have tried a number of different libraries and settled on jSerialComm. In any case, there are a number of issues that make relying on the ports timeout settings that you might want to consider when you relate it to Serial ports.

First, determining the end of a serial stream is not a simple case when you are simply connecting to an existing arbitrary port. The port may be a MAVLink device and thus data streams in nicely, or it could be some other device (sensor or such) that is also streaming data, but not MAVLink messages. If your code sees data on the line (Mavlink messages, other non-Mavlink data, or just noise) that is not Magic_V1 or Magic_V2 then that data is simply rolled back and the code never exists from MavlinkFrameReader.next(). What I am suggesting is not a timeout on the port because it is not seeing any data, but rather a timeout on the reader because it is not seeing any MAVLink messages. The latter will timeout even if you connect to some sensor that is streaming data that is not a MAVLink device.

Another problem is that even if I set a timeout on the serial port (which I do), the call to in.read() returns a 0 and not a -1 (at least for the serial port libraries I have used). The idea with serial is that you expect the data to come in with some format (your protocol) and if it doesn't you dump it until you do see the data you are after. Which is kind of what you do, you start by looking for the Magic number and then you try to build your message from the other incoming bytes, and if you determine that the data is not a message you dump it and move on listening for the next Magic number, etc. All I'm suggesting is that if you do that for a while and you don't see any valid messages that you should timeout and report such back to the caller (maybe throw a timeout exception).

I have tested this with all the different timeout settings, and the port timeout settings do effect the call to read.in(), but since that method returns 0 when it times out, my code currently stays inside of MavlinkFrameReader().next(). Also, if I connect the code to a sensor that is streaming non-Mavlink data the timeout has no effect since read.in() returns valid data (just not MAVLink data because it is not a MAVLink device). So, that still locks me up in the connection.next() method (so my code locks up waiting).

I understand that the way the library is currently written a user can implement a strategy of requiring that the program always connect only to a com port where a MAVLink device is connected; however, if you put in a timeout after which no actual MAVLink message is read then the user can connect to anything and it will timeout if it does not see an actual MAVLink message within some time frame.

benbarkay commented 6 years ago

MavlinkConnection sits as middleware between different connectivity layers and the app that it runs in, it would be a stretch for it to handle more than what InputStream offers and predict meanings of signals of different comm implementations. Timeout belongs in either the connectivity layer or the business logic layer, both of which are beneath or above where MavlinkConnection sits. For all that MavlinkConnection knows, you're writing a daemon that would listen on a comm port and wait for a connection for weeks.

You can close the connection from another thread if no heartbeat is received within a certain duration at the application scope, which is what I've been doing in my apps so far, and will likely implement in rxmavlink which will depend on this library.

Enlade commented 6 years ago

I'm not saying remove the timeout for the port or to handle the port timeout within the MavlinkConnection layer. What I'm saying is to add an additional timeout on the MavlinkConnection layer that is based not on the port timeout, but on whether it sees a valid message or not (within some time). They are two different types of timeouts, and both are needed. You already make this determination throughout your MavlinkConnection code. You already check for Magic, and try to build a valid message. At some point you determine it is a valid message or you determine it is not a valid message. So, Mavlink is already all over your connection code. All you need to do is kick out of next() if you are unable to find a valid next message after some time period. Its such an easy modification to your code and handles every case without putting the burden on your users to set up threads and manage killing a serial connection prematurely (not something JAVA on Windows likes too much).

Personally, I will just add the few little modifications to your code to have it time out when it has not found a valid message within some timeframe. I'll still use the ports timeout to get in.read() to exit periodically, but I'll drop in an "if" statement or two to gracefully exit Connection.next() if no valid Mavlink message is found. You have already done a great job (and all the hard work) of determining whether it is or is not a valid message, so the changes are simple enough. I'm just raising this as an issue for anyone that might be running into this problem. If someone is writing code that talks to a single known COM port that is guaranteed to be a running MAVLink device then the library will work fine as is, but if you are writing a larger application that either lets the user pick which port to connect to or you implement some auto connect functionality for serial port connections then you will have to do something different to work around the way that the library code is written, or your code will lock up. It is easier to just modify the library code then to try to manage killing the serial connection prematurely.

The point is that such a modification is easy to do, a declaration statement, an assignment statement, and one or two condition statements, depending on how nice you want to make it; and it would work fine with all the different stream types including Serial; and it allows for user to easily implement whatever approach to their project they desire (hard code the COM port to a known MavLink device, or allow user select/auto find functionality for the COM port).