Utility problem with setWatch

[espruino-discuss3]

Posted at 2015-08-02 by RandyHarmon

I'm experiencing a usability problem with setWatch, wherein the documented behavior matches reality precisely, but it's not useful to me (I dare say not useful to anyone). I'm exploring possibilities for adjustments toward more practical utility. Scenario:

1. setWatch() with edge: 'rising' or 'falling'
2. in the event handler, print event.time and event.lastTime
3. compare each event's output to the output from the prior event

The result is that each event's 'lastTime' is mismatched with the 'time' of the prior event. With edge: 'both' (default), that's not the case, and the documentation corroborates this evidence; lastTime is NOT the time of the prior triggered event; it's the time of the last-state-change on the pin.

Perhaps it's only my lack of imagination, but I don't see how the time-of-last-state-change is useful to any client code. Time-of-last-event is IMO the only thing that could possibly be sensical from a "serve the client" point of view. A client who wants time-since-state-change can use edge:'both'.

Can we look for a way to fix this, to produce more usability for developers? It could be approached in any number of ways. I'll suggest a few here:

• change lastTime to be time-of-last-triggered-event. Not back-compatible.

• Add lastEventTime as time-of-last-triggered-event. Heavier, but back-compatible.

• Add lastEvent as a reference to the prior-triggered event; client code consults event.lastEvent.time instead of event.lastTime. Increases memory usage by one event per setWatch, but lightens the event structure (especially if lastTime becomes deprecated and then is removed)

I have not tested with noisy inputs, with or without debouncing, being a little bit busy, and a little bit afraid of the result :). Perhaps the debouncing logic holds the key for explaining the current behavior, but I'd be glad to help work out expectations and maybe even some code for it.

@ALL and @gfwilliams - your thoughts?

R

[espruino-discuss3]

Posted at 2015-08-03 by @gfwilliams

I don't see how the time-of-last-state-change is useful to any client code.

It's actually hugely useful when measuring pulse widths. Often you'd only want your JavaScript code to be called on (for instance) the falling edge, but you want to measure the width of the pulse. In that case only calling your JS once means you can handle signals that are (almost) twice as fast as you'd be able to otherwise, while keeping your code really simple.

As you suggest it's kind of a side effect of the debounce - in most cases (edge:'both' or debounce!=0) you need both edges, so it's easier for the software to just record both, and being able to measure pulse widths is a nice side-effect.

Adding lastEventTime sounds like a possible option - what do others think?

However it's absolutely trivial to work around:

var lastTime;
function onWatch(e) {
  var d = e.time - lastTime;
  lastTime = e.time;
}

So I wonder if it's really worth slowing everything down and using more memory just to save 2 small lines of code?

---

Posted at 2015-08-05 by RandyHarmon

Oh, interesting. Thanks for helping enhance my imagination. So by measuring the pulse width, you can get a sense of the signal's "speed" on a pseudo-instantaneous basis. That's cool.

Still, I do feel that the default mode should be more obviously correct, with a mode to measure pulse-width available as a non-default option. +1 on efficiency and memory. Also, the same thing for developer time, if those two aren't in conflict.

What if...

• when edge:rising or edge:falling, default to event-orientation, unrolling the trivial workaround to non-developer responsibility
• when pulseWidth: true, additionally include pulseWidth or lastChangeTime in the event

WDYT?

R

---

Posted at 2015-08-05 by @gfwilliams

I see the reasoning, but if I understand right the issue here is it's not backwards compatible - all current modules (and users!) that have code that relies on the behaviour for rising and falling events will find that it stops working in a very non-obvious way, with no warnings or errors.

Surely the situation could just be improved by just changing the documentation, so it explicitly mentions the behaviour rather than just having the single sentance. Perhaps an optional argument could disable the behaviour, but if we're doing that it's probably better all around to just add the lastEventTime variable.

Anyone else want to chip in here?

---

Posted at 2015-08-05 by RandyHarmon

Back-compat is certainly the sticking point. Doc improvement would certainly help, if nothing else arises - no question. The rest of your logic makes sense also.

Another technique for changing interfaces to existing functionality is to simply make a new version, deprecating the original over time. Could we add PIN.prototype.watch() or PIN.prototype.on() with new behavior, or would that preclude support for signals that aren't PIN's?

Obviously my approach on this is aggressive... with setWatch being such a fundamental technique, I'm highly motivated toward absolute zero on the "surprise" scale. (By the way, thanks so much for your welcoming attitude toward that pushy Randy guy) (also: no, I'm not a farmer).

If can, can. If no can, no can. :)

R

---

Posted at 2015-08-06 by @gfwilliams

No problem - yes, adding another function and deprecating the other is definitely an option... and having a Pin.on(...) is a nice idea that is far more JS-ish.

However I have to be careful - especially on the original Espruino board the binary only just fits in available flash. If adding more functions means I then can't add some other feature that people wanted, is it really worth it?

Espruino's been going for a while now (3 years?) and has been used by thousands of people. You're the first one to complain about this, so unless loads more people have a problem with it I'm not sure it warrants such drastic action - especially as it's dead easy to work around.

---

Posted at 2015-08-06 by RandyHarmon

Yeah, I get it. So docs it is.