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

[vehicle-spec] Refactoring access methods for EventHandlers and subscribing to functions with multiple attributes #72

Closed
acrofts84 opened this issue Nov 2, 2015 · 54 comments

Comments

@acrofts84
Copy link
Contributor

Hi Everyone,

  1. In the current spec we allow for subscriptions to an interface and not to an attribute within an interface. For example, the trip interface contains multiple attributes:
interface Trip {
    readonly    attribute unsigned long   distance;
    readonly    attribute unsigned short? averageSpeed;
    readonly    attribute unsigned short? fuelConsumption;
};

// does not allow distinction between distance, averageSpeed, fuelConsumption
var tripInterface = vehicle.trip.subscribe(callbackFunction, desiredZone, period);
  1. The main criticism received from the Generic Sensor API WG was that the current spec was "not very web", and they suggested implementing a DOM Events style, as per their API, in order to make the spec more usable and intuitive to a web developer. I think it is a valid point, but maybe someone with more web development experience could advise.

In order to alleviate both of these issues, we could use the following:

interface VehicleSignalInterface : VehicleInterface {
    Promise        set (object value);
    attribute EventHandler onchange;
    attribute EventHandler onerror;
    attribute EventHandler ondata;
    attribute DOMString attributeName;
    attribute unsigned long? period;
    attribute Zone? zone;
};

//this could then be implemented as:
var averageSpeedSubscription = vehicle.trip
averageSpeedSubscription.attributeName="averageSpeed"
averageSpeedSubscription.onchange = function(){ 
    //do something
};
averageSpeedSubscription.onerror = function(error){ 
    //flag error
};
averageSpeedSubscription.period=100;
//if zones where applicable
averageSpeedSubscription.zone="left"

var distanceSubscription = vehicle.trip
distanceSubscription.attributeName="distance"
distanceSubscription.onchange= function(){ 
    //do something else
};
distanceSubscription.period=100;

The unsubscribe function would then be obsolete.

Let me know if there are any glaring errors or disadvantages to this approach. If not, I'm happy to make a full proposal for review.

Thanks,

Adam

@aShinjiroUrata
Copy link
Contributor

Hi Adam-san,

Thanks for the proposal!
I have a question.

//this could then be implemented as:
var averageSpeedSubscription = vehicle.trip
averageSpeedSubscription.attributeName="averageSpeed"
averageSpeedSubscription.onchange = function(){ 
    //do something
};
....
var distanceSubscription = vehicle.trip
distanceSubscription.attributeName="distance"
distanceSubscription.onchange= function(){ 
    //do something else
};

This looks averageSpeedSubscription and distanceSubscription are
pointing the same object and attribetName is set as "averageSpeed" first
and then overwritten by "distance".
I think, once overwritten to "distance", callback is not invoked by averageSpeed change any more,
my understanding correct?

My concern is how to watch "averageSpeed" and "distance" (and maybe fuelConsumption, etc)
concurrently.

Thanks.

@acrofts84
Copy link
Contributor Author

Hi Urata-san,

Thank you for your feedback. I agree with your point. Maybe the following would be a better solution:

//this could then be implemented as:
var averageSpeedSubscription = new Trip()
averageSpeedSubscription.attributeName="averageSpeed"
averageSpeedSubscription.onchange = function(){ 
    //do something
};
....
var distanceSubscription = new Trip()
distanceSubscription.attributeName="distance"
distanceSubscription.onchange= function(){ 
    //do something else
};

The ideal however would be a middle ground where we can use a single interface to subscribe to only what we want within that interface. I'm open to suggestions for this.

@tobie
Copy link
Member

tobie commented Nov 5, 2015

Depending on what your constraints are (which might be documented somewhere, I must admit not having searched thoroughly), you could approach this problem through different means.

For example:

  1. Creating dedicated event handlers for each properties:

    interface VehicleSignal : Vehicle {
      Promise set (object value);
      attribute EventHandler onerror;
      attribute Zone? zone;
    };
    
    interface Trip : VehicleSignal {
      attribute EventHandler ondistancechange;
      attribute EventHandler onaveragespeedchange;
      attribute EventHandler onfuelconsumptionchange;
    };

    Which would let you write code like:

    vehicle.trip.onaveragespeedchange = function(e) {
        // do stuff.
    };
  2. creating dedicated generic observers:

    [Constructor(DOMString type, Zone zone), Exposed=(Window,Worker)]
    interface VehicleSignal : Vehicle {
      Promise set (object value);
      attribute EventHandler ondata;
      attribute EventHandler onchange;
      attribute EventHandler onerror;
      attribute DOMString type; // or Enum
      attribute Zone zone;
    };
    
    interface Trip {
      attribute VehicleSignal distance;
      attribute VehicleSignal averageSpeed;
      attribute VehicleSignal fuelConsumption;
    };

    Which would let you write code like:

    vehicle.trip.averageSpeed.onchange = function(e) {
        // do stuff.
    };
  3. creating dedicated specific observers:

    [Constructor(Zone zone), Exposed=(Window,Worker)]
    interface VehicleSignal : Vehicle {
      attribute EventHandler onerror;
      attribute Zone zone;
    };
    
    interface VehicleDistanceSignal : VehicleSignal {
      // specific stuff
    };
    
    interface VehicleAverageSpeedSignal : VehicleSignal {
      // specific stuff
      EventHandler onaveragespeedchange
    };
    
    interface VehicleFuelConsumptionSignal : VehicleSignal {
      // specific stuff
    };
    
    interface Trip {
      attribute VehicleDistanceSignal distance;
      attribute VehicleAverageSpeedSignal averageSpeed;
      attribute VehicleFuelConsumptionSignal fuelConsumption;
    };

    Which would let you write code like:

    vehicle.trip.averageSpeed.onaveragespeedchange = function(e) {
        // do stuff.
    };

I believe that the last solution is what you'll end up ultimately gravitating towards. The generalizations you're trying to make to go for solution 2 will end up creating lots of edge cases you'll have to handle in prose and will create a poor overall developer experience. Solution 1 could be an interesting proposal, but I'm concerned it will make for an overall more complex and less flexible spec.

As a side note, you want to avoid suffixing your interfaces with "Interface" as those will appear in the concrete implementations if you do.

@acrofts84 acrofts84 changed the title [vehicle-spec] Subscribing to functions with multiple attributes [vehicle-spec] Refactoring access methods for EventHandlers and subscribing to functions with multiple attributes Nov 6, 2015
@aShinjiroUrata
Copy link
Contributor

I'm sorry for the delay.

Adam-san
Thanks for the answer !
It is good to me.

Tobie-san,
Thank you very much for the proposal !

If we need to change the API style, I think better to do it sooner,
otherwise the problem will get bigger in the future.
Also, I fully agree to change the style so that web developers feel natural and comfortable.

By the way, as I'm not JavaScript expert and not watching W3C's api style trend
closely, I think I'm not fully understanding the reason of this change proposal.
(Actually, I feel the current get/subscribe style is not so unnatural since
geolocation API is using similar style.)

So, if you could summarize, what is not good about current get/subscribe style
and what is good on the proposed style, it would be great help for me.
Moreover, I suppose some of the members might be interested in this point.

Since this is big change for Vehicle API style, I would like to have sufficient understanding.

I appreciate if you could answer.

@tobie
Copy link
Member

tobie commented Nov 9, 2015

Actually, I feel the current get/subscribe style is not so unnatural since geolocation API is using similar style.

The geolocation API is pretty much universally considered by developers to be an abomination, so I would strongly suggest not modeling your API after it. ;)

@aShinjiroUrata
Copy link
Contributor

Thanks for the comment!

Actually I wanted to know the reason for

The geolocation API is pretty much universally considered by developers to be an abomination

It seems the reason is the one written in 'Web API Cookbook 4.2 Specifying Callbacks' as below, right?

Downsides of this approach are that it is a little more verbose and does not encourage error handling. It also lacks in extensibility since adding new arguments to the method will typically require placing them after the optional error handler — making them de facto optional. Likewise, extending it to support not just callbacks for success and error but also for other situations that may warrant reporting information back (e.g. progress being made) is clumsy at best. Since it is only used in [GEOLOCATION-API] it does not appear to be a required pattern for consistency.

Then I understand this is the style we should not follow.

@tobie
Copy link
Member

tobie commented Nov 9, 2015

Well, there are multiple reasons why the Geolocation API design is bad. First of all it doesn't use the platform's event system (EventTarget) but comes up with its own abstraction, which makes it inconsistant with the rest of the platform. Secondly, it uses callbacks instead of promises for the one-shot case (this is more of a legacy issue, so the spec can't really be blamed for that, but copying that design is a bad idea). Thirdly, it has some extremely circumvented ways of carrying out some common tasks (like requesting data from the cache; see the fourth example here: http://www.w3.org/TR/geolocation-API/#introduction). There are additional issues such as the impossibility of setting the accuracy needed, the threshold which triggers new events or the frequency at which the position is measured. These matter greatly when you're trying to balance battery life with accurate position display and can only be determined at the application level (for example, am I using this data give the high-level view of a car along a 400km long itinerary or am I pinpointing a walking user at the maximum zoom level of a map). These are the kinds of questions that the Generic Sensor API is trying to answer.

As a side note, I don't recommend basing your design on the Web API Cookbook as it hasn't been updated in three years and doesn't mention promises (now a key part of one-shot async methods), nor any of the existing ES6 or upcoming ES7 syntax. Unfortunately, I don't have a good replacement document you could use to base your API design on instead. Read specs written by good editors, read the Extensible Web Manifesto, read the HTML design principles (old but mostly still valid), Have a look at @kriskowval's excellent General Theory of Reactivity for the async part. It would be awesome to have a comprehensive guide as to how to write good specs for the Web platform, but there's no such thing for now.

@aShinjiroUrata
Copy link
Contributor

Tobie-san, thanks for the detailed answer.
I could understand very well this time.

There are additional issues such as the impossibility of setting the accuracy needed, the threshold >which triggers new events or the frequency at which the position is measured.

We had similar discussion about threshold and frequency at #29 and at that time we just added optional period because we would like to keep the interface simple.

subscribe (callback, optional zone, optional period);

Thanks again.

@djensen47
Copy link
Contributor

I generally agree with @tobie, if the W3C specs are out of date, we shouldn't base our API on outdated ideas. Given that ES6 and maybe even ES7 will be prolific in browsers by the time this API is in cars, we should consider modern approaches to the API design. This was part of me motivation for Issue #37.

@tobie I think you meant @kriskowal (no v in the username). Kris drops by my office every so often so I can try to see if he has some time to provide some general or specific feedback about API design.

@tobie
Copy link
Member

tobie commented Nov 24, 2015

Yes. Apologies to Kris for misspelling, again, his name.

@acrofts84
Copy link
Contributor Author

Hi Everyone,
Thank you for your discussion. Here’s my quick summary of the options:

  • Dedicated handlers for each property
    • vehicle.trip.onaveragespeedchange
    • I don’t believe this will work in the case of setting a specific signal. You would have to have separate attribute for set and onchange which would get confusing.
  • Generic observers
    • vehicle.trip.averageSpeed.onchange
    • May create lots of edge cases
  • Specific observers
    • vehicle.trip.averageSpeed.onaveragespeedchange
    • Dramatically increase the quantity of APIs on vehicle
  • Existing pub sub
    • Not considered web friendly
    • Inconsistent with other web technologies

Is extra flexibility worth the significantly larger API in comparing option 2 and 3? We had a similar conversation in a discussion on #37 and decided against it. Apologies for my inexperience, but I can’t think what the edge cases would be that could not be implemented with the above? Are the edge cases referring to where we require thresholds and a specific signal granularity, as this could be included in a generic observer as in the generic sensor API? As you can see tell I’m edging toward to second option in order to keep the spec simple.

Promises are one of the big advantages of the API as it stands. We will lose this potential in the EventHandler case for one time value calls, so do we require a getter anyway to provide this functionality? Then the EventHandlers could be used in lieu of, or as an enhancement to, our subscription model?

@peterMelco
Copy link
Contributor

Hi, so we have the following interface definition according to (2):

Sorry, for a stupid question I am looking into doing a sample implementation and I have gotten so far as to add idl files, build the and generate bindings and having the code executed. Now I am looking to do a small test implementation with our latest thoughts on the API. However looking at suggestion (2) above:

[Constructor(DOMString type, Zone zone), Exposed=(Window,Worker)]
interface VehicleSignal : Vehicle {
  Promise set (object value);
  attribute EventHandler ondata;
  attribute EventHandler onchange;
  attribute EventHandler onerror;
  attribute DOMString type; // or Enum
  attribute Zone zone;
};
interface Trip {
**readonly attribute unsigned long distancevalue**
**readonly    attribute unsigned short? averageSpeedvalue;**
 **readonly    attribute unsigned short? fuelConsumptionvalue;**

  attribute VehicleSignal distance;
  attribute VehicleSignal averageSpeed;
  attribute VehicleSignal fuelConsumption;
};

How is the actual data defined ? or rather where ? Inside Trip , like above ?? and get:

vehicle.trip.averageSpeed.onchange = function(e) {
   updatedspeed = e.averageSpeedvalue;
}

Or how is the data retrieved inside the onchange method ? and how can we ensure that the set method will always be applicable ?

@peterMelco
Copy link
Contributor

One more note: I agree with Tobie-san (@tobie) that we should avoid suffixing interfaces with "Interface".

@aShinjiroUrata
Copy link
Contributor

Excuse me for very late response.
For @acrofts84 Dec,2 comment,

Apologies for my inexperience, but I can’t think what the edge cases would be that could not be implemented with the above? Are the edge cases referring to where we require thresholds and a specific signal granularity, as this could be included in a generic observer as in the generic sensor API?

I can't imagine well about edge cases. I'd like to know if there is some example.

As you can see tell I’m edging toward to second option in order to keep the spec simple.

I feel same since in the 'Specifc Observer', we would have too many API definition.
(I guess If there's strong reason, we may have to ignore 'too many' and select 'Specific Observer'. )

Then the EventHandlers could be used in lieu of, or as an enhancement to, our subscription model?

My understanding of Tobie-san's suggetions is like, the EventHandlers are replacement of 'subscribe()' and the Promises style 'get()' is still living. This is because we can find Promises style 'requestReading()' API in Generic Sensor API definition which is basically similar to 'get()'.

@aShinjiroUrata
Copy link
Contributor

This may be too detailed but, in the beginning comment

var tripInterface = vehicle.trip.subscribe(callbackFunction, desiredZone, period);

in the existing vehicle API definition, I think, this should be like
(I know Adam-san take this as just an example)

vehicle.tripMeters.subscribe( function( tripmeters ) {
    console.log( tripmeters.meters[0].distance );
    console.log( tripmeters.meters[1].averageSpeed );    
}, desiredZone, period);
# meters[] = array of Trip interface

http://rawgit.com/w3c/automotive/master/vehicle_data/data_spec.html#trip-interface
I guess, this "6.9 Trip Interface" should be "6.9 TripMeters Interface", isn't it?
(All other interfaces of section tile are subclass of VehicleCommonDataType)
This looked confusing to me.

One more thing is this TripMeters interface looks like special one, I mean,
only TripMeter interface takes array of another interface ('Trip') as its attribute
and this is confusing too.

I guess, with other interface, discussion might become bit easier.

With vehicleSpeed

vehicle.vehicleSpeed.speed.onchange = function(vehicleSpeed) {
 or
vehicle.vehicleSpeed.onchange = function(vehicleSpeed) {
    console.log( vehicleSpeed.speed );  //vehicleSpeed = vehicleSpeed interface
}

With fuel

vehicle.fuel.onchange = function(fuel) {   //fuel = fuel interface
    console.log(fuel.level);
    console.log(fuel.averageConsumption);
}
  or
vehicle.fuel.level.onchange = function(fuelLevel) { 
    console.log(fuelLevel.value);
}
# I think we need definition like
interface fuelLevel : VehicleCommnDataType

vehicle.fuel.averageConsumption.onchange = function(fuelAverageConsumption) {
    console.log(fuelAveConsumption.value);
}
# I think we need
interface fuelAverageConsumption : VehicleCommnDataType

@drkevg
Copy link
Contributor

drkevg commented Jan 5, 2016

I have a 'couple' of concerns re. this proposed change and would be very interested to hear other responses/views.

The first concern relates to 'How big a change in a value is considered to be a 'real' change' - prompting the valid generation of an onchange notification compared to just noise in the sensor measurements.

If we imagine a vehicle travelling at e.g. 70km/h (70000 meters per hour), one implementation might send an onchange notification if speed changes by 1 meter per hour, whilst another implementation might only send a notification if speed changes by 10 m/h. So in one case, the onchange event will be called much more frequently than the other.

Second concern is that the client app could easily become overwhelmed as it cannot control how often onchange events are triggered and cannot predict how often a particular implementation will send them.

In the worst case, a client application that registers onchange for a number of different signals could become overwhelmed by frequently changing signals (that are relatively unimportant) causing it to miss or delay responding to changes to signals that are more important.

It seems that the existing pub/sub approach where the client app subscribes to receive a notification when object state changes is preferable because:

a) It is less 'chatty' as it requires fewer notifications (because related data is grouped into a single object, as opposed to a notification being required for each data member/signal). This does not guarantee that the client App cannot be overwhelmed by subscription notifications, but there should be fewer of them compared to onchange notifications.

b) It is more flexible, the vehicle implementation could decide to send a notification every 0.n seconds as opposed to when a signal changes. An implementation could of course also publish a notification when a data member changed if this was desired.

c) It is more 'OO' as it encapsulates related data into an object instance (but at the expense of potentially needing to parse related data that hasn't changed)

If I was developing a client app, I believe I would have greater confidence that I could predict how my application will behave if it was coded to receive subscription notifications (returning object payloads) e.g. every 0.n seconds - the values from which are then e.g. mapped to UI controls

I would be keen to hear other's thought on this, but as it stands, my vote would be to not change the specification to add onchange and an interface for every signal.

@drkevg drkevg closed this as completed Jan 5, 2016
@djensen47
Copy link
Contributor

Was this issue accidentally closed or closed on purpose?

@tobie
Copy link
Member

tobie commented Jan 5, 2016

If we imagine a vehicle travelling at e.g. 70km/h (70000 meters per hour), one implementation might send an onchange notification if speed changes by 1 meter per hour, whilst another implementation might only send a notification if speed changes by 10 m/h. So in one case, the onchange event will be called much more frequently than the other.

Yes. That's an issue I'm trying to tackle in the sensor spec. The idea is to offer two different solutions: 1) an onchange handler that is called when the value is larger than a settable treshold, and 2) an ondata handlers which is called at regular, settable intervals.

