added pylint to project and update code #10

mrmcwethy · 2017-12-12T19:53:29Z

Lint all the libraries! #475

Not tested

mrmcwethy · 2017-12-13T13:35:05Z

#adafruit/circuitpython#475

kattni

Started testing library. ST7735R fails.

kattni · 2017-12-22T05:29:48Z

adafruit_rgb_display/st7735.py

-    def __init__(self, spi, dc, cs, rst=None, width=128, height=128):
-        super().__init__(spi, dc, cs, rst, width, height)
+    #    def __init__(self, spi, dc, cs, rst=None, width=1, height=1, baudrate=1000000,
+    #                 polarity=0, phase=0)


 class ST7735R(ST7735):


Tested with ST7735R display, shows bars and multicolored pixellation across the display. Tried with example code included at beginning of file, as well as other code. Both of my examples run with the current version (if I change the last line to self.write in the current version as well) - using PR version of rgb.py.

kattni

SSD1351 testing failed.

kattni · 2017-12-22T05:49:17Z

adafruit_rgb_display/ssd1351.py

@@ -1,5 +1,6 @@
+"""A simple driver for the SSD1351-based displays."""


Testing with SSD1351 display fails - results in static looking pixellation using example in code.. Tested successfully with current version of this file with same example - using PR version of rgb.py.

@kattni thanks for testing this. I trusted pylint too much and deleted some important __init__ code. put it back in the two you have identified and several others.

Also notice the internal identifiers were only in the rgb.py file but also should be in all .py files. So I added those required lines.

Since travis checks were failing, i fixed some more error that my pylint did not find.

mrmcwethy · 2017-12-22T13:01:27Z

Ready to re-review.

deshipu · 2017-12-23T01:17:02Z

adafruit_rgb_display/hx8353.py


    >>> import busio
    >>> import digitalio
    >>> import board
    >>> from adafruit_rgb_display import color565
    >>> import adafruit_rgb_display.hx8353 as hx8353
    >>> spi = busio.SPI(clock=board.SCK, MOSI=board.MOSI, MISO=board.MISO)
-    >>> display = hx8353.HX8383(spi, cs=digitalio.DigitalInOut(board.GPIO0), dc=digitalio.DigitalInOut(board.GPIO15))
+    >>> display = hx8353.HX8383(spi, cs=digitalio.DigitalInOut(board.GPIO0),
+        dc=digitalio.DigitalInOut(board.GPIO15))


missing "..."

deshipu · 2017-12-23T01:17:44Z

adafruit_rgb_display/ili9341.py

@@ -1,10 +1,15 @@
+"""A simple driver for the ILI9341/ILI9340-based displays."""
+
 try:
    import struct
 except ImportError:
    import ustruct as struct


This can be removed, CircuitPython always has "struct".

Argh, sorry, old CircuitPython still uses ustruct.

got ya, ignored

deshipu · 2017-12-23T01:18:05Z

adafruit_rgb_display/ili9341.py

@@ -16,7 +21,8 @@ class ILI9341(DisplaySPI):
    >>> from adafruit_rgb_display import color565
    >>> import adafruit_rgb_display.ili9341 as ili9341
    >>> spi = busio.SPI(clock=board.SCK, MOSI=board.MOSI, MISO=board.MISO)
-    >>> display = ili9341.ILI9341(spi, cs=digitalio.DigitalInOut(board.GPIO0), dc=digitalio.DigitalInOut(board.GPIO15))
+    >>> display = ili9341.ILI9341(spi, cs=digitalio.DigitalInOut(board.GPIO0),
+        dc=digitalio.DigitalInOut(board.GPIO15))


Missing "..."

deshipu · 2017-12-23T01:20:17Z

adafruit_rgb_display/ili9341.py


-    def scroll(self, dy=None):
-        if dy is None:
+    def scroll(self, delta_y=None):


This is "dy" not "delta y" or "δy". It's a common notation in math.

reverted to dy

deshipu · 2017-12-23T01:21:22Z

adafruit_rgb_display/rgb.py

        pass

    def high(self):
+        """Dummy high Pin method"""
        pass


This whole class needs to be rewritten for CircuitPython, as it has a completely different gpio pin API.

I am inclined to remove dummy pin for now.

We probably should move it to simpleio, but that would be a separate pull request. Please ignore this comment for now.

deshipu · 2017-12-23T01:25:14Z

adafruit_rgb_display/s6d02a1.py


    >>> import busio
    >>> import digitalio
    >>> import board
    >>> from adafruit_rgb_display import color565
    >>> import adafruit_rgb_display.s6d02a1 as s6d02a1
    >>> spi = busio.SPI(clock=board.SCK, MOSI=board.MOSI, MISO=board.MISO)
-    >>> display = s6d02a1.S6D02A1(spi, cs=digitalio.DigitalInOut(board.GPIO0), dc=digitalio.DigitalInOut(board.GPIO15), rst=digitalio.DigitalInOut(board.GPIO16))
+    >>> display = s6d02a1.S6D02A1(spi, cs=digitalio.DigitalInOut(board.GPIO0),
+        dc=digitalio.DigitalInOut(board.GPIO15), rst=digitalio.DigitalInOut(board.GPIO16))


Missing "..."

deshipu · 2017-12-23T01:25:27Z

adafruit_rgb_display/ssd1331.py

@@ -43,7 +48,8 @@ class SSD1331(DisplaySPI):
    >>> from adafruit_rgb_display import color565
    >>> import adafruit_rgb_display.ssd1331 as ssd1331
    >>> spi = busio.SPI(clock=board.SCK, MOSI=board.MOSI, MISO=board.MISO)
-    >>> display = ssd1331.SSD1331(spi, cs=digitalio.DigitalInOut(board.GPIO0), dc=digitalio.DigitalInOut(board.GPIO15), rst=digitalio.DigitalInOut(board.GPIO16))
+    >>> display = ssd1331.SSD1331(spi, cs=digitalio.DigitalInOut(board.GPIO0),
+        dc=digitalio.DigitalInOut(board.GPIO15), rst=digitalio.DigitalInOut(board.GPIO16))


Missing "...".

deshipu · 2017-12-23T01:25:57Z

adafruit_rgb_display/ssd1331.py

+        # (_CONTRASTA, b'\x91'), #//0xEF - 0x91
+        # (_CONTRASTB, b'\x50'), #;//0x11 - 0x50
+        # (_CONTRASTC, b'\x7d'), #;//0x48 - 0x7D
+        # (_DISPLAYON, b''),


DISPLAYON is needed.

added _DISPLAYON

deshipu · 2017-12-23T01:26:19Z

adafruit_rgb_display/ssd1351.py

@@ -44,7 +48,8 @@ class SSD1351(DisplaySPI):
    >>> from adafruit_rgb_display import color565
    >>> import adafruit_rgb_display.ssd1351 as ssd1351
    >>> spi = busio.SPI(clock=board.SCK, MOSI=board.MOSI, MISO=board.MISO)
-    >>> display = ssd1351.SSD1351(spi, cs=digitalio.DigitalInOut(board.GPIO0), dc=digitalio.DigitalInOut(board.GPIO15), rst=digitalio.DigitalInOut(board.GPIO16))
+    >>> display = ssd1351.SSD1351(spi, cs=digitalio.DigitalInOut(board.GPIO0),
+        dc=digitalio.DigitalInOut(board.GPIO15), rst=digitalio.DigitalInOut(board.GPIO16))


Missing "..."

deshipu · 2017-12-23T01:26:44Z

adafruit_rgb_display/st7735.py

@@ -62,7 +67,8 @@ class ST7735(DisplaySPI):
    >>> from adafruit_rgb_display import color565
    >>> import adafruit_rgb_display.st7735 as st7735
    >>> spi = busio.SPI(clock=board.SCK, MOSI=board.MOSI, MISO=board.MISO)
-    >>> display = st7735.ST7735(spi, cs=digitalio.DigitalInOut(board.GPIO0), dc=digitalio.DigitalInOut(board.GPIO15), rst=digitalio.DigitalInOut(board.GPIO16))
+    >>> display = st7735.ST7735(spi, cs=digitalio.DigitalInOut(board.GPIO0),
+        dc=digitalio.DigitalInOut(board.GPIO15), rst=digitalio.DigitalInOut(board.GPIO16))


Missing "..."

mrmcwethy · 2017-12-23T15:15:44Z

ok, i am done with the 2nd round of testing. does the _DISPLAYON missing explain @kattni problem with drivers?

deshipu · 2017-12-23T17:57:52Z

adafruit_rgb_display/rgb.py

@@ -70,9 +70,9 @@ def _block(self, xpos0, ypos0, xpos1, ypos1, data=None):
        return None
    #pylint: enable-msg=too-many-arguments

-    def _encode_pos(self, avalue, bvalue):
+    def _encode_pos(self, x, y): #pylint: disable-msg=invalid-name


I had the impression that pylint has those names added to exceptions in its configuration.

I thought so too: https://github.com/adafruit/cookiecutter-adafruit-circuitpython/blob/master/%7B%7B%20cookiecutter.library_name%20%7D%7D/.pylintrc#L303

That change got made after i created my template project from cookiecutter. I have updated the .pylintrc and remove the redundant #pylint lines

deshipu · 2017-12-23T18:00:18Z

adafruit_rgb_display/__init__.py

@@ -1 +1,2 @@
+"""import class color565"""


This string here is a bit confusing. First, it's a module-level docstring, so it should be describing the module, rather than commenting the code. Second, color565 is a function, not a class. And finally I'm not sure we need to explain what the "import" statement does.

deshipu · 2017-12-23T18:03:11Z

adafruit_rgb_display/rgb.py

+
+    def read(self, command, count):
+        """Derived class must implement this"""
+        raise NotImplementedError


I would advise against dummy methods raising NotImplementedError. Not only they waste memory, but they also make it very hard to use super() properly. I think that AttributeError in case something tries to call this is good enough.

I believe pylint has smarts around recognizing unimplemented methods like this and validating that suubclasses define them. The memory hit won't be so large that it isn't worth it.

The real problem is when you use this class in any multiple inheritance scenario, where you have to use super() to make sure all the methods are properly called, but this will also call this method and raise a NotImplementedError. That's why this is an anti-pattern. I understand that with CircuitPython we run a much lower risk for this, but it's still a bad habit.

Oh, interesting! What would you recommend in this case? Would you mind opening an issue for us to make the drivers consistent on how to do this?

The way to do this in normal Python is to use @abc.abstractmethod, but CircuitPython doesn't have the abc module. Personally I'm inclined to just leaving this out completely, and just mentioning in the comments that the subclasses need to implement those methods. This is not something the user of the library should ever see or need anyways.

deshipu · 2017-12-23T18:05:26Z

Does the _DISPLAYON missing explain @kattni problem with drivers?

I think so.

kattni

SSD1331 test fails.

kattni · 2017-12-23T22:44:20Z

adafruit_rgb_display/ssd1331.py

@@ -1,4 +1,9 @@
+"""A simple driver for the SSD1331-based displays."""


Testing with SSD1331 failed. However I get the same results with the current version of the driver as well. Multicolor pixellation and a single vertical line that turns the color that the code attempts to fill the entire display. That 1-pixel-wide line will change color if I change the fill color in the code. The line is close to the left side, if that matters.

mrmcwethy · 2017-12-23T23:41:07Z

@kattni what does your main.py look like?

kattni · 2017-12-23T23:43:03Z

@mrmcwethy The following:

import busio
import digitalio
import board
from adafruit_rgb_display import color565
import adafruit_rgb_display.ssd1331 as ssd1331
spi = busio.SPI(clock=board.SCK, MOSI=board.MOSI, MISO=board.MISO)
display = ssd1331.SSD1331(spi, cs=digitalio.DigitalInOut(board.D5),
                               dc=digitalio.DigitalInOut(board.D11), rst=digitalio.DigitalInOut(board.D9))

while True:
    display.fill(0x7521)
    display.pixel(32, 32, 0)

deshipu · 2017-12-23T23:47:32Z

I got the SSD1331 display to work after this library was forked from mine. If you look at the MicroPython library, you will see the fixed setup code there: https://github.com/adafruit/micropython-adafruit-rgb-display/blob/master/ssd1331.py#L52-L75

However, I believe that this would be a separate issue and a separate pull request.

kattni · 2017-12-24T00:33:59Z

I have the SSD1331 working. I'm going to make a separate PR to resolve it since it's an issue with the current version of the driver as well.

mrmcwethy · 2017-12-29T14:18:11Z

Now that i have removed the "abstract functions" can this be merged?

Also resolved conflicts to SSD1331.py

mrmcwethy · 2017-12-29T15:20:58Z

I am happy to commit resolved conflicts. If someone else does it, take the "lint" changes over master.

kattni · 2018-01-02T20:44:49Z

Current testing status: the ILI9341 and the ST7735 (non-R version) still need to be tested.

All other displays have been successfully tested.

kattni · 2018-01-03T22:18:55Z

Adafruit does not carry the ST7735, only the ST7735R. ILI9341 tested successfully. That's everything! Great job!

Updating https://github.com/adafruit/Adafruit_CircuitPython_RGB_Display to 3.0.0 from 2.0.1: > Merge pull request adafruit/Adafruit_CircuitPython_RGB_Display#10 from mrmcwethy/lint > Update to SSD1331 driver (adafruit/Adafruit_CircuitPython_RGB_Display#11) > Merge pull request adafruit/Adafruit_CircuitPython_RGB_Display#9 from deshipu/fix-small-buffer > Merge pull request adafruit/Adafruit_CircuitPython_RGB_Display#8 from deshipu/fix-readinto

added pylint to project and udpate code

69eb79f

mrmcwethy changed the title ~~added pylint to project and udpate code~~ added pylint to project and update code Dec 12, 2017

kattni suggested changes Dec 22, 2017

View reviewed changes

mrmcwethy added 4 commits December 22, 2017 05:34

reverted some important deleted lines. added pylint disables instead.

6a9b8d0

added required internal identifies

a88a657

fixing pylint problem from travis build

b742348

had a return without a value.

76e40ba

deshipu suggested changes Dec 23, 2017

View reviewed changes

fixes requested for reviewer

6e0ec5c

deshipu reviewed Dec 23, 2017

View reviewed changes

kattni suggested changes Dec 23, 2017

View reviewed changes

Fixes requested by reviewers

fd30603

deshipu approved these changes Dec 24, 2017

View reviewed changes

remove "abstract functions" and added pylint disable

d6443ce

mrmcwethy added 2 commits December 29, 2017 07:29

updated with master changes

24232d2

fix reference to rename variable

3c18ac4

kattni approved these changes Jan 3, 2018

View reviewed changes

kattni and others added 2 commits January 3, 2018 17:20

Merge branch 'master' into lint

73b8ed0

Merge branch 'master' into lint

ba35625

kattni merged commit 7e3ae1d into adafruit:master Jan 3, 2018

		@@ -1,5 +1,6 @@
		"""A simple driver for the SSD1351-based displays."""

		@@ -1,4 +1,9 @@
		"""A simple driver for the SSD1331-based displays."""

added pylint to project and update code #10

added pylint to project and update code #10

Conversation

mrmcwethy commented Dec 12, 2017

mrmcwethy commented Dec 13, 2017

kattni left a comment

Choose a reason for hiding this comment

kattni Dec 22, 2017 • edited Loading

Choose a reason for hiding this comment

kattni left a comment

Choose a reason for hiding this comment

kattni Dec 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrmcwethy Dec 22, 2017 • edited Loading

Choose a reason for hiding this comment

mrmcwethy commented Dec 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrmcwethy commented Dec 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deshipu Dec 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deshipu commented Dec 23, 2017

kattni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrmcwethy commented Dec 23, 2017

kattni commented Dec 23, 2017

deshipu commented Dec 23, 2017

kattni commented Dec 24, 2017

mrmcwethy commented Dec 29, 2017 • edited Loading

mrmcwethy commented Dec 29, 2017

kattni commented Jan 2, 2018

kattni commented Jan 3, 2018

kattni Dec 22, 2017 •

edited

Loading

kattni Dec 22, 2017 •

edited

Loading

mrmcwethy Dec 22, 2017 •

edited

Loading

deshipu Dec 23, 2017 •

edited

Loading

mrmcwethy commented Dec 29, 2017 •

edited

Loading