w3c / sensors

Generic Sensor API
https://www.w3.org/TR/generic-sensor/
Other
127 stars 59 forks source link

Add solution for one-shot readings #88

Open tobie opened 8 years ago

tobie commented 8 years ago

Warning: all example code below is for illustrative purposes only, none of the below works or is spec'ed as is.

So here's the crux of the issue. We have the following requirements:

As we're essentially going for battery/CPU savings when calling the .read() and .readFromCache() methods, we certainly don't want to start observing the sensor in that case. So the examples above, while they look nice and nifty don't really show the full picture; we need a way to get only one reading without triggering multiple reads afterwards.

We have a number of solutions here: (UPDATE: the first solution was so clearly bad we removed it already.)

  1. We have an instance read() (and readFromCache()`) method. Calling it gets a single reading and immediately stops sensor polling:

    let sensor = new GeolocationSensor({ accuracy: "high" }); sensor.read().then(reading => findAddress(reading.coords));

    What happens in the following scenario, though?

    let sensor = new GeolocationSensor({ accuracy: "high" }); sensor.read().then(reading => findAddress(reading.coords)); sensor.onchange = event => displayMap(event.reading.coords);

    Is a single reading event emitted? None? Should setting onchange throw?

    Pros:

    • Both one shot and event based APIs look really nice when used as demoed.
    • The read() method can be added at a later stage though it will really feel bolted on.

    Cons:

    • Once read() has been called the sensor object is in a completely different state than it usually is, and that might not be obvious at all to someone using it. For example, one can start listening to the sensor after having called read() and won't receive any notifications.
    • Won't let you pass around sensor instances without triggering hardware activity (which means that a future discovery API might need to rely on lightweight sensor-like objects instead of just using those).
  2. Sensors do not start polling automatically when instantiated. We have instance startObservingactivate(), read() (and readFromCache()`) methods.

    To read once:

    let sensor = new GeolocationSensor({ accuracy: "high" });
    sensor.read().then(reading => findAddress(reading.coords));

    To react to sensor updates:

    let sensor = new GeolocationSensor({ accuracy: "high" });
    sensor.activate();
    sensor.onchange = event => displayMap(event.reading.coords);

    Pros:

    • There is no ambiguity. If startObservingactivate() hasn't been called, no events are fired.
    • Allows passing around sensor instances without triggering hardware activity (simplifies a future discovery API).

    Cons:

    • Event-based solution a bit more wordy than solution one above.
    • This needs to be thought out from the start or risk pretty serious backwards compatibility issues.
  3. While sensors still start automatically polling when instantiated, we have class methods for one-shot readings:

    GeolocationSensor.read({ accuracy: "high" }).then(reading => findAddress(reading.coords));

    Pros:

    • Both one shot and event-based APIs look rather nice.
    • The one-shot read() method can be added at a later stage without causing backwards compatibility issues.

    Cons:

    • Event-based solution a bit more wordy than solution one above.
    • Won't let you pass around sensor instances without triggering hardware activity (which means that a future discovery API might need to rely on lightweight sensor-like objects instead of just using those).
    • Feels sort of awkward when you have multiple sensors of a same type:

      ProximitySensor.read({
       position: "bottom-left",
       direction: "rear"
      }).then(/* ... */);

Which we can sort of summarize as follows:

Solution 1 Solution 2 Solution 3
One-shot API
s = new Sensor({…});
s.read().then…
s = new Sensor({…});
s.read().then…
Sensor.read({…}).then…
DX ☆ ☆ ☆ ☆ ☆ :star::star::star::star::star: :star::star::star:
Event-based API
s = new Sensor({…});
s.onchange = e => …
s = new Sensor({…});
s.activate();
s.onchange = e => …
s = new Sensor({…});
s.onchange = e => …
DX ☆ ☆ ☆ :star::star::star: :star::star::star::star::star:
Nasty side effects ☠ ☠ ☠ ☠ ☠ :star::star::star::star::star: :star::star::star::star::star:
Plays well w/ discovery API :star::star::star::star::star: :star:
Total ☆ ☆ ☠ :star::star::star::star::star: :star::star::star::star:

I'd love to hear's people's opinion here. I initially thought about punting decision around this until a V2, but realized that some of the solutions aren't really backwards compatible, so would rather figure out something now.

tobie commented 8 years ago

Would love input from all and in particular from the following: @annevk , @anssiko, @borismus, @domenic, @dontcallmedom, @fhirsch, @marcoscaceres, @mfoltzgoogle, @riju, @richtr, @rwaldron, @timvolodine.

rwaldron commented 8 years ago

IIRC Solution 3 is part of my original proposal? I still think that's the ideal path:

The API surface itself describes a clear demarcation between capabilities


RE: Solution 3, Con 1

Event-based solution a bit more wordy than solution one above.

How so?

let sensor = new Geolocation({ accuracy: "high" });
sensor.ondata = event => displayMap(event.data.coords);

That could be either Solution 1 or 3.

RE: Solution 3, Con 2

Won't let you pass around sensor instances without triggering hardware activity (which means that a future discovery API might need to rely on lightweight sensor-like objects instead of just using those).

If the processor and sensor hub are powered, then hardware is already active. Also, not all use cases will specifically register events in order to access the sensor data:

let sensor = new Sensor({ accuracy: "high" });

requestAnimationFrame(function frame() {
  console.log(sensor.values);
  requestAnimationFrame(frame);
});

Initialization (new Sensor(...)) should be the only "opt-in" necessary to create an active sensor.

RE: Solution 3, Con 3

Feels sort of awkward when you have multiple sensors of a same type

That doesn't look awkward to me, it looks clean and completely understandable. Also, I like that read(...) accepts similar options as the class constructor, that's valuable. What I would suggest is that you hold firmly on that design choice and let the built-ins work for you:

// ...
Promise.all([
  Proximity.read({ position: "bottom-left", direction: "rear" }),
  Proximity.read({ position: "top-middle", direction: "front" }),
]).then(proximities => {
  proximities.forEach(prox => {
    console.log(
      tags.stripIndent`
        Position: ${prox.position}
        Direction: ${prox.direction}
        Distance: ${prox.distance}cm
        ----------------------------
      `
    );
  })
});

Would result in something like...

Position: bottom-left
Direction: rear
Distance: 1cm
----------------------------
Position: top-middle
Direction: front
Distance: 12cm
----------------------------

(complete simulation here)


Re: Solution 2, (alleged) Pro 1

... If startObserving() hasn't been called, no events are fired.

I do not see this as a "pro". As I stated above, initialization should be the only "opt-in" for an active sensor. If there is a need for pause and resume (which there may very well be) then we should consider those, but separately from this issue.

anssiko commented 8 years ago

Good points from @rwaldron.

Just noting the MessagePort has this interesting (not very intuitive IMO) behaviour:

... when using addEventListener(), the start() method must also be invoked. When using onmessage, the call to start() is implied.

Applying that pattern to the solution 1 the call to start() would be implied with onchange. We'd need to start() when using addEventListener() though.

Seems more clear to require start() for both onchange and addEventListener('change', ...), thus the solutions 1 and 2 should actually be the same. I'd probably name the method simply start().

Also wondering if there are good use cases for explicit pause() and resume() or stop() and how would the solution 1+2 compare with 3 in that case.

tobie commented 8 years ago

Re @rwaldron's comment:

If the processor and sensor hub are powered, then hardware is already active.

Well, sensors aren't running all the time. They consume a lot more battery when they do. Thus a discovery API couldn't spawn lots of Sensor instances if those actually activated the underlying sensor on instantiation.

Also, not all use cases will specifically register events in order to access the sensor data:

