Add feature to disable signal echo at runtime. #21
Conversation
| @@ -154,6 +154,7 @@ ESPiLight::ESPiLight(int8_t outputPin) { | |||
| _outputPin = outputPin; | |||
| _callback = nullptr; | |||
| _rawCallback = nullptr; | |||
| _echoEnabled = true; | |||
There was a problem hiding this comment.
Would be fine to have it disable by default. It is basically for debugging only.
janLo
force-pushed
the
disable-signal-echo
branch
from
989fe85
to
c97176c
Compare
| /** | ||
| * If set to true, the receiver will temporarely be disabled when sending. | ||
| */ | ||
| void setEchoEnabled(bool enabled); |
There was a problem hiding this comment.
Arduino Style Guide for Writing Libraries says:
Try to avoid boolean arguments. Instead, consider providing two different functions with names the describe the differences between them.
How about: enableEcho() and diableEcho()
There was a problem hiding this comment.
Well, I use this from a config-vaiable that is set to true or false. So for me a setter would be more convinient. I think thats the main reason why a boolean argument might be OK here: Its a setter.
There was a problem hiding this comment.
I see that it is more convenient for you, but the Style guide is clear in this case.
There was a problem hiding this comment.
I think what the style guide means here is that you should not use boolean flags to influence the behaviour of a method. For example instead of
void connect(const String& host, const int port, const bool use_udp) {
...
if (use_udp) {
...
}
}
write
void connectTcp(const String& host, const int port) {
...
}
void connectUdp(const String& host, const int port) {
...
}
Which comes from "Robert C. Martin's Clean Code Tip #12: Eliminate Boolean Arguments" wwich states, that a boolean flag is a sign that a function does more than one thing. As we actually do nothing here than setting a configuration value I would say its better that way.
There was a problem hiding this comment.
Did you have any example of an popular lib that use boolean setter?
There was a problem hiding this comment.
As I just had the code open:
- https://github.com/tzapu/WiFiManager/blob/master/WiFiManager.h#L111
- https://github.com/tzapu/WiFiManager/blob/master/WiFiManager.h#L91
- https://github.com/esp8266/Arduino/blob/master/libraries/ESP8266WiFi/src/ESP8266WiFiGeneric.h#L77
- https://github.com/esp8266/Arduino/blob/master/libraries/ESP8266WiFi/src/ESP8266WiFiGeneric.h#L82-#L83
- https://github.com/esp8266/Arduino/blob/master/libraries/ESP8266WiFi/src/ESP8266WiFiSTA.h#L52
janLo
force-pushed
the
disable-signal-echo
branch
from
c97176c
to
6d6e7ca
Compare
|
Rebased before the API changes |
This adds ESPiLight::setEchoEnabled(bool) to control if the receiver should be temporary switched off while sending. Signed-off-by: Jan Losinski <losinski@wh2.tu-dresden.de>
janLo
force-pushed
the
disable-signal-echo
branch
from
6d6e7ca
to
c89a0d1
Compare
This adds ESPiLight::setEchoEnabled(bool) to control if the receiver
should be temporary switched off while sending.