DriverStation.waitForData This doesn't seem right

[JLLeitschuh]

I'm not 100% sure what the original author intended for this method to be but I'm fairly confident that it doesn't do what it's documented to do.

https://github.com/wpilibsuite/allwpilib/blob/master/wpilibj/src/sim/java/edu/wpi/first/wpilibj/DriverStation.java#L135

Also, its missing the Thread.curentThread().interrupt() to reset the interrupt flag.

[PeterMitrano]

It looks right to me. It's the same way we do it in non-sim

[ThadHouse]

That is the right way to do it. We usually use a timeout of 0 anyway, which forces a wait for GetData to get called and ran. But I think I tested it last summer and the timeouts work as well.

[JLLeitschuh]

Regardless of that, you are totally throwing away the interrupted flag.

It should be:

    public void waitForData(long timeout) {
        synchronized (m_dataSem) {
            try {
                m_dataSem.wait(timeout);
            } catch (InterruptedException ex) {
                Thread.currentThread().interrupt();
            }
        }
    }

[PeterMitrano]

That certainly looks better. What does interrupting ourself do? And when might this situation occur?

[JLLeitschuh]

InterruptedException is a pain in the *\ and not many people know how to handle it correctly. http://www.yegor256.com/2015/10/20/interrupted-exception.html http://www.ibm.com/developerworks/library/j-jtp05236/

[PeterMitrano]

Ok. So since wait takes a long time we should be calling interrupt. But then we want to go back and continue the wait next time around, so what @JLLeitschuh suggested seems appropriate.

[JLLeitschuh]

@PeterMitrano catch is only called if the current thread is interrupted, ie. the the exception has been thrown. That bit of code will normally never run.

[PeterMitrano]

Yes, I'm aware. I'm just thinking through what would happen if an exception was thrown. I'll make a PR for that one line (in both places. java, and sim java).

[PeterMitrano]

Turns out this same thing exists in a few other places. Should I change those too? Like Preferences.java, CameraServer.java...

[JLLeitschuh]

I'm hitting them as I do the checkstyle refactor