Wrong LEDs inNmraDccMultiFunctionMotorDecoder
marko-pi opened this issue · 6 comments
There an error is in NmraDccMultiFunctionMotorDecoder, lines 295-296
It should be
digitalWrite(LED_PIN_FWD, newDirection ? HIGH : LOW);
digitalWrite(LED_PIN_REV, newDirection ? LOW : HIGH);
instead of
digitalWrite(LED_PIN_FWD, newDirection ? LOW : HIGH);
digitalWrite(LED_PIN_REV, newDirection ? HIGH : LOW);
Forward corresponds to newDirection == HIGH.
I would argue that either set of code can be "right" depending on how the LEDs are wired! One set of code is appropriate for "active low" LED control and the other set is appropriate for "active high" LED control. (I am too lazy to reverse-engineer the code to determine the sense of newDirection, so I cannot say for which the library's current code is appropriate!)
-
When the LED is wired from LED_PIN_* to GND, then the LED_PIN_* output must be written to HIGH to turn the LED on, and written to LOW to turn the LED off. I would call this style of output pin control "active high".
-
When the LED is wired from LED_PIN_* to VCC, then the LED_PIN_* output must be written to LOW to turn the LED on, and written to HIGH to turn the LED off. I would call this style of output pin control "active low".
Which of these two approaches is appropriate depends on how you wire your LEDs to the device pins. And the library itself cannot predict this.
Here are a couple reasons why "active low" wiring may be appropriate:
-
Locos (properly) designed for NMRA-conformance and for use as a"drop-in DCC decoder" have lamps "commoned" to a positive voltage reference from the decoder, and the various function leads are driven to "low" to light the lamps. If the function leads are directly driven by the microcontroller, "active-low" makes sense. But this limits the current of a given function to the current limit of the microcontroller, and it exposes the sensitive microcontroller output pins to ESD and other abuses by the user. In my experience (mainly with decoders no newer than 2 years), commercial decoder manufacturers tend not to control function outputs directly from microcontroller pins.
-
I know that the "active low" output type is appropriate for at least some older microcontrollers when driving function outputs directly from microcontroller pins. Those microcontrollers had insufficient output current when driven high, and would not be able to light a typical "low-efficiency" LED! For those designs providing direct drive of function outputs using microcontrollers with those limitations, the "active low" wiring and code is appropriate.
-
Even with modern microcontrollers, there can be microcontroller limitations on the total power dissipation via output pins. This issue may drive a consiencious developer to use "active low" outputs for functions.
Here are a couple of reasons why "active high" may be appropriate:
-
Locos (properly) designed for "drop-in DCC decoder installation" have lamps "commoned" to a positive voltage reference from the decoder, and the various function leads are driven to "low" to light the lamps. If the function leads are buffered on the board, it is typically done via one NPN transistor per function output, configured in the "common-emitter" mode. A high microcontroller output turns the transistor on, which turns on the function. In this case, "active high" microcontroller outputs are appropriate.
-
It is easier to understand for the novice coder/implementer.
I would argue that perhaps the code-base could/should be changed to allow for a global variable that would establish whether function outputs were "active high" or "active low".
Thank you for the reply.
I was aware that the lamps are "commoned" to a positive voltage according to NMRA. However, your sketch is intended for a microprocessor running at 5V, so with common positive at 12V the only correct way to drive LEDs is with either transistor or (as in my case) MOSFET. [This is also beneficial, as I usually put two LEDs in series to get more luminous flow for the same electrical power or the same luminous flow for the smaller electrical power.] If someone uses NPN and N-channel MOSFET, the logic of the program will be wrong. If someone used PNP or P-channel MOSFET, the logic of the program would be correct. I am not an electrical engineer, but it seems to me that you cannot use PNP or P-channel MOSFET.
I do agree with you that this is obviously not a clear-cut situation, so you can disregard my report. Following the argumentation above, I intend to use inverse logic in my sketch anyway, despite it is based on your sketch.
P.S. I have rewritten the sketch to include all NMRA recommendations (acceleration, deceleration, signal timeout, manufacturer ID, emergency stop) and added CV120 for master reset. Shall I share my sketch on my github account or contribute to NmraDcc project?
However, your sketch
The word "your" is quite ambiguous. I do not believe that I have contributed to the NmraDcc library nor its sketches. Others can claim that.
is intended for a microprocessor running at 5V
Also technically not true - the sketch is intended to be independent of microprocessor. Some users successfully use this sketch with microprocessors running at 3.3VDC, and I would not be surprised to find that some are using it with microprocessors running at 1.8VDC.
- so with common positive at 12V the only correct way to drive LEDs is with either transistor or (as in my case) MOSFET.
I would argue that "correct" is not the right word here. There are other methods. Probably the simplest method is to use a transistor or a MOSFET on each output. Some users find that integrated circuits with level-shifting "drive" transistors are a good solution. Others simply drive directly from the microcontroller output in "active low" mode and tie to +5VDC (for a non-NMRA-compliant solution). There are many ways to skin a cat, and there are many different applications, and many different users, and many different budgets.
If someone uses NPN and N-channel MOSFET, the logic of the program will be wrong.
No - there is no "right" and "wrong" here. Right/wrong depends on how the user is connecting his function loads, and the library/sketch cannot control that. But it can assume, and it has assumed a method that is opposite of what you choose to do. THAT DOES NOT make the library wrong! It does show that the library is inflexible.
I am not an electrical engineer, but it seems to me that you cannot use PNP or P-channel MOSFET.
I am an electrical engineer. It is possible to use a PNP or a P-channel MOSFET. One has to "jump through some hoops" to make it work by adding additional components. Or by using a transistor in a way that it is not very efficient. Or by paying more for more-expensive parts that suit such an application.
The logic is correct if there is no transistor between the microcontroller pin and the LED. I would argue that the vast majority of users of the library have simply connected an LED to an Arduino output pin, and it worked.
Remember, this librarary and the associated "sketches" are intended for experimenters. Users are encouraged to make use of the libraries and sketches "as-is", or to modify them to suit specific purposes.
If you have improvements to the library and/or sketches, they are welcomed.
-
Library changes are something that require careful implementation, as changes to the library could break other people's implementations if/when they update.
-
Some library changes are best made as "configurable" changes.
Your desire for a library change to invert the sense of newDirection (and presumably other funciton outputs) would probably be best done as a configurable change. There are many ways to implement a change that is "configurable", and some are "better" than others. I know that a bunch of "configurable" changes have been made to the various MRRwA libraries, and I am sure that other contributors can give specific advice. (I am not a computer scientist and do not play one on TV. But I have submitted my fair share of well-intentioned but mis-directed software changes here and in other projects, so I am not the best person to give specific advice on this kind of software change!)
- Modification to existing sketches require some thought to ensure that they do not break existing implementations. Adding functionality is a good thing; changing existing functionality usually is not.
- New sketches which do not replace existing sketches are also welcome. This might be the simplest way to include your improvements as part of MRRwA. This, of course, assumes that the features are implemented as part of the sketch, not as part of changes to the library...
Remember that I am speaking as someone who has opinions, and someone who has some amount of control of what does/does not become part of the library. Others have opinions, and have differing levels of influence on the library. I am one voice among many.
I believe that the improvements you claim are desirable features for inclusion in MRRwA! But I wish to see them included in such ways that they do not simply break other people's implementations.
- so with common positive at 12V the only correct way to drive LEDs is with either transistor or (as in my case) MOSFET.
I would argue that "correct" is not the right word here. There are other methods. Probably the simplest method is to use a transistor or a MOSFET on each output. Some users find that integrated circuits with level-shifting "drive" transistors are a good solution. Others simply drive directly from the microcontroller output in "active low" mode and tie to +5VDC (for a non-NMRA-compliant solution). There are many ways to skin a cat, and there are many different applications, and many different users, and many different budgets.
This is exactly my point: With Arduino/ATmega328/ATtiny and an NMRA-compliant solution, you can not drive directly from the microcontroller. Arguably, the simplest NMRA-compliant way (so simple that even I understand it) is to use an NPN-equivalent solution, and there the logic is exactly reversed. But as I wrote, I concede that this is not a clear-cut situation, so I give up my report/suggestion.
I was thinking of adding my new sketch to the NmraDcc library rather than replacing the existing sketch, but I see that this is so much trouble for both you and me that I'll post it as a standalone/unrelated sketch.
I've corrected this in 2.0.9, which I've just released - thanks