Skip to content
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

Relax requirement on timestamp consistency checker #2931

Closed

Conversation

PeterBowman
Copy link
Member

Don't account for inconsistent timestamps (e.g due to disconnection or hardware error) as long as there is at least one subdevice still reporting current data. Use valid timestamps only for obtaining the average timestamp used as envelope in broadcast state messages.

Also, throttle the inconsistency error message to avoid flooding logs.

Don't account for inconsistent timestamps (e.g due to disconnection or
hardware error) as long as there is at least one subdevice still
reporting current data. Use valid timestamps only for obtaining the
average timestamp used as envelope in broadcast state messages.

Also, throttle the inconsistency error message to avoid flooding logs.
@PeterBowman PeterBowman temporarily deployed to code-analysis January 19, 2023 23:56 — with GitHub Actions Inactive
@PeterBowman
Copy link
Member Author

PeterBowman commented Jan 20, 2023

On a second thought, I might want to keep publishing state even if the last read happened a while ago (e.g. some or all subdevices are currently disconnected). The main question here is: what is the desired behavior if two nodes differ significantly in the timestamp of the last successful acquisition? How is the average time used as port envelope supposed to be obtained in this case?

Edit: I believe remotecontrolboard could benefit from a thorough review regarding timestamp handling. According to the contract of IPreciselyTimed, it is the last acquisition the one we are interested in, so perhaps the stale ones (corresponding to inactive subdevices) can be ignored. Besides, maybe the NWS should fill a new field of timestamps to be later read by getEncodersTimed and getMotorEncodersTimed in the NWC instead of a common averaged timestamp.

@PeterBowman
Copy link
Member Author

I have proposed a different approach at #2939.

@PeterBowman PeterBowman closed this Feb 1, 2023
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.

1 participant