a) It is less 'chatty' as it requires fewer notifications (because related data is grouped into a single object, as opposed to a notification being required for each data member/signal). This does not guarantee that the client App cannot be overwhelmed by subscription notifications, but there should be fewer of them compared to onchange notifications.

Well, that could also get you much more data then needed if you were only intending to listen to a property that rarely changes and get notification every time there's a change in any of the related properties, no?

b) It is more flexible, the vehicle implementation could decide to send a notification every 0.n seconds as opposed to when a signal changes. An implementation could of course also publish a notification when a data member changed if this was desired.

Yeah, this is something the sensor API could give you too but with sensor specific defaults (for example, speed changes need to be updated more frequently than gaz level).

c) It is more 'OO' as it encapsulates related data into an object instance (but at the expense of potentially needing to parse related data that hasn't changed)

I don't think grouping related properties together make things more 'OO', tbh.

@paulboyes paulboyes reopened this Jan 6, 2016
@drkevg
Copy link
Contributor

drkevg commented Jan 6, 2016

Was this issue accidentally closed or closed on purpose?
Sorry all, closed by fat fingering, thanks Paul for re-opening

@acrofts84
Copy link
Contributor Author

Thank you for the discussion, it has helped a lot to distil my thoughts. I hope this is an appropriate summary.

In terms of data grouping, the EventHandlers method allows this in exposing an interface where attributes are exposed with their method. Whereas pub/sub exposes an interface with methods to a set of attributes.

In a similar vein, if we have individual dedicated generic observers, in the EventHandler case, this can be extensible by adding a VehicleSignal attribute which will gain the same methods. In the same way that the pub/sub method we just add another attribute. The difference here is that the data type for the attribute will have to be defined in prose for the EventHandler case, since the type is VehicleSignal (as @peterMelco's question).

I think an important use case is the difference between continuous and discrete signals and how they are implemented.

As in the example below, for the EventHandler case we have two methods:
ondata - data is exposed every x milliseconds defined by frequency
onchange - data is exposed every delta in value defined by threshold
If frequency and threshold are not set it would be down to the implementation. This is where confusion lies, because when I set onchange, traditionally in a DOM context, I expect it to give a discreet value and I don't usually have to set a threshold. This would have to be documented appropriately, but may be a source of confusion in using a 'constricted' onchange .

In the subscription model we subscribe to an update every x seconds defined by period. If we do not define a period, we assume (maybe this should be noted), that the implementation updates the subscription when the value changes at a sensible rate defined by the implementation. This is not as specific, but is it more flexible?

Here are a couple of examples:

// Subscription model
//continuous: subscribe to speed every 2 seconds
var vehicleSpeedSub = vehicle.vehicleSpeed.subscribe(function (vehicleSpeed) {
  console.log("Vehicle speed changed to: " + vehicleSpeed.speed);
}, 2000);
//discrete: subscribe to change in door lock. 
//No mention of on change, but a sensible implementation would implement as such. 
// Is this worth a note in the spec to account for this?
var doorLockSub = vehicle.door.subscribe(function(door){
 console.log(door.lock);
});

// EventHandlers model
// continuous: subscribe to speed every 2 seconds => using ondata
vehicle.vehicleSpeed.speed.frequency = 0.5; //Hz
vehicle.vehicleSpeed.speed.ondata = function (vehicleSpeed) {
  console.log("Vehicle speed changed to: " + vehicleSpeed.speed);
};
// continuous: subscribe to speed every change of 10000 => using ondata
vehicle.vehicleSpeed.speed.threshold = 10000; //meters per hour
vehicle.vehicleSpeed.speed.onchange = function (vehicleSpeed) {
  console.log("Vehicle speed changed by greater than 10km/h to: " + vehicleSpeed.speed);
};

// discrete: binary therefore threshold and frequency are irrelevant
// but do they need to be set or is this kept in the hands of the implementer
vehicle.door.lock.onchange = function(door){
 console.log(door.lock);
});

@tobie
Copy link
Member

tobie commented Jan 7, 2016

when I set onchange, traditionally in a DOM context, I expect it to give a discreet value and I don't usually have to set a threshold.

A default threshold can be left implementation specific or be defined by the spec.

@tobie
Copy link
Member

tobie commented Jan 7, 2016

There's a lot to unpack here. @acrofts84, I emailed you separately as I have a bunch of related questions I don't want to sidetrack this thread with. I'm happy to get back here with some further comments (and maybe even a convergence plan for the Sensor and Automotive APIs) once I have a clearer picture in mind.

@djensen47
Copy link
Contributor

Going back a little bit

The main criticism received from the Generic Sensor API WG was that the current spec was "not very web", and they suggested implementing a DOM Events style, as per their API, in order to make the spec more usable and intuitive to a web developer.

It seems the crux of this conversation is to make the API more like older W3C APIs (using EventHandlers) to make it more "intuitive" to the developer. No offense to all the great work of W3C past but if all the W3C APIs were perfect and intuitive we wouldn't have light frameworks like jQuery.

This is more of a meta-argument in that we shouldn't use EventHandlers because everybody else is doing. If we use EventHandlers, we should do it because it's the right solution.

Another thing to consider is that vehicle sensor signals are not UI, they are not DOM elements.

Aside: after looking at some of these examples here I think the API, in general pollutes the global namespace and it seems much too static.

Also, I think this API suggestion worsens the situation mentioned in my aside by setting variables on static (or singleton?) instances of, for instance, vehicleSpeed. This is inflexible and maybe a security problem. First, I cannot subscribe to a signal multiple times with different parameters. (Maybe I want to update a UI frequently with speed but sample slower for data analysis/collection). Second, a malicious script might be able change values on vehicle.vehicleSpeed.


The conversation about discrete vs. continuous is a good one. For better or worse, I've seen APIs that split these into two different APIs: give my a value ASAP vs. give me updates as soon as possible,

Also, isn't the example of the door lock being discrete is actually continuous? The interval is very long but you are continuously watching for the doors being locked, no?

@tobie
Copy link
Member

tobie commented Jan 7, 2016

Also, isn't the example of the door lock being discrete is actually continuous? The interval is very long but you are continuously watching for the doors being locked, no?

Absolutely, the crux of this issue is whether you're referring to the implementation (which currently is polling for status on MEMS sensor, but might be pushed from sensor hubs in the near future) or to the interval at which the API consumer wants to be updated.

@tobie
Copy link
Member

tobie commented Jan 7, 2016

No offense to all the great work of W3C past but if all the W3C APIs were perfect and intuitive we wouldn't have light frameworks like jQuery.

Well, there are mainly two things to this: old design choices which look passé nowadays and weren't that good a design to begin with (so definitely agree there's API design debt here we have to deal with), but most importantly, there's an explicit decision to expose lower level APIs today and allow third party JavaScript libraries to implement higher level APIs on top of them. That's documented in the Extensible Web Manifesto and has had massive Web developer buy in.

