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

Future proofing terminology #294

Closed
tobie opened this issue Sep 29, 2017 · 3 comments · Fixed by #295
Closed

Future proofing terminology #294

tobie opened this issue Sep 29, 2017 · 3 comments · Fixed by #295

Comments

@tobie
Copy link
Member

tobie commented Sep 29, 2017

So following up on the terminology change of #290. Let me recap a couple of things. What you end up doing with: new FooSensor({ frequency: 100 }); is, I think we all agree, essentially two things:

  1. Set the frequency at which onreading is called (the reporting frequency), and
  2. indirectly, set the sampling frequency of the associated platform sensor. You're not setting it directly to 100 Hz, but you are modifying its settings so the UA is able to deliver on (1) to the best of the platform's capabilities.

Now let's say that we have a sensor that can be sampled at 1KHz. But collecting that data 1000 per seconds is not desired. Perhaps you only want to collect them in batch of 10 (obviously a level 2 feature, but please bear with me). Your API would look something like this:

new FooSensor({ frequency: 1000, groupBy: 10 });

And thus [desiredSamplingFrequency] would be 1000 and [reportingFrequency] would be 100, right? This seems to indicate that the frequency option is actually setting the [desiredSamplingFrequency] and not the [reportingFrequency], no?

Similarly, if you're doing:

let foo = new FooSensor({ frequency: 1000 });
requestAnimationFrame(function render() {
    draw(foo.value);
    requestAnimationFrame(render);
});

…as is common to improve latency (as demonstrated here). Then from a developer's perspective, you're not setting the reporting frequency (you're don't even have an event listener). No, here again you're setting the desired sampling frequency.

And going further, let's say your HW sensor doesn't support frequencies above 100 Hz, what will the reporting frequency be if the frequency option was set to 200Hz? Well the spec is pretty clear about this too:

"Any time a new sensor reading for a platform sensor is obtained and if the user agent can expose sensor readings, update latest reading is invoked with the platform sensor and the sensor reading as arguments."

If this really was "the frequency at which onreading is called", why would it be impacted by the underlying hardware?

All that to say that the reporting frequency is a function of the sampling frequency and not the opposite.

So while you may well get away with this terminology change right now (as the API is still very simple) it'll come back to haunt you the second you want to add features to the API. Sure, you can always revert to the previous terminology at that point, but that will create confusion with terminology in implementations, developer documentation, articles, etc.

Your best strategy here would be to refer to the frequency option as the way to set the desiredSamplingFrequency (with all of the caveats known—HW and platform limitations, etc). And have the reporting frequency be a function of that. As initially designed.

@pozdnyakov
Copy link

All that to say that the reporting frequency is a function of the sampling frequency and not the opposite.

Absolutely agree, and it is reflected in the abstract operations.

The problem as I see it now (and thanks for pointing it out!) is an unfortunate name and definition of the Sensor.requestedReportingFrequency internal slot. Now it might give a wrong impression that it is only to define reporting frequency , actually this slot stores the frequency option and its value is used for these two things:

  1. (indirectly) set the requested sampling frequency for the associated platform sensor.
  2. set the reporting frequency for the given Sensor object (in practice it sets the upper limit because reporting frequency is always capped to sampling frequency).

I will rename the slot to frequency (so that it reflects the ctor option name) and provide a better description to avoid this confusion.

pozdnyakov pushed a commit to pozdnyakov/sensors that referenced this issue Sep 29, 2017
The name of the Sensor.requestedReportingFrequency internal slot gave
a wrong impression that it was used only to define a reporting
frequency.

Actually, this slot stores the frequency option and its value is used
for these two things:
- (indirectly) set the requested sampling frequency for the associated
  platform sensor.
- set the reporting frequency for the given Sensor object (in practice
  it sets the upper limit because reporting frequency is always capped to
  sampling frequency).

Now the slot is renamed to `frequency` (so that it reflects the sensor
option name) to avoid this confusion.

Fixes w3c#294
@tobie
Copy link
Member Author

tobie commented Sep 29, 2017

Good to hear we're on the same page. I don't understand why you renamed [desiredSamplingFrequency] to [requestedReportingFrequency] in #290, then, as we both seem to agree it's the former term we actually want.

pozdnyakov pushed a commit to pozdnyakov/sensors that referenced this issue Sep 29, 2017
The name of the Sensor.requestedReportingFrequency internal slot gave
a wrong impression that it was used only to define a reporting
frequency.

Actually, this slot stores the frequency option and its value is used
for these two things:
- (indirectly) set the requested sampling frequency for the associated
  platform sensor.
- set the reporting frequency for the given Sensor object (in practice
  it sets the upper limit because reporting frequency is always capped to
  sampling frequency).

Now the slot is renamed to `frequency` (so that it reflects the sensor
option name) to avoid this confusion.

Fixes w3c#294
@pozdnyakov
Copy link

I don't understand why you renamed [desiredSamplingFrequency] to [requestedReportingFrequency]

I did not want it to be confused with the requested sampling frequency and I intended to underline that it pretty much defines the Sensor's reporting frequency. However, now I see that it is definitely not the best name for the slot.

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 a pull request may close this issue.

2 participants