Call 'onchange' on a Senor instance considering its own frequency hint

[pozdnyakov]

The spec says: "onchange is an EventHandler which is called whenever a new reading is available."
Would it be better if onchange was called only when reading values (except timestamp) are actually changed? User most probably is interested in data fields changing, so calling onchange only on actual data change would reduce client code and save some cpu instructions.

[pozdnyakov]

@tobie PTAL

[tobie]

There's a difference between obtaining the same data at t0 and t1 (in terms of what this info confers about actual polling frequency, latency, etc.), so we can't just do away with it in some cases. We could however make this behavior opt in or opt out, though.

[pozdnyakov]

than, if we have two sensors: one with polling frequency 5Hz and another with 60Hz, and they both reference to the same shared reading instance. How often onchange should be called for the first sensor (having 5Hz)? will it still be 60 times per second when the actual reading is updated?

[tobie]

This gets very complicated very quickly, especially when you start getting sensors with multiple values, even more so if you include a primary/secondary distinction as we might want to (see #151).

If think specifying this more clearly is a really good thing and I'm happy (and impressed) you guys are willing to do such a thorough job of thinking this through.

The easy answer here is to follow what Android does (report at 5Hz no matter what), but I really want to include this in the broader reflection I'm having on this right now.

[pozdnyakov]

The way onchange is described now would fit more to an event handler called onreadingupdate or onnewreading and thus it is conflicting with the requirement of shared reading instance as supposed to be called simultaneously for all the sensor instances.

Can we just specify that onchange is called:

1. when the data is actually changed
2. not more often than the given frequency

[tobie]

The way onchange is described now would fit more to an event handler called onreadingupdate or onnewreading

I agree. But that's been argued against a number of times (and I lost these arguments). Terseness trumped correctness here.

and thus it is conflicting with the requirement of shared reading instance as supposed to be called simultaneously for all the sensor instances.

Yes, that's a good point. This wasn't an issue before, as I thought implementers were not going to be willing to send events are different frequencies to the different instances for perf reasons (and implementation complexity).

Can we just specify that onchange is called:

1. when the data is actually changed

Some use cases (notably motion sensors) might need data sent at regular intervals regardless of whether the sensor values changed, we'll need to dig into this a bit more.

2. not more often than the given frequency

That's the Android way mentioned above and might be the reasonable thing to do.

[rwaldron]

I agree. But that's been argued against a number of times (and I lost these arguments). Terseness trumped correctness here.

@tobie can you link to the issues/threads? In one of my original explanations, I defined both an "ondata" and "onchange" event: https://bugzilla.mozilla.org/show_bug.cgi?id=910150#c5.

• The "data" event (needs further definition).
  • The event must fire on the sensor instance object.
  • The event should fire on a constant, implementation independent interval, however the interval is recommended to match the current frame rate of requestAnimationFrame (this needs more attention).
• The "change" event (needs further definition).
  • The event must fire on the sensor instance object.
  • The event should fire whenever there is a significant change in the sensor value. The definition of "significant change" is when the sensor's value changes between two "data" events and the difference of that change is greater then then value specified by the "threshold" property.

I'm not arguing for those semantics—just that I think two events might be ideal (this has always been the design of all sensor classes in J5)

[tobie]

@tobie can you link to the issues/threads?

It's the result of a bunch of discussions, meetings, decisions in meetings, etc. Some rationales jotted down here: #78 (comment).

That said, I'm refactoring a bunch of things and open to reconsidering this in the process.

[rwaldron]

Originally, J5 had a "read" event, which matches your suggestion here. Eventually that was renamed "data", simply to match the rest of Node.js platform.

[foolip]

In fixing this, the precise timing of when data changes should also be defined. In https://codereview.chromium.org/2551223003/ there's a change to make the change itself and the event aligned with animation frames, and maybe that's what you want.

FWIW, it's not obvious to me that this is what you want. If you have a sensor that produces data faster than animation frames and that data is used to influence Web Audio, faster than 60 Hz could make sense.

[tobie]

@foolip wow. My understanding from raising this issue with blink folks a while back was that there was no way to sync the frame clocks with the sensor polling. So that's pretty exciting if it's solvable!

There's an open issue on this topic here: #4.

Note the main reason to request polling frequencies that are great than framerate are:

1. to decrease latency (pick the very latest values), so this means that polling must happen as close to the frame as possible, and
2. combining multiple values together on each frame.

It feels like this might need a different notation, e.g.:

new Gyroscope({ frequency: "2pf" });
// or
new Gyroscope({ readingsPerFrame: "2" });

Thoughts?

[foolip]

no way to sync the frame clocks with the sensor polling

I actually don't know about this. It's not hard to just delay the observable state change and the corresponding event to animation frame timing, but I don't know if that's all that's happening, or if one can also tickle the sensors into sampling and providing data really close to the animation frames. Will defer to @pozdnyakov on that, as long as observable behavior is well defined I'm happy :)

[tobie]

OK, so that's different, right?

The key reason for sampling > display rate is to reduce latency (so you only grab the latest value, but it's as close to the moment where you push pixels to the screen buffer as possible).

Being able to sync the polling with rAF would enable similar low latency but avoid polling the sensor so often (for reference, oculus rift polls the gyroscope at 1KHz, but discards all but the latest reading for each frame).

If the only thing we're doing, however, is delaying the task that updates the sensor object so it happens right before the new frame, we want to make sure we get the timestamps from when the sensor was polled and not from when the data shows up in JS.

[pozdnyakov]

Sensor polling and sending of 'onchange' event are different things (in Chromium even done in different processes): physical sensor is polled on platform side (and timestamp for reading is fetched from platform) considering the given frequency hint; updating of reading object on JS side and sending of 'onchane' event are synchronized with rAF (so can be deferred a bit) so that Critical Rendering Path is not interrupted and CPU and battery are not drained.

[foolip]

OK, that is what I initially guessed, that state changes and events are simply delayed to animation frame timing.

[tobie]

@foolip: that sounds like a non-normative perf improvement, now, no?

I could nevertheless add a note along the lines of:

Note: implementations may (slightly) delay update and/or event-triggering to improve performance and/or battery life. E.g. by synchronizing it with rAF.

WDYT?

[foolip]

It's an observable difference that web developers could have reason to care about, so can't the spec require a certain timing? (Otherwise, one might want to requestAnimationFrame in the event handler.)

The timing of certain other events is guaranteed by https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model and in whatwg/html#707 we're thinking about introducing an "animation frame task" concept.

Although it is tricky, it is possible to test the timing to some degree, see https://github.com/w3c/web-platform-tests/blob/master/fullscreen/api/element-request-fullscreen-timing-manual.html#L30

[tobie]

So I think specifying the processing model precisely and tying it to the event-loop makes a lot of sense for almost all use cases.

We can always provide an avoidFrameSync option down the line if such a request arises.

[pozdnyakov]

Think we must distinguish between polling frequency (one which is used by the actual device sensor to poll data) and update frequency (how often a JS Sensor object notifies its updates).
The user provides frequency hint only for update frequency, this hint also defines minimal desired interval between two updates.
The polling frequency will be chosen internally by UA. However, polling frequency affects the latency of the returned sensor data. So, it would be advisable to introduce also latency sensor construction parameter, so that UA will consider it when selecting the actual polling frequency. @tobie, wdyt?

[tobie]

We certainly need to be moving in a direction where we distinguish polling frequency from reporting frequency.

For low-level motion sensors, which are essentially going to be used as below (so without event handlers), I think we should make polling frequency settable and be more than a hint (but be clamped to HW values, obviously).

let gyro = new Gyro({ pollingFrequency: 120 });
function display() {
    requestAnimationFrame(function() {
        document.getElementById("foo").innerText = `x: ${gyro.x}, y: ${gyro.y}, z: ${gyro.z}`;
        display();
    });
}};

We'd also need sound default values for reporting polling frequency in such a case.

If we move towards supporting BYOB for motion sensors, we'll need to think of reporting frequency in terms of "your buffer is full and needs to be swapped."

I like how you're thinking about these two different frequencies once in term of update frequency and once in term of latency.

I'd probably argue for thinking about it differently (and I have use cases to back it up that I still need to write up), but I think we're converging here. :)

[kenchris]

^ @jimmy-huang now you are implementing sensor support for Zephyr, I know that you and Geoffrey had discussions regarding frequency and buffers. Could you please have a look at this issue?

[kenchris]

Basically the issue you have on some devices (like the Intel based Arduino 101), is that the actual sensor core sampling frequency might be faster than IPC can handle and thus, data needs to be buffered up and transferred in chunks.

[pozdnyakov]

Basically the issue you have on some devices (like the Intel based Arduino 101), is that the actual sensor core sampling frequency might be faster than IPC can handle and thus, data needs to be buffered up and transferred in chunks.

Why using a faster IPC (i.e. shared memory) is not an option?

[kenchris]

It might be, I don't know, but maybe that is not an option on the Arduino 101 on Zephyr. I don't think you can do shared memory between the x86 and ARC core. @jimmy-huang should be able to answer that better.

[jimmy-huang]

It's not the IPC that's the bottleneck per se, there's ringbuffer that we use that stores the callbacks since the sensor readings are coming in from an interrupt and we have to queue them up so it can be serviced in another context, and if the thread executing the JS application is not able to service them fast enough, the ringbuffer just fills up. The interrupt can be calling in like 1600Hz, where the JS engine might be only able to service the JS calls at 100Hz, and possibly slower if we are sending it out over BLE, as since each onchange() call, we have no control over how long the app returns control.

[pozdnyakov]

Is missing from this resolution is what I pointed out in #209 (comment). That is: does an update to the shared reading trigger each sensor to check if it's time for them to update their own client with a new reading, or does each sensor simply poll the shared reading at regular intervals?

The first, once shared reading is updated each of the Sensor instances either sends updates immediately or schedules update sending.

In the former, what happens if no new reading arrives in the shared buffer before the time interval of a given sensor elapses.

Nothing, in this case continue waiting for shared reading update and notify immediately

Please see #210

[tobie]

The first, once shared reading is updated each of the Sensor instances either sends updates immediately or schedules update sending

So this won't send the freshest data available, right?

I wonder if that's actually the behavior that developers want.

[pozdnyakov]

So this won't send the freshest data available, right?

mm.. why not? It will always send the freshest available data (i.e. freshest at the moment of sending).

[tobie]

mm.. why not? It will always send the freshest available data (i.e. freshest at the moment of sending).

I was referring to the scheduled update sending.

[pozdnyakov]

on scheduled update Sensor should do both 1) update its reading from shared reading and 2) fire onchange event

[tobie]

on scheduled update Sensor should do both 1) update its reading from shared reading and 2) fire onchange event

OK. So is the only point of the cache to make sure that sensor.x doesn't change if queried in between when events are fired?

[pozdnyakov]

OK. So is the only point of the cache to make sure that sensor.x doesn't change if queried in between when events are fired?

Not really, the point is that Sensor A is decoupled from Sensor B.
For example if you start a slow sensor { frequency : 1 } and listen to onchange from it you anticipate this onchange to be called once per second, later you start a fast sensor {frequency : 40 } but you still expect to receive notification from the slow sensor once per second. However, with the current ED you will suddenly get it 40 times per second, and then when fast sensor stops, you'll get it again once per second :)

[tobie]

Not really, the point is that Sensor A is decoupled from Sensor B.
For example if you start a slow sensor { frequency : 1 } and listen to onchange from it you anticipate this onchange to be called once per second, later you start a fast sensor {frequency : 40 } but you still expect to receive notification from the slow sensor once per second. However, with the current ED you will suddenly get it 40 times per second, and then when fast sensor stops, you'll get it again once per second :)

Right, but that's completely orthogonal to the existence of a per instance cache of the sensor reading. See, this is precisely why I don't like merging huge diffs in. ;)

I'm not saying the per instance cache might not be a desirable feature, but it's unrelated to what you describe above, which is just eventing mechanics.

[pozdnyakov]

to me more clear: slowSensor.x is not (always) equal to fastSensor.x.

Each object returns the cached values from the reading attributes, so that contained data is consistent with onchange notification: every Sensor object is self-contained

[tobie]

to me more clear: slowSensor.x is not (always) equal to fastSensor.x.

Well, yes. That's an effect of the caching. My question is: is this desirable, given (1) none of the other behavior we agree we want to add depend on this, and (2) === was a requirement (see #8).

[pozdnyakov]

and (2) === was a requirement (see #8).

Thanks for the links! This is exactly what I had asked for at #152 (comment) :)

Still I would like to hear people's opinion on whether it is OK if appearance of one object transparently modifies behavior of another object.

[tobie]

Thanks for the links! This is exactly what I had asked for at #152 (comment) :)

Oops, sorry. :-/

Still I would like to hear people's opinion on whether it is OK if appearance of one object transparently modifies behavior of another object.

What do you mean by behavior, here? Getting the value of attributes returning a fresher value or something else?

[pozdnyakov]

What do you mean by behavior, here? Getting the value of attributes returning a fresher value or something else?

yeah, that slowSensor.x could be updated at different frequencies at arbitrary periods of time depending on other Sensor instances activation.

Reading through #8, I found that the rational for returning the same values from all Sensor objects is
#8 (comment) : "In that way you will always be working with the freshest sensor data." - which makes sense from the user perspective (why would you need outdated data?)

Generic thoughts:

We do not have good options till we have multiple Sensor instances bound to the same platform sensor:

Option 1 (the current ED):
All Sensor objects point to the same data and 'onchange' is fired simultaneously on each object, ignoring individual frequency hint for "slower" sensors

Problems: basically described in this issue

• objects are not decoupled from each other
• frequency at which sensor.x is updated and at which change is fired may vary arbitrary, apart from the given hint.

Option 2 (#8 (comment)):

All Sensor objects point to the same data but the change event fires at different intervals on the different objects as specified in their respective initialization options.

Problems:

• objects are still not decoupled from each other
• frequency at which sensor.x is updated still may vary arbitrary
• onchange notification is not synced with returned data update

Option 3 (#210)
Sensor objects have their own reading copies

• Slower Sensors objects can have "outdated" reading

(In the above list I did not mention implementation complexity and memory&cpu resources required: it is increasing steadily from option 1 to option 3)

The only good option would be having 1-to-1 mapping between Sensor object and platform sensor.

Even though #8 and #9 are closed I propose to file a new dedicated issue and start working on it.
Meanwhile Chromium implementation can follow the option 1 which is currently in ED and easier to implement, hopefully not many users will create multiple Sensor instances of the same type :-/

[tobie]

I'm kind of confused why we would move back to option 1 after having had consensus to move to option 2 or option 3.

I initially spec'ed option 1 because my understanding was that it was (a) what was implemented in both Android and iOS and thus something developers would be used to (according to you, that's not the case), and (b) it would be a lot simpler and less resource hungry to implement.

The only good option would be having 1-to-1 mapping between Sensor object and platform sensor.

I'm no longer sure about that actually. It becomes especially complicated if we need both rAF-sync and non-rAF sync on the same page.

Decoupling the sensor object from sensor observers is something I suggested in the past already and that could make sense looking into again, but I don't think we'd be able to move to a singleton altogether.

[pozdnyakov]

I'm kind of confused why we would move back to option 1 after having had consensus to move to option 2 or option 3.

From #152 (comment) I understood that you were opposed to option 3 or did you mean something else?

[tobie]

I not sure how you've read this from that comment, tbh.

What I've been trying to do is get everyone on the same page with making a distinction between option 2 and 3 (as so far this had been conflated, notably in the pull requests you made).

I agree that there are other options we could/should explore (such as splitting sensor and observer into two different objects as mentioned above).

[alexshalamov]

@tobie @pozdnyakov I hope you don't mind hijacking discussion 😄 could everyone answer simple two questions:

• Is Sensor.reading === Sensor.reading still valid requirement and must be satisfied (even when instances have different frequency hint)?
• Is it acceptable that 'onchange' notification is out-of-sync with sensor reading modification that initiated 'onchange' event?

[pozdnyakov]

I not sure how you've read this from that comment, tbh.

I treated "(1) none of the other behavior we agree we want to add depend on this, and (2) === was a requirement (see #8)." as a kind of stopper for option 3. If it was not the case, I'm happy to proceed with option 3.

@alexshalamov I'd prefer sensor1.reading != sensor2.reading but than reading update and onchange events synchronized for each Sensor object.

[tobie]

@alexshalamov:

Is it acceptable that 'onchange' notification is out-of-sync with sensor reading modification that initiated 'onchange' event?

I don't think it would be, though. The new task both sets the value that sensor.x gets and fires the event synchronously. So in the event turn where the task runs, everything is in sync.

[tobie]

I not sure how you've read this from that comment, tbh.

I treated "(1) none of the other behavior we agree we want to add depend on this, and (2) === was a requirement (see #8)." as a kind of stopper for option 3. If it was not the case, I'm happy to proceed with option 3.

Sorry, I confused option 2 and 3 here (I thought they were ordered chronologically, not in order of increasing implementation cost and complexity).

So: option 2, we're good to do; option 3 we don't have consensus on (we might end-up doing so, but I'm not sure we want to do so yet).

That is:

per instance reporting frequency: yes.
per instance reading cache: no.

[pozdnyakov]

Ok, let's follow option 2 for now, I'll update #210 accordingly

[alexshalamov]

+1 for option (2), if we rename 'onchange' #205 to something suitable, e.g., onreading, ondata

[anssiko]

@tobie, PR #210 has been updated to reflect the consensus position, quoting it here for clarity:

Option 2

All Sensor objects point to the same data but the change event fires at different intervals on the different objects as specified in their respective initialization options.

Problems:

• objects are still not decoupled from each other
• frequency at which sensor.x is updated still may vary arbitrary
• onchange notification is not synced with returned data update

Could you review #210 and let us know if you have any concerns. Thanks!