Better waitEvents implementation for *nix-like systems

[graynk]

jssc/src/cpp/_nix_based/jssc.cpp

Line 716 in 0d0e6f7

* Collecting data for EventListener class (Linux have no implementation of "WaitCommEvent" function from Windows)

So, this line mentions that there's no analogue for WaitCommEvent from WinApi. However, isn't that what poll/epoll/select functions are for? My concern is that for me using this library on Linux causes a memory leak and garbage collection around every 30 seconds (with default flags) when the app is just idle, and I don't think there's a good reason for that.

On Windows this behaviour is not present, of course. Now, I have zero experience with C++ code, so I probably shouldn't try to make a PR with critical code, but could someone take a look at it? Here's the example on SO of using poll on serial port and it's very close to what jSerialComm does:

JNIEXPORT jint JNICALL Java_com_fazecast_jSerialComm_SerialPort_waitForEvent(JNIEnv *env, jobject obj, jlong serialPortFD)
{
	// Initialize the waiting set
	if (serialPortFD <= 0)
		return 0;
	struct pollfd waitingSet = { serialPortFD, POLLIN, 0 };


	// Wait for a serial port event
	if (poll(&waitingSet, 1, 1000) <= 0)
		return 0;
	return (waitingSet.revents & POLLIN) ? com_fazecast_jSerialComm_SerialPort_LISTENING_EVENT_DATA_AVAILABLE : 0;
}

Then there's this blogpost with three different implementations. I just don't know how to connect all that to JNI and what to do with all the lines statuses.

[graynk]

Multithreaded applications
If a file descriptor being monitored by select() is closed in another
thread, the result is unspecified. On some UNIX systems, select()
unblocks and returns, with an indication that the file descriptor is
ready (a subsequent I/O operation will likely fail with an error,
unless another process reopens file descriptor between the time
select() returned and the I/O operation is performed). On Linux (and
some other systems), closing the file descriptor in another thread
has no effect on select(). In summary, any application that relies
on a particular behavior in this scenario must be considered buggy.

I'm beginning to see the problem.

[ZZerog]

PS: The event thread pooling every 100ns so in the small device use 100% of the CPU. Tested on a Onion Omega.

See LinuxEventThread

[graynk]

The only way of resolving indefinite blocking that I can think of is using pselect with self-pipe. We pselect on serial port and self-pipe. If serial port can be read, we read it as usual (or use read or something, I still don't quite understand how status lines work and what do we need them for). If nothing is there, but we need to interrupt (for example, application is closing and we need to close the port), we send something to self-pipe and pselect unblocks. Ideally, we could also use regular select and read with timeout for waitBytesWithTimeout instead of current method of sleeping in a loop. Why reinvent the wheel, when there's a system function for it (unless status lines are used for something very cryptic and we really do need them checked every 100 ns)?

[graynk]

So, I removed 100ns sleep and instead added these two lines (from jSerialComm with added explicit cast) to waitEvents:

struct pollfd waitingSet = { static_cast<int>(portHandle), POLLIN, 0 };
poll(&waitingSet, 1, 1000);

I left the rest of the code unchanged. So now it times out every second if nothing comes, checks if EventThread has been interrupted, then waits again. I didn't bother doing messaging between Java and JNI to interrupt indefinite poll, it works for me as is, since I close the port only in specific non-timebound circumstances, i.e. closing the app or passing the port to another app (avrdude).

It's interesting that readBytes already uses select, so I don't know what's the reason for not using it in waitEvents also.

Here's the relevant commit. Not sure if it's worth including here though, I still don't fully know what I'm doing.
graynk@9f8f07b

[dukenguyen]

I'm also seeing pretty high CPU usage even during idle. any chance we can get this merged and released?

[tresf]

any chance we can get this merged and released?

vs.

I still don't fully know what I'm doing.

I have zero experience with C++ code, so I probably shouldn't try to make a PR with critical code, but could someone take a look at it?

Probably best to wait for proper review, which no one's volunteered to do, so it'll wait until I hire someone, or someone volunteers. 🍻

[dukenguyen]

thanks all for the responses. Just to add some testing results info. I did build and test this on Debian 9 with ARM chipset with Octavo processor and did verify that the CPU dropped quite dramatically. Functionally, everything appears to be correct still when communicating over the serial ports. I will continue to monitor for a long time and report back if I see anything strange.

[tresf]

@graynk do you mind opening a PR?

[graynk]

@graynk do you mind opening a PR?

Sure, no problem. I'll take a look at the merge conflicts tomorrow, and open a PR once I resolve them

[tresf]

Reopening per @centic9 comments in #78 (comment).