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

Fleshed out #1

Open
wants to merge 5 commits into
base: master
from

Conversation

@dastels
Copy link

dastels commented Feb 2, 2019

No description provided.

Copy link
Member

kattni left a comment

Thanks for writing this!

There's a few issues here, and I'm not sure if they're a cookiecutter failure, a failure on our part to explain cookiecutter's updates clearly, or an issue with what you've entered into cookiecutter.

  • The .files (dot files) are missing, which is hopefully something you missed and not that cookiecutter stopped generating them. Travis isn't running on this because there's no .travis.yml file present.
  • The module file name is adafruit_circuitpython_bitbangio and it should be adafruit_bitbangio. Only the repo and the PyPI info should have the full repo-style name in them.
  • Due to the above, there are multiple places where it says adafruit_circuitpython_bitbangio and it should say adafruit_bitbangio instead (aside from the module file name itself)
  • There are still todo's present in multiple files. I won't put details here as they each explain what needs to be done. Do a search for todo and you'll find them.

It's late and I haven't looked at much past the cookiecutter issues. This needs to be gone through more thoroughly to make sure that nothing else was missed. Travis will fail in the current state FYI.

Redoing cookiecutter properly and then shifting all the code into where it goes may be the most thorough way to resolve this, however that is a lot of work, and it may also be possible to fix the issues I've found without redoing cookiecutter. I would need more time to go through it and make sure I've located all the things that may be a problem.

dastels added 2 commits Feb 2, 2019
Copy link

sommersoft left a comment

I went ahead and denoted the additional areas that need the name changes @kattni mentioned. Like she said, it may save time to just run the cookiecutter again and move over the library files. And please, please, please let us know if we can tweak the "new" process to make it better/easier.

With regard to the bme280_bbspi examples, I understand that the current library needed to be changed since adafruit_bus_device is hardcoded in. However, I don't think it serves as a good example, nor should it require a user (or anyone else) to re-write and maintain a "bitbang" version of the library. I don't have a full solution in my mind how this can be addressed across the library landscape. Probably a good thing to bring up In The Weeds during a meeting.

I'm also thinking that SPI and SPIDevice can be sub/super classed, but that may just be a preference.

Looks good though! And, I imagine it was a fun one to work out.. 😄

.travis.yml Outdated
- pip install --force-reinstall pylint==1.9.2

script:
- pylint adafruit_circuitpython_bitbangio.py

This comment has been minimized.

Copy link
@sommersoft

sommersoft Feb 3, 2019

change to - pylint adafruit_bitbangio.py

This comment has been minimized.

Copy link
@dastels

dastels Feb 4, 2019

Author

Agreed. It isn't a great example (not very simple), but it's what I had... I used it as a case to test the implementation.

Also agreed about rewriting. The existing drivers weren't written with the idea of swappable SPI implementations in mind. Maybe a slight rearchitecting is in order to make it all more modular?

.. If your library file(s) are nested in a directory (e.g. /adafruit_foo/foo.py)
.. use this format as the module name: "adafruit_foo.foo"
.. automodule:: adafruit_circuitpython_bitbangio

This comment has been minimized.

Copy link
@sommersoft

sommersoft Feb 3, 2019

change to .. automodule:: adafruit_bitbangio

master_doc = 'index'

# General information about the project.
project = u'Adafruit_circuitpython BitBangIO Library'

This comment has been minimized.

Copy link
@sommersoft

sommersoft Feb 3, 2019

change to project = u'Adafruit BitBangIO Library'

# (source start file, target name, title,
# author, documentclass [howto, manual, or own class]).
latex_documents = [
(master_doc, 'Adafruit_circuitpythonBitBangIOLibrary.tex', u'Adafruit_circuitpythonBitBangIO Library Documentation',

This comment has been minimized.

Copy link
@sommersoft

sommersoft Feb 3, 2019

change to (master_doc, 'AdafruitBitBangIOLibrary.tex', u'Adafruit BitBangIO Library Documentation',

texinfo_documents = [
(master_doc, 'Adafruit_circuitpythonBitBangIOLibrary', u'Adafruit_circuitpython BitBangIO Library Documentation',
author, 'Adafruit_circuitpythonBitBangIOLibrary', 'One line description of project.',
'Miscellaneous'),

This comment has been minimized.

Copy link
@sommersoft

sommersoft Feb 3, 2019

change to:

(master_doc, 'AdafruitBitBangIOLibrary', u'Adafruit BitBangIO Library Documentation', 
     author, 'AdafruitBitBangIOLibrary', 'One line description of project.', 
     'Miscellaneous'),
# One entry per manual page. List of tuples
# (source start file, name, description, authors, manual section).
man_pages = [
(master_doc, 'Adafruit_circuitpythonBitBangIOlibrary', u'Adafruit_circuitpython BitBangIO Library Documentation',

This comment has been minimized.

Copy link
@sommersoft

sommersoft Feb 3, 2019

change to (master_doc, 'AdafruitBitBangIOlibrary', u'Adafruit BitBangIO Library Documentation',

Ensure your device works with this simple test.

.. literalinclude:: ../examples/bitbangio_simpletest.py
:caption: examples/bitbangio_simpletest.py

This comment has been minimized.

Copy link
@sommersoft

sommersoft Feb 3, 2019

there is no examples/bitbangio_simpletest.py file...

@sommersoft

This comment has been minimized.

Copy link

sommersoft commented Feb 3, 2019

argh! A few of my comments dropped. Let's see if I can remember them. 🤦‍♂️

Copy link

sommersoft left a comment

Missing Comments Found!


def __init__(self, clock=None, MOSI=None, MISO=None):
if clock is None or MOSI is None or MISO is None:
return None

This comment has been minimized.

Copy link
@sommersoft

sommersoft Feb 3, 2019

Would it be better to raise an exception here? Returning None still creates the SPI object, but doesn't initialize anything. Further, SPIDevice doesn't check for a "valid" SPI object (i.e. not None) and continues on to initialize cs and spi.configure.

I recommend raising ValueError, denoting all 3 pins are required.

Example of what I'm saying:

Python 3.6.2 (v3.6.2:5fd33b5, Jul  8 2017, 04:57:36) [MSC v.1900 64 bit (AMD64)] on win32
Type "copyright", "credits" or "license()" for more information.
>>> class SPI(object):
	def __init__(self, clock=None, MOSI=None):
		if clock is None or MOSI is None:
			return None
		self._clock = clock
		self._MOSI = MOSI

	def values(self):
		return self._clock, self._MOSI

>>> class SPIDevice(object):
	def __init__(self, spi):
		self._spi = spi

	def spi(self):
		return self._spi

>>> x = SPI()
>>> y = SPIDevice(x)
>>> y.spi
<__main__.SPI object at 0x000001EA656A6E48>
>>> y.spi.values
Traceback (most recent call last):
  File "<pyshell#13>", line 1, in <module>
    y.spi.values
  File "<pyshell#2>", line 9, in values
    return self._clock, self._MOSI
AttributeError: 'SPI' object has no attribute '_clock'
>>> 

This comment has been minimized.

Copy link
@dastels

def unlock(self):
"""Releases the SPI lock."""
pass

This comment has been minimized.

Copy link
@sommersoft

sommersoft Feb 3, 2019

Is there a way to utilize Blinka's Lockable class for try_lock and unlock? I have zero experience with Blinka yet, so this is more of a question than a suggestion.

@@ -0,0 +1,198 @@
import time
from adafruit_bitbangio import SPIDevice

This comment has been minimized.

Copy link
@ladyada

ladyada Feb 3, 2019

Member

we shouldn't need this right? if bitbangio.SPI replaces only busio.SPI then you should be able to swap in either

This comment has been minimized.

Copy link
@dastels

dastels Feb 3, 2019

Author

"import adafruit_bus_device.spi_device as spi_device"

is hardcoded into Adafruit_CircuitPython_BME280/adafruit_bme280.py : Adafruit_BME280_SPI

This comment has been minimized.

Copy link
@dastels

dastels Feb 3, 2019

Author

I suppose we could subclass Adafruit_CircuitPython_BME280/adafruit_bme280.py : Adafruit_BME280 with something that works with the bitbanged support.

This comment has been minimized.

Copy link
@ladyada

ladyada Feb 3, 2019

Member

why would you have to? bitbangio.SPI should be completely identical API to the hardware SPI - whats different?

This comment has been minimized.

Copy link
@ladyada

ladyada Feb 3, 2019

Member

(we pass SPI into spi_device - the spi_device part doesnt do any hardware SPI stuff)

@dastels

This comment has been minimized.

Copy link
Author

dastels commented Feb 4, 2019

Here's my thinking after poking around a bit.
Using adafruit_bus_device:SPIDevice looks like it should work. I'll give it a try next.
Adafruit_BME280:BME280 only supports I2C.
I was using a BME280 class based on the circuitpython version, which subclasses for I2C and SPI. That wasn't working and I eventually tracked it down to it's _read_register method which does a write of the register number, followed by a readinto of the data. Changing that to use write_readinto to do everythign in one transaction worked.

@kattni kattni referenced this pull request Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.