This is more of a meta-argument in that we shouldn't use EventHandlers because everybody else is doing. If we use EventHandlers, we should do it because it's the right solution.

Well, consistency with the rest of the platform is a laudable goal in itself. There's also forward compat benefits when a native ES Observables lands and replaces EventTarget in the browser.

Another thing to consider is that vehicle sensor signals are not UI, they are not DOM elements.

I think that's irrelevant, really. All of node.js is built on an EvenTarget-like construct (EventEmitter) and doesn't even sit in a browser.

Aside: after looking at some of these examples here I think the API, in general pollutes the global namespace and it seems much too static.

Global pollution is an unfortunate consequence of using WebIDL. Other APIs similarly pollute the global namespace despite it not being so obvious upfront (as an aside, the use of the [NoInterfaceObject] extended argument in the existing spec to avoid this global name pollution is strongly discouraged by the WebIDL spec). The real solution to global namespace pollution is ES6 modules. And those are coming to the Web soon.

Also, I think this API suggestion worsens the situation mentioned in my aside by setting variables on static (or singleton?) instances of, for instance, vehicleSpeed. This is inflexible and maybe a security problem. First, I cannot subscribe to a signal multiple times with different parameters. (Maybe I want to update a UI frequently with speed but sample slower for data analysis/collection).

Yes, though I think basing this spec on EventTarget (or yet better, on the Generic Sensor API) is the right choice, I don't think that the current design (which tries to recreate a giant model of the vehicle) is the right one.

Second, a malicious script might be able change values on vehicle.vehicleSpeed.

Once you give access to the page to a third party script, all bets are off anyway.

@djensen47
Copy link
Contributor

@tobie 👍

@djensen47
Copy link
Contributor

Do you have a link handy for the Generic Sensor API?

@djensen47
Copy link
Contributor

After a quick scan, I really like this approach in the Generic Sensor API. I'll do a more thorough reading tonight.

@tobie
Copy link
Member

tobie commented Jan 7, 2016

@djensen47 looking forward to your comments. Also happy to setup up a call if there's interest on your side. LMK.

@djensen47
Copy link
Contributor

This thread is so long, I think I missed this comment: #72 (comment)

I like the suggestions in the aforementioned comment as well – it definitely fits in line w/the Generic Sensors API.

In general, when dealing with any signal that is sensor-like, why wouldn't Automotive WG want it to be compatible with the Generic Sensors API and the Permissions API? Then, the only aspect of the API that needs to be different is actuation of something in the vehicle.

@djensen47
Copy link
Contributor

Also, instead of using vehicle.data directly. I would prefer a constructor. var door = new vehicle.Door();

@acrofts84
Copy link
Contributor Author

I have tried to give some more detailed examples below, which utilise EventHandlers on interfaces with multiple attributes.

Some key points:

  • Interfaces with multiple atributes will have to adhere to the same threshold (see fuel.onchange below) or use implicit thresholds for the entire interface
    • If we don't group data this will not be an issue, but leads to a less flexible interface
  • In order to return data for multiple zones, the interface will return an array of data. I believe in WebIDL this will be a dictionary, but I will have to confirm (see door.onchange below)
    • I have added a settable zone property to VehicleInterface to allow setting individual zones (see wheelTick.selectedZone = new Zone(['rear', 'left']))
    • If selectedZone is set to null the interface should return all zones
interface VehicleInterface {
    Promise get (optional Zone zone);
    readonly        attribute Zone? selectedZone;
    readonly        attribute Zone[] zones;
};

interface VehicleSignalInterface : VehicleInterface {
    Promise        set (object value, optional Zone zone);
    EventHandler   onchange
    EventHandler   ondata
    EventHandler   onerror
    attribute      double   period
    attribute      double?  threshold
}
var vehicleSpeed = new vehicle.vehicleSpeed();

// continuous: subscribe to speed every 2 seconds => using ondata
vehicleSpeed.frequency = 2000; //milliseconds
vehicleSpeed.ondata = function (vehicleSpeed) {
  console.log("Vehicle speed changed to: " + vehicleSpeed.speed);
};

// continuous: subscribe to speed every change of 10000 => using onchange
vehicleSpeed.threshold = 10000; //meters per hour
vehicleSpeed.onchange = function (vehicleSpeed) {
  console.log("Vehicle speed changed by greater than 10km/h to: " + vehicleSpeed.speed);
};

// static data
var vehicleId = new vehicle.identification();
vehicleId.get = function (id) {
    console.log("The vehicle identification number is " + id.VIN);
}

//dynamic data regardless of zone
var gyro = new vehicle.gyro();
gyro.threshold = 6;
gyro.onchange = function(gyro) {
    console.log("Gyro data has changed by 6 degrees/second");
    console.log("Yaw rate: " + gyro.yawRate);
    console.log("Roll rate: " + gyro.rollRate);
    console.log("Pitch rate: " + gyro.pitchRate);
}

// onchange with implicit threshold
// returns an array of wheelTicks
// not sure how to represent this in IDL ... dictionary?
var door = new vehicle.door();
door.onchange = function(doors) {
    for (var i = 0; i < doors.length; i++) {
        console.log("Door in zone " + doors[i].zone.value + " is " + (doors[i].lock == true ? "locked" : "unlocked"));
    }
}

// dynamic data with all zones
// returns an array of wheelTicks
// not sure how to represent this in IDL ... dictionary?
var wheelTick = new vehicle.wheelTick();
wheelTick.threshold = 1000;
wheelTick.currentZone = null;
wheelTick.onchange = function(wheelTick) {
    for (var i = 0; i < wheelTick.length ; i++) {
        console.log("Update after 1000 wheel ticks:" + wheelTick[i].zone.value + ", wheel tick: " + wheelTick[i].value);
    }
}

// dynamic data with single zone
// returns an array of wheelTicks
// not sure how to represent this in IDL ... dictionary?
var wheelTick = new vehicle.wheelTick();
wheelTick.threshold = 1000;
wheelTick.selectedZone = new Zone(['rear', 'left']);
wheelTick.onchange = function(wheelTick) {
    for (var i = 0; i < wheelTick.length ; i++) {
        console.log("Update after 1000 wheel ticks:" + wheelTick[i].zone.value + ", wheel tick: " + wheelTick[i].value);
    }
}

// data in interface with different types
// interfaces will have to be separated into data of the same type with the same threshold, or take the default threshold
// the other option is to not encapsulate data into group
var fuel = new vehicle.fuel();
fuel.threshold = /* has to take default value or break interface */
fuel.onchange = function(fuel) {
    console.log("Average consumption in millilitres per 100 kilometer: " + fuel.averageConsumption);
    console.log("Percentage fuel level: " + fuel.level );
}

@tobie
Copy link
Member

tobie commented Jan 20, 2016

Couple of comments.

  1. What's the benefit of setting a threshold? Is this something you expect to pass to the ECUs themselves to lower the traffic on the CAN bus or is this something the implementation will need to handle at the software level? If it's the latter, there is no benefit in adding this feature. You can drop it and have API consumers implement it in application code if they need it.

  2. Your units are very weird. Either go for your vertical (km/h, etc.) or the platform (ms, meters, etc.), but don't do both (meters per hour!??).

  3. VehicleInterface looks really close to the Generic Sensor API. I think that you need to either consider these things as sensors and actuators and converge in the Generic Sensor API or think about them as message channels on the CAN bus and come up with an API for that (possibly inspired on WebSocket, EventSource or the like).

  4. Being able to change zones after instantiation complicates the implementation and reasoning about the program to an important degree. It might have some annoying security issues to. I recommend setting the zone through the constructor and making it readonly unless you have compelling use cases not to.

  5. You don't need a zone constructor, passing the enum values in an array or a string should be enough. So instead of:

    var wheelTick = new vehicle.wheelTick();
    wheelTick.selectedZone = new Zone(['rear', 'left']);

    You can do:

    var wheelTick = new vehicle.WheelTick({ zone: 'rear-left' });
  6. Nit: capitalize constructors, so new vehicle.Door(), not new vehicle.door().

  7. Use promises to get static data:

    vehicle.getId().then(id => console.log("VIN is " + id.VIN));
    // or, more generic, but harder to do static analysis on:
    vehicle.get("identification").then(id => console.log("VIN is " + id.VIN));

@paulboyes
Copy link

@tobie I will do my best to answer some of your questions:

A threshold was used for onchange as you may not want to see changes until your threshold is reached.

If I recollect correctly, the units were chosen for consistency and to avoid loss of precision. The smallest anticipated unit consistent with integers was chosen. If you look at the data spec you will see an editor's note saying this: "Integers have been used in favor of floating point values in order to preserve precision during calculations (e.g. meters per hour instead of kilometers per hour)."

Regarding number 3 above, I personally would like to have a conversation with the group as to the potential context differences and ramifications.

@tobie
Copy link
Member

tobie commented Jan 21, 2016

threshold was used for onchange as you may not want to see changes until your threshold is reached.

Right. But this is only implemented in software, no? The reason we added this to the Generic Sensor API is that it allows devices equipped with sensor hubs/smart sensors to dramatically reduce power consumption. Unless there's similar incentives here, it's a nice to have, not a must have. And given it seems to cause issues when there's multiple data sources on the same interface, getting rid of it seems like a reasonable option.

@tobie
Copy link
Member

tobie commented Jan 21, 2016

If I recollect correctly, the units were chosen for consistency and to avoid loss of precision. The smallest anticipated unit consistent with integers was chosen. If you look at the data spec you will see an editor's note saying this: "Integers have been used in favor of floating point values in order to preserve precision during calculations (e.g. meters per hour instead of kilometers per hour)."

Floating. Point. Values. This is clearly one of JavaScript's biggest shortcomings. :(

@peterMelco
Copy link
Contributor

Do we need to suffix interfaces with interface ? Is it not enough to just have VehicleSignal and Vehicle

@drkevg
Copy link
Contributor

drkevg commented Jan 25, 2016

onData vs subscribe/unsubscribe.

Conceptually 'onData' seems to be logically equivalent to 'subscribe'.

distanceSubscription.period=100;
The unsubscribe function would then be obsolete.

Suggest that retaining an unsubscribe method has benefits and that subscribing for a period is less flexible, more complex and error prone (please see reasons below).

Using a subscription period based approach is more likely to waste valuable server processing cycles and memory because there will inevitably be periods when the server is still preparing and sending notifications, but client no longer needs them (and will ignore them) e.g. because the user is now doing something different.

Also it is more complex, as the client App will need to keep re subscribing for another period, and another period (in the long running case), client may need to use timers to ensure it re-subscribes and this seems prone to not receiving notifications if one period has expired but the client has not yet renewed the subscription.

In the current spec we allow for subscriptions to an interface and not to an attribute within an interface

The benefit of returning individual signals is that the client can obtain just the signals it is interested in. However, there are a number of benefits to the current object based approach, which supports combining related signal values into an object which is then returned:

a) It enables vehicle specific characteristics to be encapsulated and abstracted.

It is true that a car or a heart rate monitor can be viewed in a generalised way as 'a collection of sensors', just as matter can be viewed as a collection of atoms (or even quarks). However, suggest that it is important to choose a level of generalisation that is helpful for the problem at hand (i.e. most helpful to Developers writing Apps for vehicles).

So whilst generalisation can sometimes be useful, significant benefits can follow from categorising entities into different classes which have attributes or behaviours in common. and there are (of course :-) characteristics that are shared by all cars that are not generally shared by most medical equipment e.g. speed, direction, number of doors etc.

Suggest that an object based approach makes development easier and that it is more efficient for App developers to work with these higher levels of abstraction.

There are also benefits for implementers (OEM's/Tier 1's) that follow from encapsulating raw data/signals - which our current object based approach supports. Encapsulation has the benefit that the implementer can change how particular processed signal values are implemented (as long as they do not break the interface). [In a similar way that the internals of a manual gearbox can be completely changed as long as the input and output shafts and gear lever remain the same].

c) Returning an object enables temporally (time) related data to be returned in a single object instance. For example, say two or more related signals correspond to a particular event or measurement time, they can be returned in a single object and the semantics can be that both signals apply for the same event or instance in time.

In contrast, if each signal is returned and handled separately, inevitably at slightly different times (e.g. because client is single threaded), it will be impossible to know whether or not they relate to the same real world event or instant in time.

Not very Web

The sensor based approach seems to be more like a low level C style approach where a vehicle App developer will need to collect together data from a large number of individual signals, (possibly based on a table of Sensor Id numbers or strings).

In contrast, interfaces that return objects are (~by definition :-) more Object-Oriented and less procedural. As currently defined, the web service interfaces that the vehicle implements would return simple objects using JavaScript Object Notation (JSON), which surely is a (modern) web approach :-)

@acrofts84
Copy link
Contributor Author

My closing remarks, having given this a lot of thought and discussion is to favour the the original pub / sub model.

One of the key things for us, as we've discussed on a previous call is data encapsulation. We want data to be encapsulated so we can change the implementation without having an external effect. This then impacts upon the use of a threshold. As discussed previously, if we have grouped data the threshold would have to be set for each of the variables within the interface and this is not an elegant solution. So thresholds do not work with encapsulation. If the implementation is left to determine this threshold, we are left with the original subscribe function.

We will not have an impact on CAN traffic, but we can have an impact on CPU performance. If we subscribe with a given period, we can then unsubscribe again and the server is no longer producing and sending us data unnecessarily. With the onchange method we have no way to unsubscribe, we just null the callback. The server is then unaware of this change and we have no mechanism of letting the server know to reduce CPU usage, as Kevin highlighted.

The existing model for zones is also more elegant since the same instance of the interface can be used for each zone, by iterating over each available zone, whereas we require a new instance for each zone with the onchange model. Having an all zone method would be useful, but brings complexity to the spec where it isn't necessary.

@tobie
Copy link
Member

tobie commented Jan 27, 2016

@acrofts84 wrote:

One of the key things for us, as we've discussed on a previous call is data encapsulation. We want data to be encapsulated so we can change the implementation without having an external effect. This then impacts upon the use of a threshold. As discussed previously, if we have grouped data the threshold would have to be set for each of the variables within the interface and this is not an elegant solution. So thresholds do not work with encapsulation. If the implementation is left to determine this threshold, we are left with the original subscribe function.

I'm not sure I understand this, as these seem to be orthogonal concerns. You can have event-based systems where event frequency or threshold are settable by the API consumer much like you can have event-based systems where frequency and threshold are fixed or even implementation-specific.

We will not have an impact on CAN traffic, but we can have an impact on CPU performance. If we subscribe with a given period, we can then unsubscribe again and the server is no longer producing and sending us data unnecessarily. With the onchange method we have no way to unsubscribe, we just null the callback. The server is then unaware of this change and we have no mechanism of letting the server know to reduce CPU usage, as Kevin highlighted.

So this assumes API consumers will take care of unsubscribing from a channel to save resources. Unless there are clear incentives for them to do so, there's a huge body of evidence (e.g. Web, but also Android background threads, etc.) that shows this never happens in practice. Either there are such incentives (and you need to mention those and make sure they're actually implemented), or you're much better off designing a system which is able to save resources without API consumer needing to take specific steps. For example, by letting the implementation, that has access to garbage collection events, deal with it.

@tobie
Copy link
Member

tobie commented Jan 27, 2016

@drkevg wrote:

As currently defined, the web service interfaces that the vehicle implements would return simple objects using JavaScript Object Notation (JSON), which surely is a (modern) web approach :-)

So I pointed out above (see point 3 in #72 (comment)) that there needed to be a clear choice between considering these things as sensors and actuators or as messages on the CAN bus.

If it's the latter, I strongly agree with you that a JSON-based, web service-like interface would be very Web-y. In that case, though, you probably want to structure your spec differently. Specify a lightweight API for the CAN bus similar to Web Messaging and then build a JSON-based schema on top of it using JSON-Schema in a different spec.

@QingAn
Copy link
Contributor

QingAn commented Jan 29, 2016

We will not have an impact on CAN traffic, but we can have an impact on CPU performance. If we subscribe with a given period, we can then unsubscribe again and the server is no longer producing and sending us data unnecessarily. With the onchange method we have no way to unsubscribe, we just null the callback. The server is then unaware of this change and we have no mechanism of letting the server know to reduce CPU usage, as Kevin highlighted.

If this happens during implementation in vehicle as @acrofts84 wrote (would need some examples or evidences), I suggest to reconsider the subscribe/unsubscribe model.

@peterMelco
Copy link
Contributor

Could someone explain why the event based approach does not support data encapsulation. It seems to me that the design is flawed if a vehicle API is unable to support a more pure event driven approach . Also, if we should have a low level approach and then let JavaScript API developers evolve on top of this it is necessary to refactor. If you look at current API implementations many are based on EventTarget. I don't really buy into the CPU argument: I completely agree with @tobie that this is much better handled by the implementation - we have many evidence of this. The idea must be to have an attractive API that removes - if possible - the unpleasantness of having to deal with CAN based data.

@acrofts84
Copy link
Contributor Author

Using the event based approach with encapsulation will mean you have no control over what onchange relates to within the interface. It could be any of the signals, even those that have yet to even be defined as members of that interface.

I have discussed this within the company and the JLR standpoint is that the change would make the specification harder to implement and therefore less likely to be employed.

@tobie
Copy link
Member

tobie commented Jan 29, 2016

Using the event based approach with encapsulation will mean you have no control over what onchange relates to within the interface. It could be any of the signals, even those that have yet to even be defined as members of that interface.

I thought that was precisely the issue with the current pub/sub model, to quote you above (#72 (comment)):

In the current spec we allow for subscriptions to an interface and not to an attribute within an interface.

I'm now unsure what the desirable outcome is. Is there a list of requirements for this API somewhere? What is it that makes events in general unfit for this API? Note you're not limited to use onchange events. You could have event listeners of all types, e.g.:

vehicle.onspeedchange = // ...

vehicle.ondata = // ...

vehicle.onnotification = // ...

vehicle.ontimeinterval = // ...

vehicle.onheartbeat = // ...

// etc.

You could even have specific props on events emitted by different ECUs or for different data types, on the same interface).

vehicle.onchange = e => {
    if (e.dataType == "fuelConsumption") {
        // do stuff...
    }
};

Etc.

I have discussed this within the company and the JLR standpoint is that the change would make the specification harder to implement and therefore less likely to be employed.

Can you explain what difficulties implementors would bump into here that the pub/sub model avoids? What features of the existing pub/sub model are missing from the DOM's EventTarget/Event* APIs?
How is what appears to be basically API cosmetics a blocker for implementation purposes?

You might also want to keep in mind the priority of constituencies when making such decisions:

[C]onsider users over authors over implementors over specifiers over theoretical purity.

I maintain that you should address some or the more general questions I brought up earlier (namely: who's your developer audience? Do you want them to think of the API as message passing over a bus or accessing MEMS sensors, etc.)

That said, I'm not even an invited expert in this group and don't want to derail your progress more than I have already, so I'll keep quite going forward. Still, more than happy to help if you need me. You know where to reach me. :)


* Note that I'm specifically mentioning EventTarget here and not the Generic Sensor API.

@peterMelco
Copy link
Contributor

Using the event based approach with encapsulation will mean you have no control over what onchange relates to within the interface

So in my view we could take object-orientation one step further and have dedicated observers: onspeedchange, onwheelpulsechange when that is suitable, and use datatypes on events when that is suitable.

@djensen47
Copy link
Contributor

@tobie said …
You might also want to keep in mind the priority of constituencies when making such decisions:

[C]onsider users over authors over implementors over specifiers over theoretical purity.

I maintain that you should address some or the more general questions I brought up earlier (namely: who's your developer audience? Do you want them to think of the API as message passing over a bus or accessing MEMS sensors, etc.)

I completely agree with this. As a user of APIs, I see a benefit in abstracting the API for simplicity. The base level abstraction can be the Sensors API, similar to (but not the same as) the Sensors API, or something else. I don't want to use a complicated, non-intuitive API.

so I'll keep quiet going forward

Please don't keep quiet. I, for one, have found your feedback useful. Maybe you should be an invited expert in this group.

@djensen47
Copy link
Contributor

@tobie wrote

So I pointed out above (see point 3 in #72 (comment)) that there needed to be a clear choice between considering these things as sensors and actuators or as messages on the CAN bus.

This is probably the first decision we need to make and then we can move forward with the details.

@paulboyes
Copy link

All, For a bit of context. When the Vehicle API was created, the following creation guidelines were published (by QNX) and used. In short, the prime goal was to provide a simple API for accessing vehicle data for in-vehicle infotainment application developers at the vehicle level.

https://cdn.rawgit.com/w3c/automotive-bg/master/W3C%20Vehicle%20API%20Creation%20Guidelines%20v4.docx

Please take a look for next Tuesday's meeting. I will make sure they make it to the wiki as a PDF file as well.

All of this input has been great. We need to come to a conclusion on this issue and pick a path forward. I will compile the list of issues as I see them for next Tuesday's meeting.

@paulboyes
Copy link

@peterMelco
Copy link
Contributor

WebSocket API with a JSON schema and then build javascript APIs on top of this seems like an attractive idea. The WebSocket API could then allow the API on top to close a socket(either direct or indirect). The WebSocket API could either define a send schema and a return schema , or it could use the same for both ways ? The bundling of data is handled by the JavaScript API and the "open-source community", and we will probably end up with both event driven data encapsulation APIS and the more procedural object encapsulation scheme as suggested here, but that would then not be a W3C concern. I think that this could be good approach and I totally buy into the idea that the developers APIs should be community driven, and as an example of this it could be handled by the GENIVI alliance.

@djensen47
Copy link
Contributor

Here's a thought on how to do this via something that is WebSocket-ish, i.e. a service layer:

https://gist.github.com/djensen47/293f3b9e4500e778b443

@tguild
Copy link
Member

tguild commented Mar 14, 2017

API refactored from WebIDL approach

@tguild tguild closed this as completed Mar 14, 2017
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

No branches or pull requests

9 participants