Agreed. startObserving() might be the wrong name, here, maybe activate() is better (fixing above):

let sensor = new Sensor({ accuracy: "high" });
sensor.activate();
requestAnimationFrame(function frame() {
  console.log(sensor.reading);
  requestAnimationFrame(frame);
});

That doesn't look awkward to me, it looks clean and completely understandable. Also, I like that read(...) accepts similar options as the class constructor, that's valuable.

OK, I'm glad someone likes it. :) I've been of feeling conflicted about it because it works pretty well when you look at it (it's concise and reads well), but it combines two different concepts in the same method: 1) find the hardware sensor with these characteristics, and 2) read from it.

What I would suggest is that you hold firmly on that design choice and let the built-ins work for you.

I think that's irrelevant. The following would work just as well:

Promise.all([
  new Proximity({ position: "bottom-left", direction: "rear" }).read(),
  new Proximity({ position: "top-middle-left", direction: "front" }).read()
]).then(…);

Overall, you're clearly in favor of Solution 3 and don't even consider Solution 1 as worth discussing. Thanks for your input. It's super useful.

tobie commented 8 years ago

Re @anssiko's comment:

Just noting the MessagePort has this interesting (not very intuitive IMO) behaviour:

... when using addEventListener(), the start() method must also be invoked. When using onmessage, the call to start() is implied.

Applying that pattern to the solution 1 the call to start() would be implied with onchange. We'd need to start() when using addEventListener() though.

Well I was thinking about having, read() implicitly deactivate the polling (which is doable as both instantiation and call to read() would be in the same turn, with the polling steps queued-up for the next turn).

Definitely seems like a really bad idea. methods shouldn't have unexpected side effects.

Seems more clear to require start() for both onchange and addEventListener('change', ...), thus the solutions 1 and 2 should actually be the same.

Well, I guess we should just say Solution 1 is dreadful and kill it. :)

I'd probably name the method simply start().

I changed it to activate() already, but yeah, start() could be even better (provided we went down that path).

Also wondering if there are good use cases for explicit pause() and resume() or stop() and how would the solution 1+2 compare with 3 in that case.

pause and resume go well with both. Given the underlying implementation, there is no need to distinguish between stop and pause. So you'd probably want to go with: activate and deactivate for Solution 2 and pause and resume for Solution 3.

tobie commented 8 years ago

@rwaldron to be totally fair, I thought I came up with solution 3. So now that you pointed out it wasn't my idea, I've started to like it better. :)

rwaldron commented 8 years ago

... when using addEventListener(), the start() method must also be invoked. When using onmessage, the call to start() is implied.

I agree this is very unintuitive and we should absolutely avoid this pattern.

Seems more clear to require start() for both onchange and addEventListener('change', ...), thus the solutions 1 and 2 should actually be the same. I'd probably name the method simply start().

Not a start, because the sensor is born "started" by default. From there it may only pause or resume. I don't think these should have any effect on registering a handler and vice versa. If a program has called pause(), the only way to resume activity is to call resume.

Which brings me to...

Solution 4

One Shot

Geolocation.read({ accuracy: "high" }).then(reading => findAddress(reading.coords));

Continuous

let geo = new Geolocation({ accuracy: "high" }); // triggered an "Allow this app to use location"...

// Call pause() before adding an event handler...
geo.pause();
geo.ondata = event => displayMap(event.data.coords);

// ... or after...
geo.pause();

// ... It doesn't matter because nothing interesting can come from the geo sensor 
// until the next execution turn. Also, it doesn't matter if `ondata = ...` is used or 
// `addEventListener`, same behavior

I wanted to prove to myself that this was reasonably easy to use in a real thing, and that real benefit could be derived from it. Using @ryanflorence's https://github.com/ryanflorence/react-project, I modified Ryan's default placeholder to have a "Map" view (it's not really, as you'll see), and a "Home" view. The Map component should initialize a new, paused Geolocation sensor at runtime, and only resume it when the Map component is in view. I collected 4 lat/long points along a path down Atlantic avenue in Brooklyn, to use as data in a simulation. Here's a video of pause/resume semantics aligned with React component mounting and unmounting: https://youtu.be/2rU0xudkp5s

Here's the code for for the map component:

import React from "react";
import Geolocation from "./geolocation-simulator";
import { header } from "./styles.css";
import Title from "react-title-component";

let handlers = new Map();
let geo = new Geolocation({ frequency: 4 });
geo.pause();
geo.on("data", coords => {
  console.log(`Data Event; received: ${coords.latitude} ${coords.longitude}`);
  handlers.forEach(handler => handler(coords));
});

export default React.createClass({
  getInitialState() {
    return {
      latitude: null,
      longitude: null,
    };
  },
  componentDidMount() {
    geo.resume();
    handlers.set(this, coords => {
      this.setState({ ...coords });
    });
  },
  componentWillUnmount() {
    geo.pause();
    handlers.clear();
  },
  render() {
    return (
      <div>
        <Title render="Map"/>
        <h2 className={header}>Longitude & Latitude</h2>
        <ul>
          <li>Latitude: {this.state.latitude}</li>
          <li>Longitude: {this.state.longitude}</li>
        </ul>
      </div>
    )
  }
});

(Also, here)

Next is a similar thing, but this one only gets the coords once per "componentDidMount" and shows how nice the one-shot looks in action:

import React from "react";
import Geolocation from "./geolocation-simulator";
import { header } from "./styles.css";
import Title from "react-title-component";

export default React.createClass({
  getInitialState() {
    return {
      latitude: null,
      longitude: null,
    };
  },
  componentDidMount() {
    Geolocation.read().then(coords => {
      let {latitude, longitude} = coords;
      this.setState({ ...coords });
    });
  },
  render() {
    return (
      <div>
        <Title render="Map"/>
        <h2 className={header}>Longitude & Latitude</h2>
        <ul>
          <li>Latitude: {this.state.latitude}</li>
          <li>Longitude: {this.state.longitude}</li>
        </ul>
      </div>
    )
  }
});

Edit: Ignore the fact that I'm using EventEmitter, I just wanted to make the simulator mechanically functional

darobin commented 8 years ago

So, I might be missing something here but would it be wrong to have a sensor being active when it has event listeners and deactivated when it doesn't? The difference between listening continuously and asking just for one value:

let sensor = new GenericSensor();
// listen
sensor.on('whatever', val => console.log(val));
// just get one
sensor.once('whatever', val => console.log(val));

As for caching, the sensor instance can expose a cache event emitter. It's a simple (generic) interface that fires the appropriate event whenever its cache is set. The difference with the sensor is that it remembers the last event for every event type, such that when you register a listener you always get the latest value (but if there is none it will wait for one to happen).

// gets the latest read value
sensor.cache.once('whatever', val => console.log(val))l
tobie commented 8 years ago

re @rwaldron's comment:

Just for kicks changed your first example with a sensor born deactivated (as in iOS, for example) and that has activate/deactivate instead of resume/pause.

On second thought I see now how activate/deactivate might sort of imply hardware level actions, which isn't necessarily the case. Maybe start/stop is better. Or something else.

import React from "react";
import Geolocation from "./geolocation-simulator";
import { header } from "./styles.css";
import Title from "react-title-component";

let handlers = new Map();
let geo = new Geolocation({ frequency: 4 });
geo.on("data", coords => {
  console.log(`Data Event; received: ${coords.latitude} ${coords.longitude}`);
  handlers.forEach(handler => handler(coords));
});

export default React.createClass({
  getInitialState() {
    return {
      latitude: null,
      longitude: null,
    };
  },
  componentDidMount() {
    geo.activate(); // or geo.start();
    handlers.set(this, coords => {
      this.setState({ ...coords });
    });
  },
  componentWillUnmount() {
    geo.deactivate(); // or geo.stop();
    handlers.clear();
  },
  render() {
    return (
      <div>
        <Title render="Map"/>
        <h2 className={header}>Longitude & Latitude</h2>
        <ul>
          <li>Latitude: {this.state.latitude}</li>
          <li>Longitude: {this.state.longitude}</li>
        </ul>
      </div>
    )
  }
});
tobie commented 8 years ago

Re @darobin's comment:

So, I might be missing something here but would it be wrong to have a sensor being active when it has event listeners and deactivated when it doesn't?

Yeah, you want to be able to cater for the following use case:

let sensor = new Sensor({ accuracy: "high" });
requestAnimationFrame(function frame() {
  console.log(sensor.reading);
  requestAnimationFrame(frame);
});

The difference between listening continuously and asking just for one value:

let sensor = new GenericSensor();
// listen
sensor.on('whatever', val => console.log(val));
// just get one
sensor.once('whatever', val => console.log(val));

Ahem. You're spending too much time in environments which have more modern primitives than we do. We're still in EventTarget land here.

As for caching, the sensor instance can expose a cache event emitter. It's a simple (generic) interface that fires the appropriate event whenever its cache is set. The difference with the sensor is that it remembers the last event for every event type, such that when you register a listener you always get the latest value (but if there is none it will wait for one to happen).

The use case here is to hit the cache, but not necessarily hit the sensor if there is no cached values. For example, offer reverse geolocation to fill-in your current address, but not if your battery's super low and there is no cache.

rwaldron commented 8 years ago

Just for kicks changed your first example with a sensor born deactivated

Yeah, I can see that it improves this example (one less call in the code), but is this the most common case?

On second thought I see now how activate/deactivate might sort of imply hardware level actions, which isn't necessarily the case

I agree.

start/stop

Meh.

......

Currently, when this code is written:

window.addEventListener("devicemotion", event => {
  // ... it just starts emitting events. 
}, true);

...Nothing else tells it to start emitting those events

When a similar Android thing is made:

public class Main extends Activity implements SensorEventListener {
  private SensorManager manager;
  private Sensor accelerometer;

  @Override
  public void onCreate(Bundle savedState) {
    super.onCreate(savedState);
    manager = (SensorManager) getSystemService(Context.SENSOR_SERVICE);
    accelerometer = manager.getDefaultSensor(Sensor.TYPE_ACCELEROMETER);
    manager.registerListener(this, accelerometer, SensorManager.SENSOR_DELAY_NORMAL);
  }

  @Override
  public void onSensorChanged(SensorEvent event) {
    // this will just start happening
  }
}

...The author would have to write an explicit onPause and onResume that actually unregistered and registered the listener mechanism (yeeeeesh).

Anyway, I just learned how to write a crappy accelerometer output app with swift. Considering the amount of boilerplate crap involved in both the Android version and iOS version, I think whatever we choose will be better.

tobie commented 8 years ago

Considering the amount of boilerplate crap involved in both the Android version and iOS version, I think whatever we choose will be better.

That sums it up pretty well. :)

darobin commented 8 years ago

@tobie I've been out of the loop for a while, but is the rAF example realistic (and fundamental)? I'm presuming that it does not involve synchronous reading, so you're just getting whatever cached value there is.

requestAnimationFrame(function frame() {
  console.log(sensor.cache.lastValue);
  requestAnimationFrame(frame);
});

Regarding more advanced primitives, well, you gotta keep the pressure up :) Also: you can reproduce the same behaviour with EventTarget (albeit more verbosely).

Other than that, I wonder if you're not mixing the caching behaviour into the sensor too much? You have pretty granular caching needs, it strikes me as strange to have them built into the sensor itself. By splitting up the responsibility you can design both separately; you can also think about cases in which you would make the cache user-replaceable so that devs could fine-tune the caching behaviour they want.

annevk commented 8 years ago
domenic commented 8 years ago

I haven't looked through this thread in its entirety yet but one principle that I would in general like the web to follow is that constructors do not perform side-effecty work (like spinning up a hardware sensor), and that's left for initialization methods (like .start()). That's not very well-followed across the web platform today, but it makes me lean toward 2.

tobie commented 8 years ago

re @darobin's comment:

@tobie I've been out of the loop for a while, but is the rAF example realistic (and fundamental)?

Yeah, it seems like a common use case for all the device orientation stuff.

I'm presuming that it does not involve synchronous reading, so you're just getting whatever cached value there is.

Yup. Basically it's a convenience for doing something like:

var currentReading = null;
sensor.onchange = event => currentReading = event.reading;
requestAnimationFrame(function frame() {
    console.log(currentReading);
    requestAnimationFrame(frame);
});

Are you suggesting that by abandoning this feature altogether we could instead tie permission request plus starting to poll the sensor to adding a listener? I thought another key design principle of the platform was that listening to events shouldn't cause side effects. /cc @annevk

Regarding more advanced primitives, well, you gotta keep the pressure up :)

heh!

Also: you can reproduce the same behaviour with EventTarget (albeit more verbosely).

Well, that's a really interesting question. Afaik, there's no real way to tie the event loop and the sensor polling together, so I'm not sure you could actually prevent the sensor from being polled a second time.

Other than that, I wonder if you're not mixing the caching behaviour into the sensor too much?

Possibly, but some of it comes from requirements from concrete sensors (and have valid battery saving implications).

You have pretty granular caching needs,

Well, not really. You want access to the latest reading and eventually access to a previous reading if you don't need super fresh data (i.e. geolocation might be happy with a reading that's a few seconds old in some cases).

it strikes me as strange to have them built into the sensor itself.

That I buy for the current sensor reading. If you're talking about a data point that's seconds to minutes older, you want this to sit at a lower level (e.g. in the sensor hub itself), so as to be cross-origin and even cross-apps.

By splitting up the responsibility you can design both separately; you can also think about cases in which you would make the cache user-replaceable so that devs could fine-tune the caching behaviour they want.

I don't think this method actually prevents API consumers to bring their own cache. It's just a convenience for the most common case.

tobie commented 8 years ago

re @annevk's comment:

  • If you expose "once" as static, passing around instances is less useful. E.g., you might want all your code to use a particular sensor with some settings (accuracy: "high"). If you have to pass that separately in the constructor and the "once" static, reuse is a little harder.

This makes sense, though it also appears that you wouldn't be passing the permission request (which is inherently async) as part of this object, which I feel sort of lessens the overall value.

  • I think it does matter when you invoke pause(). The sensor is likely instantiated "in parallel" (see HTML Standard) so any long-lasting bit of script between instantiation and pause() could cause it to start doing things.

I imagined "in parallel" implied it would be queued at the end of the current event turn. Isn't that the case? (Genuine question.)

tobie commented 8 years ago

re @domenic's comment:

Thanks for your input. In such a case, I imagine start() would be async and would trigger the permission request (probably both returning a promise AND triggering an error event in case the permission was rejected).

annevk commented 8 years ago

No, "in parallel" can start straight away (and might be done by end-of-task, even reported through a microtask).

And yes, adding listeners should generally not have side effects: https://dom.spec.whatwg.org/#observing-event-listeners.

tobie commented 8 years ago

Thanks for the clarifications and pointer, @annevk.

marcoscaceres commented 8 years ago

I don't have much to add here, but I'm kinda liking Solution 4 from @rwaldron. So long as each request to the one-shot static method (i.e., Geolocation.read(options)) "promises" to respond as per its options, and other subsequent synchronous calls to it don't affect previous callers, then it would be really nice.

I'd also like to echo the calls for no side effects, so would be strongly against .addEventListener() starting the sensor.