Skip to content

Conversation

@agasser
Copy link
Contributor

@agasser agasser commented Oct 17, 2022

Timer.getFPGATimesptamp() returns the current time in seconds, but res.getLatencyMillis() is in miliseconds.

Fixes #498

`Timer.getFPGATimesptamp()` returns the current time in _seconds_, but `res.getLatencyMillis()` is in _miliseconds_.
@agasser agasser requested a review from a team as a code owner October 17, 2022 00:47
@shueja
Copy link
Contributor

shueja commented Oct 17, 2022

How does this commit fix the issue? Don't you need to multiply one or divide the other by 1000?

@agasser
Copy link
Contributor Author

agasser commented Oct 17, 2022

Yikes, I don't know how I managed to get that wrong. It's fixed now.

Copy link
Contributor

@gerth2 gerth2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup

@gerth2
Copy link
Contributor

gerth2 commented Oct 17, 2022

Nitpick - d is possibly going to cause some lack of resolution if millis ever returns a fraction. And also might be confusing to a newbie not familiar with the syntax (since that line shows up with the docs). But I don't think it's worth blocking a merge till we know there's a real issue.

@agasser
Copy link
Contributor Author

agasser commented Oct 17, 2022

Nitpick - d is possibly going to cause some lack of resolution if millis ever returns a fraction.

Can you explain?

I'm fine just using an integer or using 1000.0 if its easier to read.

@gerth2
Copy link
Contributor

gerth2 commented Oct 17, 2022

1000.0 would be my preference - simply because it still reads well, and future-proofs against keeping things floating point. Which, as far as I know, is generally "best practice" for FRC hardware and purposes.

@shueja shueja merged commit d1bfb86 into PhotonVision:master Oct 17, 2022
@agasser agasser deleted the patch-1 branch January 15, 2023 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

simposeest imageCaptureTime incorrect

3 participants