fix(photon-lib): align C++/Python getTimestampSeconds on Java's capture-time semantic#2496
Conversation
…re-time semantic
C++ and Python compute getTimestampSeconds() as (receive - latency)
while Java reads metadata.captureTimestampMicros directly. The C++
path additionally pre-subtracts latency in PhotonCamera before
SetReceiveTimestamp, so GetTimestamp() subtracts latency a second
time — making every C++ vision timestamp off by ~one full processing
latency.
This change has all three ports read metadata.captureTimestampMicros
directly. Two properties of that field make this safe:
1. It is in the Time Sync Server's nt::Now timebase (FPGA-relative
on a real robot). At
photon-core/src/main/java/org/photonvision/common/dataflow/networktables/NTDataPublisher.java:200-211
the coprocessor reads getImageCaptureTimestampNanos(), adds the
TimeSyncClient offset, and writes the result to the wire — with
the in-line comment "Transform the metadata timestamps from the
local wpi::nt::Now timebase to the Time Sync Server's timebase".
2. Latency compensation is already baked in by the coprocessor.
photon-core/src/main/java/org/photonvision/vision/pipeline/CVPipeline.java:82
stamps frame.timestampNanos onto the result AFTER process()
returns, so the value is the moment of capture, not publish.
frame.timestampNanos itself is shutter-open time, set by the
driver at
photon-core/src/main/java/org/photonvision/vision/frame/provider/LibcameraGpuFrameProvider.java:95-101
before any processing happens. The direction is confirmed by
photon-core/src/main/java/org/photonvision/vision/pipeline/result/CVPipelineResult.java:112-114
where getLatencyMillis() returns (wpiNanoTime() - capture) — a
positive value, meaning capture precedes the latency reading.
ntReceiveTimestamp keeps its documented meaning (FPGA time bytes
arrived at robot code) for diagnostics. PhotonCamera no longer
pre-subtracts latency before SetReceiveTimestamp.
C++ tests previously relied on the (receive - 0_latency) projection
through GetTimestamp; updated to set captureTimestampMicros in
metadata directly, matching the Java test idiom.
github-actions
Bot
added
the
photonlib
…nit wrap wpiformat (clang-format with the WPILib config) reflowed two PhotonPipelineResult brace-initializers in this file. The CI Lint and Format job rejected the prior wrap in both ClosestToCameraHeightReturnsEmptyForNoFiducialTargets and ConstrainedPnpOneTag; this commit applies the formatter's chosen split. Pure whitespace; no semantic change.
| // ntReceiveTimestamp records when the bytes arrived at robot code, | ||
| // independent of capture time. GetTimestamp() reads capture time | ||
| // directly from metadata. | ||
| result.SetReceiveTimestamp(wpi::units::microsecond_t(value.time)); |
There was a problem hiding this comment.
Do we still need this function?
There was a problem hiding this comment.
No we shouldn't. I'll go through and clean it up. Probably a few more things as well that can be cleaned up.
There was a problem hiding this comment.
Good catch — audited the whole path and you're right, the field was write-only across C++ and Python (and Java never had the analog at all). Folded a cleanup into d323b97: deletes the field, the setter, the bespoke copy/move/operator= members that existed solely to propagate it across the proto-generated Base, the 14 dead test calls in PhotonPoseEstimatorTest, and the Python sim's matching +10 no-op offset. Net +2/−87.
…thon Responds to mcm001's review on PR PhotonVision#2496: with getTimestampSeconds() now reading metadata.captureTimestampMicros directly, the ntReceiveTimestamp fallback has no remaining consumers — across the entire repo, the field is written but never read. Java has never had the analog at all. Deleting it brings the three ports into alignment with Java as the reference. C++ (photon-targeting + photon-lib): - Remove ntReceiveTimestamp field, SetReceiveTimestamp setter, and the five bespoke copy/move/operator= members on PhotonPipelineResult whose sole purpose was propagating that field across the proto- generated Base. Defaulted special members are equivalent now that the only "extra" subclass field is gone. - Drop both SetReceiveTimestamp call sites in PhotonCamera.cpp, the now-unused 'now' local in GetLatestResult, and the <wpi/system/RobotController.hpp> include. - Strip 14 SetReceiveTimestamp lines from PhotonPoseEstimatorTest.cpp. Every test still sets metadata.captureTimestampMicros directly, so GetTimestamp() returns the expected value via the metadata path. - Drop the CopyResult test that existed solely to verify the bespoke copy ctor propagated ntReceiveTimestamp; defaulted base copy is exercised by every other test in the suite. Python (photon-lib/py): - Remove ntReceiveTimestampMicros field from the PhotonPipelineResult dataclass and the three assignments (two in PhotonCamera, one in the sim). The sim's "+10" microsecond offset was decorative; the value was never observed. - Drop the now-unused RobotController import in photonCamera.py and the _receiveLatencyMicros / receiveTimestampMicros() helper on PipelineTimestamps in test/testUtil.py.
The C++ port of CheckTimeSyncOrWarn writes the debounce comparison backwards: it warns when LESS than WARN_DEBOUNCE_SEC has elapsed since the last warning rather than MORE. Java has the same function with the correct sense at PhotonCamera.java:319. Practical effect: at 50 Hz NT packet rate with timeSinceLastPong > 5s, every call sets prevTimeSyncWarnTime to "now", so the next call ~20 ms later checks `now+0.02 < now+5` — always true — and fires another warning. Result is a 50 Hz warning storm on the rio console rather than the once-per-five-seconds debounce the surrounding code is clearly trying to implement. In a less common path (packets pause for >5 s, then resume with timesync still broken), the inverted check becomes false on resume and the warning goes permanently silent until timesync recovers and resets prevTimeSyncWarnTime to 0. Fix is the one character that brings C++ in line with Java: flip `<` to `>`. No semantic change to the alert (timesyncAlert.Set(true)) or the reset branch.
|
Adjacent fix folded in as 0382956: the C++ port of |
| * with a timestamp. | ||
| * time base (nt::Now). The robot shall run a server, so this is FPGA-relative | ||
| * on a real robot. Reads metadata.captureTimestampMicros directly — latency | ||
| * compensation is already baked in by the coprocessor. |
There was a problem hiding this comment.
Please match https://javadocs.photonvision.org/release/org/photonvision/targeting/PhotonPipelineResult.html#getTimestampSeconds() instead of this comment (is this an LLM)
There was a problem hiding this comment.
Yeah I asked it to update comments. Been using it for comments and commit messages to try and speed things up. I'll try to be more careful.
There was a problem hiding this comment.
To be clear -- our current posture on LLM use is that it's allowed, just that if you LLM write code to please call it out in the MR description so I know how/what to review for. LLMs make different mistakes from people and that changes how and what I review for, and it's helpful to know.
In this case my review comment is more about the comment talking about implementation details and not (IMO) adding value. Not that my old comment was very helpful :(
|
Can we roll #2497 into this MR as well? |
|
Yeah I haven't had a chance to test it on a real system yet though. |
…docs
The getCaptureTimestampMicros / getPublishTimestampMicros Javadocs
claim "coprocessor's time base", but NTDataPublisher applies the
TimeSyncClient offset to both values before publishing — the
on-wire value is in the Time Sync Server's nt::Now timebase, not
the coprocessor's local clock.
The field-level comment at PhotonPipelineMetadata.java:25-26 already
states the correct timebase ("wpi::nt::Now on the time sync server").
This commit aligns the getter Javadocs (which users see in IDE
autocomplete and online API docs) with that truth.
The Python per-field comment carried the same misleading prose;
updated to match.
No code or behavior change.
…stampMicros Javadoc Spotless (google-java-format) rejected the prior wrap because the first line fell under the 100-col target — its preferred split lands "time base" on line 1 and "real robot." on line 2. CI Java Formatting job failed on this file only. Pure whitespace reflow; identical wording.
|
@ |
|
Failing pipeline hm |
2 participants
Summary
C++
PhotonCamera::GetAllUnreadResultswritesvalue.time - latencyintontReceiveTimestamp, thenGetTimestamp()subtracts latency again — every C++ vision timestamp is off by ~one full processing latency (~16 ms @ 60 fps). Java and Python land in roughly the right place by different routes.This change has all three ports read
metadata.captureTimestampMicrosdirectly. Java already did; C++ and Python now match.Why reading the field directly is correct
The coprocessor already populates the field with the value the consumer wants:
TimeSyncClientoffset before publishing, with the in-line comment "Transform the metadata timestamps from the local wpi::nt::Now timebase to the Time Sync Server's timebase".frame.timestampNanos(shutter-open time, set by the driver at LibcameraGpuFrameProvider.java:95-101) onto the result afterprocess()returns. Direction confirmed by CVPipelineResult.java:112-114 wheregetLatencyMillis()returns(now - capture)— positive.ntReceiveTimestampkeeps its documented meaning (receive time) for diagnostics;PhotonCamerajust no longer pre-subtracts latency before storing it.Test plan
result.getTimestampSeconds()from a C++ poseest example is ~one frame closer toTimer.getFPGATimestamp()than beforeOut of scope
Mid-exposure correction (anchoring
captureTimestampMicrosto the middle of the integration window instead of shutter-open) — see #2489.