Skip to content
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

NewRelayDriver should allow for creating inverted relay #648

Closed
andrewebdev opened this issue Dec 31, 2018 · 9 comments
Closed

NewRelayDriver should allow for creating inverted relay #648

andrewebdev opened this issue Dec 31, 2018 · 9 comments

Comments

@andrewebdev
Copy link
Contributor

Currently .On() sets the relay state to high, and .Off() sets the relay state to low.

We have realays boards in one of our projects that are actually inverted.
This causes us to do things in the code like relayDriver.On() when we want it switched off, and of course it leads to some confusion down the line.

It may be a good idea for NewRelayDriver to allow for this use case?

@deadprogram
Copy link
Member

Seems like a good idea. A PR that added this would be gladly accepted.

@andrewebdev
Copy link
Contributor Author

@deadprogram just a ping to remind that I have done a PR for this. (just in case this issue was overlooked)

andrewebdev added a commit to andrewebdev/gobot that referenced this issue Jun 20, 2019
@kneerunjun
Copy link

func (l *RelayDriver) On() (err error) {
	if err = l.connection.DigitalWrite(l.Pin(), 1); err != nil {
		return
	}

	if l.Inverted {
		l.high = false
	} else {
		l.high = true
	}

	return
}

I notice you are still writing 1 to DigitalWriter (Outside the Inverted if logic) in Driver.On - this is hardly helping. Most of the relays close NO-C on LOW ! Wouldn't it help to check Inverted before you DigitalWrite 1.

@stuartleeks
Copy link
Contributor

As @kneerunjun noted, On() always writes 1 and Off() always write 0. I expected inverted to invert the value that was written as well. Unfortunately this means that I'm still having to use Off() to turn on the component attached to the relay.

Would you accept a PR that takes the Inverted field into account when writing?

@deadprogram
Copy link
Member

Yes, please, and thanks!

@stuartleeks
Copy link
Contributor

Great :-)

@andrewebdev
Copy link
Contributor Author

@stuartleeks Thank for this. I saw this email yesterday and was going to start updating this today; Then I noticed you already submitted a pull request :)

Not sure how I missed that it still writes 1 instead of 0. Our code that uses this functionality is currently still doing the old manual if-check, so we never actually caught this bug in our code.

Good job catching it!

@stuartleeks
Copy link
Contributor

Thanks @andrewebdev. I was working on a home project with my raspberry pi and thought I'd misunderstood gobot or mis-wired my relay. After checking my circuits and code I started digging in to the gobot code and found what @kneerunjun described which led me to this issue.

Hopefully we can get the PR merged and the change released soon, and then I can use the Inverted setting and remove the big NOTE comment I have in my code explaining the use of Off() for On() 😁

@gen2thomas
Copy link
Collaborator

Seems to be fixed. Otherwise please try "WithPinActiveLow()", available on "dev" branch.

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

5 participants