-
Notifications
You must be signed in to change notification settings - Fork 2
Spin out driver subclass #4
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
base: master
Are you sure you want to change the base?
Changes from all commits
402c05c
968b8cb
80ffa73
c7b7a01
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| source 'https://rubygems.org' | ||
|
|
||
| # Specify your gem's dependencies in pi_piper-sysfs.gemspec | ||
| gemspec | ||
| gemspec |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| require "pi_piper/sysfs/version" | ||
| require "pi_piper/sysfs/sysfs" | ||
| require 'pi_piper' | ||
| require 'pi_piper/sysfs/version' | ||
| require 'pi_piper/sysfs/driver' | ||
|
|
||
| module PiPiper | ||
| self.driver= PiPiper::Sysfs | ||
| self.driver = PiPiper::Sysfs::Driver | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| module PiPiper | ||
| module Sysfs | ||
| class Driver < PiPiper::Driver | ||
|
|
||
| GPIO_HIGH = 1 | ||
| GPIO_LOW = 0 | ||
|
|
||
| def initialize | ||
| @exported_pins = Set.new | ||
| end | ||
|
|
||
| def close | ||
| unexport_all | ||
| @exported_pins.empty? | ||
| end | ||
|
|
||
| # Support GPIO pins | ||
| def pin_direction(pin, direction) | ||
| raise ArgumentError, "direction should be :in or :out" unless [:in, :out].include? direction | ||
| export(pin) | ||
| raise RuntimeError, "Pin #{pin} not exported" unless exported?(pin) | ||
| File.write(direction_file(pin), direction) | ||
| end | ||
|
|
||
| def pin_read(pin) | ||
| raise ArgumentError, "Pin #{pin} not exported" unless exported?(pin) | ||
| File.read(value_file(pin)).to_i | ||
| end | ||
|
|
||
| def pin_write(pin, value) | ||
| raise ArgumentError, "value should be GPIO_HIGH or GPIO_LOW" unless [GPIO_LOW, GPIO_HIGH].include? value | ||
| raise ArgumentError, "Pin #{pin} not exported" unless exported?(pin) | ||
| File.write(value_file(pin), value) | ||
| end | ||
|
|
||
| def pin_set_pud(pin, value) | ||
| raise NotImplementedError, "Pull up/down not available with this driver. keep it on :off" unless value == :off | ||
| end | ||
|
|
||
| def pin_set_trigger(pin, trigger) | ||
| raise ArgumentError, "trigger should be :falling, :rising, :both or :none" unless [:falling, :rising, :both, :none].include? trigger | ||
| raise ArgumentError, "Pin #{pin} not exported" unless exported?(pin) | ||
| File.write(edge_file(pin), trigger) | ||
| end | ||
|
|
||
| def pin_wait_for(pin, trigger) | ||
| pin_set_trigger(pin, trigger) | ||
| fd = File.open(value_file(pin), 'r') | ||
| value = nil | ||
|
|
||
| loop do | ||
| fd.read | ||
| IO.select(nil, nil, [fd], nil) | ||
| last_value = value | ||
| value = self.pin_read(pin) | ||
| if last_value != value | ||
| next if trigger == :rising and value == 0 | ||
| next if trigger == :falling and value == 1 | ||
| break | ||
| end | ||
| end | ||
|
|
||
| end | ||
|
|
||
| # Specific behaviours | ||
|
|
||
| def unexport(pin) | ||
| File.write("/sys/class/gpio/unexport", pin) | ||
| @exported_pins.delete(pin) | ||
| end | ||
|
|
||
| def unexport_all | ||
| @exported_pins.dup.each { |pin| unexport(pin) } | ||
| end | ||
|
|
||
| def exported?(pin) | ||
| @exported_pins.include?(pin) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def export(pin) | ||
| raise RuntimeError, "pin #{pin} is already reserved by another Pin instance" if @exported_pins.include?(pin) | ||
| File.write("/sys/class/gpio/export", pin) | ||
| @exported_pins << pin | ||
| end | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. following methods are not useful.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But I won't make a case for it !
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure I can see your point. I was just thinking along 2 points. Anything called twice or more should be in a method. And second, we could stub out the method to test another file when writing tests. |
||
| def value_file(pin) | ||
| "/sys/class/gpio/gpio#{pin}/value" | ||
| end | ||
|
|
||
| def edge_file(pin) | ||
| "/sys/class/gpio/gpio#{pin}/edge" | ||
| end | ||
|
|
||
| def direction_file(pin) | ||
| "/sys/class/gpio/gpio#{pin}/direction" | ||
| end | ||
|
|
||
| def pin_value_changed?(pin, trigger, value) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see above
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. where do you use this method, I can't see. maybe we can halt on this until we sort the #pin_wait_for issue
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah this was actually leftover from my implementation of |
||
| last_value = value | ||
| value = pin_read(pin) | ||
| return false if value == last_value | ||
| return false if trigger == :rising && value == 0 | ||
| return false if trigger == :falling && value == 1 | ||
| true | ||
| end | ||
| end | ||
| end | ||
| end | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| module PiPiper | ||
| class Driver; end | ||
| class Sysfs < Driver | ||
| VERSION = "0.1.0" | ||
| module Sysfs | ||
| VERSION = '0.1.0' | ||
| end | ||
| end |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not aware of any CPU usage issue with the previous implementation, it was backed by a unix system call
select()which is widely used since august 1984... and worked pretty well since I used PiPiper also (maybe a year).can you please explain me a bit more the issue ?
on your implementation : This need to be discussed ; your are watching the value_file and not the edge_file, why ? Is it wise to add a new dependecy to the gem ? FileWatcher seems to use File#mtime to check if a file is changed, is it reliable ? nothing in the gpio kernel module is said about dates of the value or edge files.
Let's talk.
EDIT : My bad select(2) must watch the value_file obviously !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I thought the original implementation watched the value file and compared whether the value changed or not? And the reason I implemented it this way is because the original implementation actually spins at 100%. I verified this by running the original method manually in an
irbsession. I can double check this again but that was my inspiration. I agree about adding another gem, I wasn't too keen on it but I had difficulty finding some sample code that would block the script until a file write. This gem apparently does it and seems to work well. We should definitely run it through some stuff to verify performance though.But basically that is what my thinking was. It actually doesn't use File#mtime but rather listens for kernel events. Having said that I did not look at the source code in great detail but the readme mentioned that part. If that is the case then we ultimately hit the kernel anyway so this method may be more robust. I know every OS has a "file system watcher" event handler so hopefully this one is using one in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some stackoverflow questions I was researching when testing the original implementation:
http://stackoverflow.com/questions/11367719/why-is-my-ruby-script-utilizing-90-of-my-cpu
http://linux.die.net/man/2/select
(there are a couple more, I will have to re-search them on google but will link them once I get home)
Apparently
selectdoes not block because thereaddoes not block when atEOFbut rather returnsnil. This is why it consumes the CPU. If you look at the man page for select it even states that it will only block if the read blocks, this is not the case for us. Read will always succeed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @zsyed91,
I tried to produce what you were speeking about. see the gist I wrote to do it :
https://gist.github.com/elmatou/4fd0edb39ea278c465b211ddbb773f45
While I was watching the cpu usage in another ssh session :
grep 'cpu ' /proc/stat | awk '{usage=($2+$4)*100/($2+$4+$5)} END {print usage "%"}'It happens I always get something around 11.4% more or less....
Of course I'm looking at the edge file, which is not the same thing the value file which cannot have an EOF (it is updated on every clock round).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that @jwhitehorn implementation look at the value_file as mine look at the edge_file, with which one did you hit 100% ?