-
Notifications
You must be signed in to change notification settings - Fork 606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DriverStation.waitForData This doesn't seem right #12
Comments
It looks right to me. It's the same way we do it in non-sim |
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. |
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();
}
}
} |
That certainly looks better. What does interrupting ourself do? And when might this situation occur? |
InterruptedException is a pain in the *** and not many people know how to handle it correctly. |
Ok. So since |
@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). |
Turns out this same thing exists in a few other places. Should I change those too? Like Preferences.java, CameraServer.java... |
I'm hitting them as I do the checkstyle refactor |
Makes the jar much easier to find when attempting to combine.
Co-authored-by: Zhiquan Yeo <zyeo8@bloomberg.net>
* FRC Update Suite * index fix for updatesuite
* Enable CI for more toolchains * lint
* FRC Update Suite * index fix for updatesuite
Added cpp version of sysid routine commands
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.The text was updated successfully, but these errors were encountered: