-
Notifications
You must be signed in to change notification settings - Fork 0
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
Better PhotonCamera #132
base: main
Are you sure you want to change the base?
Better PhotonCamera #132
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
main comment about the thread and simulation. Otherwise looks good. I still think moving the Vision system into its own subsystem and then maybe centralizing the "state" for odometry and stuff like a few other teams have done may declutter things a little.
new Thread(() -> { | ||
Timer timer = new Timer(); | ||
while (true) { | ||
if (timer.advanceIfElapsed(10.0) && !isPhotonOk.get()) { | ||
try { | ||
uploadSettings(cameraIP + ":5800", | ||
new File(Filesystem.getDeployDirectory().getAbsoluteFile(), | ||
"photon-configs/" + cameraName + ".zip")); | ||
} catch (IOException e) { | ||
e.printStackTrace(); | ||
} | ||
} | ||
Thread.yield(); | ||
} | ||
}).start(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should only do this if in simulation mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably. Agreed on the separate "Vision" subsystem.
var res = inputs.result; | ||
if (res == null || inputs.timeSinceLastHeartbeat > 0.5) { | ||
var res = this.camera.getLatestResult(); | ||
if (res == null || Timer.getFPGATimestamp() - res.getTimestampSeconds() > 0.5) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
U should be able to use latency() > 0.5
since u have the same function below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think latency is actually different from this (calculated pi-side). I'll check, but I agree it should be consistent whatever we do here.
As we've seen with simulation stuff, not having an actual
PhotonCamera
is a big barrier for using much of PhotonVision's API. This update gets us roughly the same behavior as the oldPhotonIO
, but as an extension ofPhotonCamera
instead of a replacement. NowPhotonCameraWrapper
contains an instance ofPhotonCamera
that could be used for simulation.