mysensors / NodeManager

Plugin for a rapid development of battery-powered sensors
130 stars 82 forks source link

SensorDigitalOutput relay trigger #475

Open Xedecimal opened 5 years ago

Xedecimal commented 5 years ago

When booting up, if there's a Low Trigger relay hooked up, it'll switch on immediately. Not good for a garage door opener. I think I found a solution for this though!

During our setup we run opener.setInvertValueToWrite(ON); to flip the config. However when calling SensorDigitalOutput::onSetup(), there's a line of code that doesn't care about that setting.

setStatus(OFF);

I think this should be more like the if (_invert_value_to_write) value = !value; in the _switchOutput method to write ON instead. Does this sound right ?

mtiutiu commented 5 years ago

@Xedecimal correct me if I'm wrong but as I see in the code setStatus calls the _switchOutput method under the hood which takes that into account. Or am I missing something here?

Even though it calls setStatus (OFF) because it's in the inverted mode it sets the output to HIGH level which must turn the relay off. So it should work correctly unless there's something else affecting the expected behavior...

Nevermind actually...my fault. You're right.

Xedecimal commented 5 years ago

I may have jumped the gun. When I disabled that line of code, the relay stopped triggering on boot. So I went and wired a few more sensors up and came back, re-uploaded the same code with another sensor enabled and the relay switched on boot again. There must be something else around here causing it... Doing a few more tests now...

Ah ha! Turns out it's a combination of that send, and the setPulseWidth(50);. This would trigger an ON, OFF (leaving it ON as it's backwards). Do we want to do the setPulseWidth in the before() method? Let me just dump some code here to make more sense.

void before()
{
    opener.setInvertValueToWrite(ON);
    opener.setPulseWidth(50);
    // Stop relay from triggering on boot.
    digitalWrite(6, HIGH);
    nodeManager.setReportIntervalSeconds(10);
    nodeManager.before();
}

This will work flawlessly! Yet if I remove that digitalWrite up there, it'll switch the relay ON on boot, leaving the relay on for a second, then turning back off after it communicates to the gateway for some weird reason. However when I send a V_STATUS 1 to it, it'll switch on/off properly in this configuration.

mtiutiu commented 5 years ago

@Xedecimal That is because of this logic from the _switchOutput method (you're using setPulseWidth which enables it):

// set the value to write. If pulse (latching relay), value is always ON, otherwise is the requested status
        int value = _pulse_width > 0 ? ON : requested_status;
        // invert the value to write if needed. E.g. if ON is received, write LOW, if OFF write HIGH
        if (_invert_value_to_write) value = !value;

So the ternary operator sets the value to ON when you are using a pulsed output and that combined with the inverted logic sets the relay input to OFF which turns it ON actually.

At least this is what I found...still it doesn't make sense to me why is it like that...

user2684 commented 5 years ago

Hi guys, to your points, yes, _switchOutput() should take it into account and based on the setInvertValueToWrite () ON/OFF is handled differently. setPulseWidth() is supposed to be used in before() since just sets a variable. When the sensor starts, it switches immediately OFF, probably I should make this optional since as you said cannot comply with all the use cases. Is there any other issue I'm missing? thanks!

Xedecimal commented 5 years ago

No other issue. I think the solution here would be a way to disable that immediate OFF setting because on start up, you may want your relay to not do anything at all until you tell it to. Also interesting, is that initial OFF even needed in any use case? I'm not sure people normally start these relays in an OFF position as it should already be off on start. Everything seems to work great for me (at least in my low activated relay situation) without that line as long as the inverse is set.

user2684 commented 5 years ago

Clear, thanks, let me add this to the development queue then. The initial use case was to implement some sort of "panic" button (this thing doesn't turn off, let me reset the board which will automatically shut it down). But there is not useful anymore since the user can just call the switch off in the code during setup if needed. Thanks

nekromant commented 3 years ago

@Xedecimal Have a look at #536 It fixes the problem for me.