Skip to content

Add isAnode to Led Docs #1026

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

Closed
reconbot opened this issue Jan 30, 2016 · 25 comments
Closed

Add isAnode to Led Docs #1026

reconbot opened this issue Jan 30, 2016 · 25 comments
Assignees

Comments

@reconbot
Copy link
Collaborator

It's missing as a possible property to the options.

@reconbot reconbot added the DOCS label Jan 30, 2016
@scottgonzalez
Copy link
Contributor

isAnode doesn't apply to Led. This is used to denote whether the common pin on a multi-LED component (e.g., RGB LEDs, seven segment displays, etc.) is anode or cathode. On a single LED, there is no common pin because it's just one diode.

@reconbot
Copy link
Collaborator Author

It does work for leds that are wired in reverse however.

On Sat, Jan 30, 2016, 7:48 PM Scott González [email protected]
wrote:

isAnode doesn't apply to Led. This is used to denote whether the common
pin on a multi-LED component (e.g., RGB LEDs, seven segment displays, etc.)
is anode or cathode. On a single LED, there is no common pin because it's
just one diode.


Reply to this email directly or view it on GitHub
#1026 (comment)
.

@derekwheee
Copy link
Collaborator

This topic comes up about every 6 months. We should definitely do something. I'd opt for removing it from Led altogether.

  • isAnode is in Led because originally Led.RGB was just 3 instances of Led
  • Led.RGB no longer extends from Led, so it really doesn't need to exist in Led anymore

I suppose it's true that you could use isAnode for an LED wired in reverse, but is there any practical reason to do that?

@soundanalogous
Copy link

There are 2 different ways to wire an LED to a microcontroller. The more common way is to wire the LED anode to the microcontroller pin and the cathode to ground (with a resistor between the mcu pin and the anode or the cathode and gnd). This is known as a "source drive" circuit since the mcu pin is sourcing the current to power the LED. The other way is "sync drive" in which the cathode is wired to the mcu pin and the anode is wired to power and current flows into the mcu (which can be more power efficient in some designs but likely makes little difference for prototypes and hobby applications). When using sync drive, you set the pin value to 0 rather than 1 to turn the LED on. Therefore it's necessary to provide a means to differentiate between the two configurations in order to determine if led.on should set the pin value to 0 or to 1.

@soundanalogous
Copy link

Here's a slightly more detailed explanation on source vs sync: https://startingelectronics.org/articles/current-sourcing-sinking/

@derekwheee
Copy link
Collaborator

That's really interesting, thanks for detailed explanation @soundanalogous. If Johnny-Five wants to support that configuration, isAnode still probably isn't the best name for that property.

@dtex
Copy link
Collaborator

dtex commented Feb 1, 2016

button, joystick, motor and servo all have an invert property that does a similar (conceptually) thing. Not saying it's the right property name (no hablo LED), just one to consider.

@reconbot
Copy link
Collaborator Author

reconbot commented Feb 1, 2016

I actually thought invert existed and had to look at the source to figure
out isAnode.

Both kinda make sense here, for both led and rgbled. I'm happy as long as
there are docs. And happier if the APIs are consistent.

On Mon, Feb 1, 2016, 9:36 AM Derek Wheelden [email protected]
wrote:

That's really interesting, thanks for detailed explanation @soundanalogous
https://github.com/soundanalogous. If Johnny-Five wants to support that
configuration, isAnode still probably isn't the best name for that
property.


Reply to this email directly or view it on GitHub
#1026 (comment)
.

@Resseguie
Copy link
Collaborator

isAnode makes sense for Led.RGB because that's what it's actually called ("common anode"). I've always thought it was confusing in the vanilla Led class. But I had never thought about the case @soundanalogous described. I'd tend to agree with @dtex on inverted in that case. I just don't like the idea of Led and Led.RGB having different names for similar concepts.

Could you also have a common anode RGB LED that is similarly wired? That doesn't make sense to me, but I acknowledge that I hadn't heard of the "sync drive" case before, so maybe I'm missing it.

@reconbot
Copy link
Collaborator Author

reconbot commented Feb 2, 2016

Isn't it "sink drive"?

I like inverted but I worry about the undocumented isAnode as people might have been using it. Both?

@derekwheee
Copy link
Collaborator

I like invert as well. I'd say alias isAnode to invert, add a deprecation warning, and remove it in the next major.

@derekwheee
Copy link
Collaborator

@Resseguie If that's the case for Led.RGB it would make sense to have isAnode and invert in that class.

@soundanalogous
Copy link

inverted is kinda weird unless you understand the reference to what is inverted. I would think explicit Led.SOURCE_DRIVE and Led.SINK_DRIVE modes would be best if well documented. Default to Led.SOURCE_DRIVE since that is the most common configuration in hobby applications since it's easier for the user to understand that 1 = ON. In the likely rare case someone wires an LED in the sink drive configuration, they set the mode on the Led constructor param object.

@Resseguie
Copy link
Collaborator

@Frxnz I thought the same thing about having both, but I don't know if that's an actual case that exists. For a standard LED, you're just either applying power or not. For an "inverted"/sink drive RGB, what would be controlling the values to R-G-B respectively? That's why I didn't know if such a use case exists.

If we go with inverted (since we've used it elsewhere) I think aliasing it with isAnode is probably fine (and deprecating it). But I like the thinking of @soundanalogous on explicit modes as an alternative. I could go either way.

@rwaldron
Copy link
Owner

rwaldron commented Feb 2, 2016

#258

@reconbot
Copy link
Collaborator Author

reconbot commented Feb 2, 2016

So it's a docs issue now?

On Tue, Feb 2, 2016, 11:28 AM Rick Waldron [email protected] wrote:

#258 #258


Reply to this email directly or view it on GitHub
#1026 (comment)
.

@Resseguie
Copy link
Collaborator

#258 is where it first got added, but RGB was still based off three LED instances then. Once Led.RGB became its own thing, isAnode got left as an artifact in Led

@rudacs
Copy link
Contributor

rudacs commented Oct 10, 2017

Any consensus on this item? Will the inverted be created?

@dtex
Copy link
Collaborator

dtex commented Oct 10, 2017

I propose that we deprecate isAnode on LED (warning message) and add a syncDrive device type. It'll be the first device type in LED, but that's NBD.

@stale
Copy link

stale bot commented Oct 6, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Oct 6, 2018
@dtex dtex self-assigned this Oct 6, 2018
@stale stale bot removed the stale label Oct 6, 2018
@dtex
Copy link
Collaborator

dtex commented Oct 6, 2018

I'm going to keep this issue open. There is a consensus that we need to fix it, and the fix will be easy to implement.

We do not need a device type. I was wrong, that is overkill.

Functionally the isAnode property does exactly what we need so I propose that we rename it sinkDrive, update the docs and make isAnode an alias to sinkDrive.

Easy-peasy. Sound good?

@reconbot
Copy link
Collaborator Author

reconbot commented Oct 6, 2018 via email

@stale
Copy link

stale bot commented Oct 1, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Oct 1, 2019
@dtex
Copy link
Collaborator

dtex commented Oct 1, 2019

No Stalebot!

I've learned a lot about this in the past year, and a sinkDrive property is still the right way to go.

@stale stale bot removed the stale label Oct 1, 2019
@stale
Copy link

stale bot commented Sep 27, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Sep 27, 2020
@stale stale bot closed this as completed Oct 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants