-
Notifications
You must be signed in to change notification settings - Fork 32
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
Adds basic LED functionality #52
Conversation
@@ -15,6 +16,13 @@ function Tessel() { | |||
}; | |||
this.port = this.ports; | |||
|
|||
this.led = [ | |||
new LED('red', '/sys/devices/leds/leds/tessel:red:error/brightness'), | |||
new LED('amber', '/sys/devices/leds/leds/tessel:amber:wlan//brightness'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? wlan//brightness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, I should remove that extra slash. Somehow it still worked :)
lgtm |
@kevinmehall can you comment on point 4 above? |
It doesn't change the triggers, so I don't think it will do anything with the WLAN LED and won't be able to add triggers . To modify the triggers, you write to the |
So if I understand you correctly, network traffic will continue to manipulate the WLAN LED after it's been "manually" set to one state or the other? |
Also, when designing APIs that manipulate triggers, we need to to figure out whether to write to sysfs directly, or use UCI (/etc/config/system). sysfs will not persist across reboots, but the UCI LED settings are appled at boot. |
I think so, yes. |
👍 Will merge as is. |
Adds basic LED functionality
return | ||
} | ||
else { | ||
throw new Error("Invalid state returned by LED:" + value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johnnyman727 why throw _
instead of callback(_)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's not a recoverable error or one that is expected to ever happen during normal use. My hope is that if this does get hit, the user reports it so we can fix any unknown bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what way is it not a recoverable error? I agree that it's not something that is expected to ever happen, but that dosen't necessarily mean that throwing is the correct thing to do.
According to Joyent's "Error Handling in Nodejs" this is an operational error in an asynchronous function, and thus it should deliver the error thru the callback.
I'm sorry to be such a pain in the arse but I think it's important that best practices be followed in such a large project as this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry to be such a pain in the arse but I think it's important that best practices be followed in such a large project as this :)
No apologies necessary! Please continue to point out questionable code - it helps the project and the programmers (like myself!) improve.
In what way is it not a recoverable error?
I was on the fence about this because it seems like if you can't use an LED for visual indication as expected, you are out of luck. I suppose you can turn on another LED to indicate that something went wrong.
In any case, I think you're right that, for consistency sake, this should be provided in the callback. Thanks for looking through this! Another PR coming soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This PR adds the ability to blink the four LEDs on T2. There are a couple of things to note:
LED
classcolor
property because I thought that would be useful for some people.@kevinmehall can you check it out?