Recently ran into an issue with clearWatch in library code. (This cost me couple of hours, so possibly over exaggerating this...)
clearWatch() and clearWatch(undefined) clears all pin-watches. You can accidentally call it with undefined, if you don't check whether you have an actual watch Id, for example line 31-32 in DHT22.js.
Because of the "reliability" of the DHT11 & 22 sensors and the 10 default retries in the libraries, these are dangerous for almost 6 seconds (550 ms timeout * 10). If you don't read the docs, you can run into a race condition where you start a conversion before all possible timeouts of the previous read had finished.
If you use setWatch elsewhere, that clearWatch(undefined) just destroys you program without crash, and leads to some head scratching... Or leads to a memory leak, if you call .read() too soon. It did leak slowly at 10 retries and 3 second read interval. At the end the setWatch 'count' was up to 50. But that's a different matter.
Also clearTimeout and clearInterval is the same.
First, a suggestion:
What would be the preferred way to handle clear***()? Seen several ways in devices / modules code: I think it shouldn't be without checks and should set the 'watch' property to falsey, if there is one.
For example there is this nice, but nontrivial looking example from the TouchRD code (_ is "this", and _.w is the watch Id):
_.w = _.w && clearWatch(_.w);
Or the more obvious
if (this.watch) {
clearWatch(this.watch);
this.watch = undefined;
}
If there is a consensus, I'm willing to change the libraries, around dozen uses in JS files.
Does anyone think it would worth making clearWatch(undefined) invalid, and changing the clear-all call explicit (for example pass in -1)? I know, it's a breaking change. Possibly write a warn to console?
Or better just improve the libraries & docs?
Espruino is a JavaScript interpreter for low-power Microcontrollers. This site is both a support community for Espruino and a place to share what you are working on.
Hi!
Recently ran into an issue with
clearWatch
in library code. (This cost me couple of hours, so possibly over exaggerating this...)clearWatch()
andclearWatch(undefined)
clears all pin-watches. You can accidentally call it with undefined, if you don't check whether you have an actual watch Id, for example line 31-32 in DHT22.js.Because of the "reliability" of the DHT11 & 22 sensors and the 10 default retries in the libraries, these are dangerous for almost 6 seconds (550 ms timeout * 10). If you don't read the docs, you can run into a race condition where you start a conversion before all possible timeouts of the previous
read
had finished.If you use
setWatch
elsewhere, thatclearWatch(undefined)
just destroys you program without crash, and leads to some head scratching... Or leads to a memory leak, if you call.read()
too soon. It did leak slowly at 10 retries and 3 second read interval. At the end the setWatch 'count' was up to 50. But that's a different matter.Also
clearTimeout
andclearInterval
is the same.First, a suggestion:
What would be the preferred way to handle
clear***()
? Seen several ways in devices / modules code: I think it shouldn't be without checks and should set the 'watch' property to falsey, if there is one.For example there is this nice, but nontrivial looking example from the TouchRD code (
_
is "this", and_.w
is the watch Id):Or the more obvious
If there is a consensus, I'm willing to change the libraries, around dozen uses in JS files.
Does anyone think it would worth making
clearWatch(undefined)
invalid, and changing the clear-all call explicit (for example pass in-1
)? I know, it's a breaking change. Possibly write a warn to console?Or better just improve the libraries & docs?