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

Changed semantics of pulse() #26

Merged
merged 1 commit into from
May 26, 2017
Merged

Changed semantics of pulse() #26

merged 1 commit into from
May 26, 2017

Conversation

jweisz
Copy link
Collaborator

@jweisz jweisz commented May 25, 2017

Scope of Changes

  • changed pulse() behavior to match that of pulseOnce() -- issuing a single pulse to the LED
  • removed pulseOnce(), isPulsing(), and stopPulsing()

Rationale -- pulsing has been a very unreliable operation in Node.js (at least the way it was implemented). When pulsing continuously, the event loop of Node.js is blocked during the LED pulse. That meant the only time available for processing other things, such as the callback used in tj.listen(), was between pulses. This made for a bad user experience; the intent of pulse() was to show feedback that TJBot was listening, but by the nature of it's implementation, it wasn't able to listen during the pulse(), causing spoken input to be ignored. Removing the stateful pulse() and changing it to a single issuance of a pulse (i.e. what pulseOnce() did), makes it a bit safer to use this method in conjunction with other methods that have callbacks.

Note: accepting this PR will cause a breakage in recipes that rely on the existing API (at least where pulsing is concerned). I recommend we bump TJBotLib to v1.3 to indicate the lack of backwards compatibility with the v1.2.x series.

@jweisz jweisz requested a review from victordibia May 25, 2017 22:22
Copy link
Contributor

@victordibia victordibia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.
I agree we should bump up version before any further changes.

@victordibia victordibia merged commit 9ff5cc1 into master May 26, 2017
@jweisz jweisz deleted the refactor/jw-pulse branch June 27, 2017 17:30
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

Successfully merging this pull request may close these issues.

2 participants