Change GpioDriver to be an interface and inital implementation of UnixDriver #23
Conversation
|
cc: @JohnTasler |
| { | ||
| throw new NotImplementedException(); | ||
| string pinPath = $"{_gpioBasePath}/gpio{pinNumber}"; | ||
| if (Directory.Exists(pinPath)) |
There was a problem hiding this comment.
should you throw an exception when directory doesn't exist?
There was a problem hiding this comment.
I rather this be a no-op in case two different controller instances want to close the same pin at the same time. If there is only one controller, then the controller itself won't permit this to happen.
There was a problem hiding this comment.
I think at least a comment explaining this is ok
| } | ||
|
|
||
| protected internal override void OpenPin(int pinNumber) | ||
| public PinValue Read(int pinNumber) |
There was a problem hiding this comment.
I'd personally prefer PinValue to be bool unless we plan to support undefined states
There was a problem hiding this comment.
I thought of that too, but for now I went to what other frameworks do which is to define the enum. We can discuss this in tomorrow's Api Review.
There was a problem hiding this comment.
I think the enumeration is better, because what does true mean: high or low (current value), rising or falling (event fired).
In reply to: 230099441 [](ancestors = 230099441)
| { | ||
| throw new GpioException("Reading a pin value requires root permissions."); | ||
| } | ||
| } |
There was a problem hiding this comment.
The controller should take care of this by not letting you write to a pin that is not opened. I guess you could run into this in case there are two separate instances of the controller, but I wonder if we should guard against that.
There was a problem hiding this comment.
a comment should suffice so that someone else doesn't add it
| { | ||
| while (_exportedPins.Count > 0) | ||
| { | ||
| ClosePin(_exportedPins.FirstOrDefault()); |
There was a problem hiding this comment.
That is effectively what this loop will do.
| catch (UnauthorizedAccessException) | ||
| { | ||
| throw new GpioException("Reading a pin value requires root permissions."); | ||
| } |
There was a problem hiding this comment.
Just for good measure, you should store the UnauthorizedAccessException in the InnerException or GpioException.
There was a problem hiding this comment.
makes sense, I'll do that.
| break; | ||
| default: | ||
| throw new ArgumentException($"Invalid pin value {value}"); | ||
| } |
There was a problem hiding this comment.
Also pass in the argument name: nameof(value)
| } | ||
|
|
||
| protected internal override void Write(int pinNumber, PinValue value) | ||
| public bool isPinModeSupported(int pinNumber, PinMode mode) |
There was a problem hiding this comment.
i [](start = 20, length = 1)
i should be I
Cleanup ubuntu/16
These changes will: