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

Merged in tjanson's work and overloaded write/read methods with options object #37

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

risingtiger
Copy link

I’ve created a rather simple method called writeRegardless (open to name suggestions here) that allows for writing to a pin without knowing whether its opened and set to the ‘out' direction. The method simply opens the pin if not opened already and sets the direction to out.

My argument for adding this is that there are legitimate cases where a script won’t know before hand which pins will be used. And I believe the logic to detect whether open or not should be scoped to the pi-gpio library, not the script referencing it.

I’d be curious to hear other’s remarks about this. I do understand that there is overhead with running testing logic, thats why it wouldn’t be a replacement for write, but more of a convenience method in the particular cases where it makes sense.

@tjanson
Copy link
Collaborator

tjanson commented May 4, 2015

I’ve implemented the same functionality (among other things) in #27 (I called it "automatic export", see the README of that branch). Which hasn’t been merged yet – probably because no one’s had the time to test it thoroughly.

That doesn’t mean that we shouldn’t merge yours in the meantime though.

One thing though: You changed the README and removed all its contents – instead of that, you should document the additional functions in the existing README.

@risingtiger
Copy link
Author

Thanks for the heads up tjanson. I'll reup the README, double check the code again, then we can merge.

My thoughts are having a separate method, keeping the original write method as is, because of the additional overhead. For time sensitive applications such as robotics this could prove worth it.

What are your thoughts? Think there really isn't an appreciable performance hit in the "automatic export" vs straight write?

And will there be cases when a developer may not want auto open every time a write is called?

@tjanson
Copy link
Collaborator

tjanson commented May 4, 2015

If I remember correctly (this was ~8 months ago), the automatic export was optional, enabled by a passing a boolean flag to the write method – so, effectively, the same thing as having a separate, new method; giving the programmer full control in the choice of comfort vs speed.

@rakeshpai
Copy link
Owner

+1 for overloading the existing .write method. Not a huge fan of different functions with nearly the same functionality. The overhead will only be of the 'if' statement that checks whether the 'autoOpen' (or whatever) argument is true or not. That'll be a negligible delay compared to the delay that Linux itself introduces. The subsequent performance lag of having to check the pin's state is opt-in, so it should be ok I guess.

That said, I don't think it should be a boolean argument, because it's very hard to tell by looking at the caller what the boolean is for. ie., .write(16, 1, true, cb) is rather confusing without documentation. Also, if we want to expand it further in the future, it gets really messy.

Any suggestions for an alternative function signature? Considering consistency with other functions, I can only come up with .write(pinNumber, {value: 1, open: true}, cb), but this feels strange. Any suggestions? (It's worth noting that this change will be permanent, since people will write code for the new signature once it's in the readme, and we can't break people's code.)

Or am I all wrong, and it's just simpler to have a boolean argument? :)

@risingtiger
Copy link
Author

I hear you. Wrapping it into one function makes more sense. And once again, I agree that passing a boolean is frustrating and not at all self documenting. However, I would also find it frustrating having to wrap value as a key within an object, when I assume most people most the times will just want to just pass the basic: pin and value (and maybe cb).

How about keeping the format intact as it is now: .write(pinNumber, 1, cb). But if the third parameter is an object vs a function then parse that for further options. Then it would like: .write(pinNumber, 1, {autoOpen: true}, cb). This also leaves plenty of room for further expansion.

@rakeshpai
Copy link
Owner

Sorry, I somehow seem to have missed the issue @tjanson referenced above. The "automatic" approach in #27 seems pretty good too, isn't it? I don't know why I missed it before.

@risingtiger
Copy link
Author

I'm in favor of merging tjansons work. I'll do some testing today of his branch, see if anything breaks.

@risingtiger
Copy link
Author

tjanson's branch seems to be working so far on Model B+, Revision: 1.2...

@risingtiger risingtiger changed the title Add method to write to pin regardless of whether its opened or not Merged in tjanson's work and overloaded write/read methods with options object May 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants