-
Notifications
You must be signed in to change notification settings - Fork 206
refactor user-interface to Python #144
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?
Conversation
f1ef67f to
7f1ec9c
Compare
|
Questions:
|
|
Good questions!
On first glance, it looks like, yes.
I've never seen any other value being supported anywhere. Which doesn't necessarily mean such cases don't (and will never) exist, but IMHO it would be safe enough to restrict it to those three values. |
This is the same behavior as other Python I/O stream's `fileno()` method. BREAKING CHANGE: ValueError instead of -1 if the SPI device connection is not open.
BREAKING CHANGE: Raises ValueError if mode is not between 0 and 3.
Calling `open()` without arguments can use the instance attributes `bus`
and `device` or `path` to call `open(bus, device)` or `open_path(path`
from the super class respectively. Thus the following options are
available:
```python
SpiDev().open(0, 1)
SpiDev(bus=0, device=1).open()
SpiDev(bus=1, device=1).open(0, 0) # args to open() overwrite attributes
SpiDev(path='/dev/myspi').open() # calls open_path('/dev/myspi')
```
BREAKING CHANGE: bits_per_word raises a ValueError instead of a TypeError when its value is not between 8 and 32.
Posed some issues with setting attributes of the super class and it's harder to refactor later on when C implementation will be cleaned up.
- SpiDev().open(bus, device) --> SpiDev(bus, device).open() - SpiDev().open_path(path) --> SpiDev(path=path).open() - SpiDev().readbytes() --> SpiDev().read() - SpiDev().writebytes() --> SpiDev().write() - SpiDev().writebytes2() --> SpiDev().write()
More in line with Python conventions
|
This is ready for review. I'm quite happy with it's current state; AFAIA, it is backwards compatible except some minor acceptable changes (listed above, mostly different Errors raised for certain situations). In it's current state their is quite some "boilerplate" code, e.g.: @property
def read0(self) -> bool:
"""Read 0 bytes after transfer to lower CS if cshigh is set."""
return self._cmod.read0
@read0.setter
def read0(self, value: bool, /) -> None:
self._cmod.read0 = try_convert(value, bool, "read0")This enables to lift its logic from the C module to the Python class eventually (maybe by someone more experienced with the C codebase). There are a couple TODO's in there: this is mostly parts where my knowledge is lacking, would be great to see some suggestions there. help(spidev.SpiDev) |
As I described in #128 (comment), this PR changes the project structure to have the user interfacing code mostly in a .py file while keeping the core functionality in C.
Currently, the
_spi.pyfilesubclasseswraps the SpiDev class as defined inspidev_module.c(renamed to_cspi.c). From there functionality can be gradually lifted to the new_spi.SpiDevclass.This change would allow for easier contributions from other Python programmers and clearly separates the core functionality and the Pythonic interface to it. It's also easier to include things like type hints and automatic documentation generation. This would be the first steps to a more modern and maintainable version of the current spidev package.
If someone is able to test the current initial version (@fschrempf?) and confirm it works as expected, I can continue with gradually lifting more into the new
SpiDevclass.Eventually, I think the C module doesn't have to define the
SpiDevclass anymore, but rather expose a couple of functions (likeopen()andclose()) that are called from theSpiDevclass in the python file. But my knowledge of Python C modules is limited -- need an expert opinion on this.Overview:
Added:
path: enablesopen()w.o. args instead ofopen_path().mode: no need to set mode later.bits_per_word: no need to set it later.max_speed_hz: no need to set it later.closed: IOBase, returns True if the connection is opened (fdis set).read(): IOBase, wrapsreadbytes()readable(): IOBase, same asnot self.closed.writeable(): IOBase, same asnot self.closed.write(): IOBase, wrapswritebytes2().__str__(): String representation.__repr__(): Forprint()ing.__eq__(): Compare if both instances point to the same device (simply compares the path of both)Changed:
fileno()raises a value error if the connection is not opened instead of returning -1. This is more in line with Python builtins.open()without arguments can use either bus and device attributes or the path attribute to open the connection viaopen_path().open()is removed from the C module.clientrenamed todevice.Removed / deprecated:
SpiDev().open(bus, device)→SpiDev(bus, device).open()SpiDev().open_path(path)→SpiDev(path=path).open()SpiDev().readbytes()→SpiDev().read()SpiDev().writebytes()→SpiDev().write()SpiDev().writebytes2()→SpiDev().write()Breaking Changes:
fileno()raises ValueError instead of returning -1 if file is not opened.modeproperty raises ValueError instead of TypeError when value is not between 0 and 3.bits_per_wordproperty raises ValueError instead of TypeError when value is not 8, 16 or 32.clientrenamed todevice.