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

data delivery: Set timestamp as an "unsafe shared current time" #280

Merged
merged 2 commits into from
Jun 13, 2024

Conversation

rakuco
Copy link
Member

@rakuco rakuco commented Jun 12, 2024

Follow-up to #274, related to #257.

The wording used in #274, which mentions platform-specific timestamps, made more sense in the context of the Generic Sensor API spec, as multiple platform-specific sensor APIs provide samples with timestamps in a platform-specific format that needs to be converted.

Of the OS-provided telemetry APIs, however, only Windows optionally provides samples with a timestamp. As such, it makes more sense to define a timestamp using the monotonic clock's unsafe current time instead. Aditionally, the accompanying note was rewritten to indicate that the same value should be used for all globals, otherwise the same sample would end up with different "raw" timestamps in different frames/workers.


Preview | Diff

Follow-up to #274, related to #257.

The wording used in #274, which mentions platform-specific timestamps, made
more sense in the context of the Generic Sensor API spec, as multiple
platform-specific sensor APIs provide samples with timestamps in a
platform-specific format that needs to be converted.

Of the OS-provided telemetry APIs, however, only Windows optionally provides
samples with a timestamp. As such, it makes more sense to define a timestamp
using the monotonic clock's unsafe current time instead. Aditionally, the
accompanying note was rewritten to indicate that the same value should be
used for all globals, otherwise the same sample would end up with different
"raw" timestamps in different frames/workers.
@rakuco rakuco requested a review from arskama June 12, 2024 15:09
@rakuco
Copy link
Member Author

rakuco commented Jun 12, 2024

@arskama sorry for flip-flopping on the contents here; this only occurred to me when I tried to integrate your changes from #274 into #265.

rakuco pushed a commit to kenchris/compute-pressure that referenced this pull request Jun 12, 2024
…ases

With platform collectors now being per-global and per-source type, we need a
way to clearly indicate that the "unprocessed" timestamp from a sample (i.e.
what is fed to "relative high resolution time") must be the same across all
globals.

We also need to make sure that this works for both real and virtual pressure
sources.

- The abstract "pressure source" concept added a few commits ago now has an
  accompanying "pressure source sample" concept. It is a struct holding
  implementation-defined telemetry data as well as a timestamp corresponding
  to when it was obtained.
  Functionally, this should be identical to the what the spec has as of pull
  request w3c#280.

- Virtual pressure source's "pending samples" is now a struct as well,
  holding similar data: a PressureState (like before) and a timestamp that
  is set to the "unshared current time" when the "update virtual pressure
  source" endpoint is called.

- The "data collection" algorithm is responsible for reading either a
  "pressure source sample" or a virtual pressure source's "pending sample"
  and processing the timestamp value as necessary.
@arskama
Copy link
Contributor

arskama commented Jun 13, 2024

LGTM. It makes sense! Thanks for fixing it.
I also had doubts about the wording, I should have challenged more myself and you on the meaning of this sentence.

@rakuco rakuco merged commit 3a6fa23 into main Jun 13, 2024
2 checks passed
@rakuco rakuco deleted the data-delivery-timestamp-calculation branch June 13, 2024 07:42
Copy link
Contributor

@kenchris kenchris left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants