CPython compatibility fix #31

shulltronics · 2022-09-28T02:34:21Z

For this PR, I changed two things.

I added a conditional import for the board module, so that on non-CircuitPython devices (i.e. generic computers), this script will library will still work.
I removed the tuple subclass from the Vec2D class, as the constructor/initializer was incompatible with CPython. Instead, I use the __new__ constructor and just explicitly return a tuple instead of calling the super constructor.

Now this library works as-is for devices running adafruit-blinka, such as a Raspberry Pi with a PiTFT display, or generic PCs using something like PygameDisplay.

tekktrik · 2022-09-28T03:00:21Z

Looks like the CI is having some issues, you'll need to lint this. You can use pre-commit to help automate the formatting changes and work through the linter issues. Instructions on getting started with pre-commit can be found in this Learn Guide:

https://learn.adafruit.com/creating-and-sharing-a-circuitpython-library/check-your-code

shulltronics · 2022-09-28T03:32:55Z

@tekktrik Can't believe I missed this part :( But I fixed the issues!

tekktrik · 2022-09-30T15:07:53Z

adafruit_turtle.py

 import displayio

+try:
+    import board


This is an usual import to mask with a try/except, both because it seems its used functionally in this module (not just for typing) and because it silently does so, masking what should be an exception with a print() statement. Is there any reason when we're catching NotImplementedError instead of ImportError? Maybe I'm missing something, I still need to check the rest, but wanted to ask about this in the meantime.

Hey @tekktrik, the reason I catch that exception is so that I can run this module on systems that don't have the board module. When running with Blinka, the board module is defined for systems such as a Raspberry Pi SBC, but on generic x86 computers, the Blinka library will raise a NotImplementedError error (not sure why it's not an ImportError). That exception is thrown at the bottom of this file.

I suppose another approach could be to remove the import all together, and then in the turtle initializer make the display argument a required one. This would force users to pass the board.DISPLAY for CircuitPython boards with a built-in display, vs the current behavior where that is the default -- note that this itself is wrapped in a try/except, which seems a bit weird because it will cause an exception on CircuitPython boards without a built-in display. This change, however, would break some existing scripts that use the library without passing the display parameter.

Let me know what you think!

I think this was written before the generic x86 computer implementation was added, hence why it currently only catches AttributeError. However, is there a use case where a display can be defined for a generic x86 computer? I'm not familiar with that implementation of Blinka and how it's used, and I've reached out to the other devs as well. If you know of one, it would help me out!

Yes there is! @FoamyGuy has one here, and I've included that code in my project here. It's fun because you can incorporate displayio graphics into CPython applications. It's also useful for designing/testing embedded UIs for CircuitPython when you might not have access to the hardware.

I've been using this to demonstrate the power of abstraction/modularity to some of my students at University of Florida, where we can run (almost) identical code on laptops, RPis (either with HDMI or a PiTFT), and on microcontrollers with an embedded display -- just by changing the display object to target the proper display for our system.

I worked on this pull request because I want to be able to use turtle in a similar fashion, but the current implementation just doesn't allow for it.

Got it, thanks! I'll get to this in the next few days, but that helps but the change into context!

Another potential option is to move the import down to inside the if statement so that it will only try to import board. if the user has not passed a display object.

FoamyGuy · 2022-09-30T22:35:10Z

Thanks for tracking this down and submitting the fix @shulltronics. I will look into this and try it out tomorrow.

FoamyGuy · 2022-10-01T14:38:41Z

adafruit_turtle.py

@@ -1,6 +1,7 @@
 # SPDX-FileCopyrightText: 2006-2010 Gregor Lingl for Adafruit Industries
 # SPDX-FileCopyrightText: 2019 LadyAda for Adafruit Industries
 # SPDX-FileCopyrightText: 2021 Dave Astels for Adafruit Industries
+# SPDX-FileCopyrightText: 2022 Carsten Thue-Bludworth for Adafruit Industries


I think the intention is to only use for Adafruit Industiries if you were paid by them for the development work.

Haha, ok I understand (I wish I was working for Adafruit!). I added it because I was trying to follow all of the proper protocols for contributing according to this guide.

FoamyGuy

I tried this out and was running into some issues with it crashing. I've tinkered a bit and found a version which does (so far) seem to be working on both microcontroller and CPython. I'll PR it to your branch @shulltronics

FoamyGuy · 2022-10-01T16:00:16Z

adafruit_turtle.py

-    def __init__(self, x, y):
-        super().__init__((x, y))
+    def __new__(cls, x, y):
+        return (x, y)


I get this error using this version on a PyPortal running the simpletest script in this repo:

Adafruit CircuitPython 8.0.0-beta.0 on 2022-08-18; Adafruit PyPortal with samd51j20 Board ID:pyportal

Traceback (most recent call last): File "code.py", line 32, in <module> TypeError: unsupported types for __sub__: 'tuple', 'tuple'

The simpletest is trying to subtract positions here:

Adafruit_CircuitPython_turtle/examples/turtle_simpletest.py

Line 23 in 9f23b46

if abs(turtle.pos() - start) < 1:

Which turns out not to work because tuple doesn't have support for minus operator. This was odd to me at first because __sub__ is defined for Vec2D but it turns out because of the change in __new__ that these were actually being tuple objects instead of Vec2D objects.

I ended up trying this version with CPython using PyGameDisplay and got the same error from the simpletest where it crashes due to trying to subtract the Vec2D's which are actually tuples instead.

Some of the functionality does work properly though because you can see it draw the first line of the star in the simpletest before crashing. It seems only if you try to utilize some of the other Vec2D functions it will have trouble.

FoamyGuy · 2022-10-01T16:01:58Z

adafruit_turtle.py

+        return (x, y)
+
+    def __getitem__(self, index):
+        return getattr(self, index)


The way you overrode __getitem__ here gave me inspiration for a different approach. If we can have Vec2D "hold" a tuple instead of "being" a tuple then perhaps we can have the "best of both worlds" and it will work on both microcontroller and SBC / CPYthon.

FoamyGuy · 2022-10-01T16:15:29Z

adafruit_turtle.py

@@ -1,7 +1,6 @@
 # SPDX-FileCopyrightText: 2006-2010 Gregor Lingl for Adafruit Industries
 # SPDX-FileCopyrightText: 2019 LadyAda for Adafruit Industries
 # SPDX-FileCopyrightText: 2021 Dave Astels for Adafruit Industries
-# SPDX-FileCopyrightText: 2022 Carsten Thue-Bludworth for Adafruit Industries


If you'd like to keep yourself credited, you can definitely do that, just without the for Adafruit Industries portion. It is up to you though.

…o foamyguy_cpython_compat # Conflicts: # adafruit_turtle.py

CPython compatibility

shulltronics · 2022-10-02T01:54:33Z

I merged the changes made by @FoamyGuy and tested on three platforms including stable CircuitPython 7.3.0 on a Feather nRF52840 with an SPI display, an Adafruit CLUE running 8.0.0beta, and on CPython 3.10.7 with the PygameDisplay. Seems to work well on all of them. I tested quite a few of the example scripts in the turtle library.

FoamyGuy

Looks good to me.

Tested successfully with PyPortal, RaspberryPi + SPI Display w/ Blinka_DisplayIO, and PC with Blinka_DIsplayIO + PygameDisplay

Thanks again for working on this @shulltronics. Also for sharing what you're doing with it :) it's always great to hear that it's been used to teach these concepts.

shulltronics · 2022-10-03T23:33:04Z

Thank you everyone! Appreciate the time and energy to get these changes added in :)

shulltronics added 3 commits September 27, 2022 22:20

Fixed CPython compatibility

30c0988

Added author info and ran through black

b197cd6

Actually added author info

0161017

shulltronics mentioned this pull request Sep 28, 2022

turtle constructor incompatible with CPython: tuple expected at most 1 argument, got 2 #30

Closed

tekktrik self-requested a review September 28, 2022 03:00

Fixed test errors by implementing __getitem__ method for Vec2D

cf9a11a

tekktrik reviewed Sep 30, 2022

View reviewed changes

FoamyGuy reviewed Oct 1, 2022

View reviewed changes

shulltronics and others added 2 commits October 1, 2022 10:50

Removed author info

8c32679

try has-a tuple instead of is-a tuple for Vec2D

017591b

FoamyGuy reviewed Oct 1, 2022

View reviewed changes

FoamyGuy added 3 commits October 1, 2022 11:04

only import board if we are going to try to use builtin display

5acb2d0

allow import outside top for board

c146d6b

remove unused super init

87ee248

FoamyGuy reviewed Oct 1, 2022

View reviewed changes

FoamyGuy and others added 2 commits October 1, 2022 11:17

Merge remote-tracking branch 'shulltronics/cpython-compatibility' int…

12e4ec2

…o foamyguy_cpython_compat # Conflicts: # adafruit_turtle.py

Merge pull request #1 from FoamyGuy/foamyguy_cpython_compat

47f94f2

CPython compatibility

FoamyGuy approved these changes Oct 3, 2022

View reviewed changes

FoamyGuy merged commit b7b4386 into adafruit:main Oct 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CPython compatibility fix #31

CPython compatibility fix #31

shulltronics commented Sep 28, 2022

tekktrik commented Sep 28, 2022

shulltronics commented Sep 28, 2022

tekktrik Sep 30, 2022 •

edited

Loading

shulltronics Sep 30, 2022

tekktrik Sep 30, 2022

shulltronics Sep 30, 2022

tekktrik Sep 30, 2022

FoamyGuy Oct 1, 2022

FoamyGuy commented Sep 30, 2022

FoamyGuy Oct 1, 2022

shulltronics Oct 1, 2022

FoamyGuy left a comment

FoamyGuy Oct 1, 2022

FoamyGuy Oct 1, 2022

FoamyGuy Oct 1, 2022

shulltronics commented Oct 2, 2022

FoamyGuy left a comment

shulltronics commented Oct 3, 2022

CPython compatibility fix #31

CPython compatibility fix #31

Conversation

shulltronics commented Sep 28, 2022

tekktrik commented Sep 28, 2022

shulltronics commented Sep 28, 2022

tekktrik Sep 30, 2022 • 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

Choose a reason for hiding this comment

FoamyGuy commented Sep 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FoamyGuy left a 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

shulltronics commented Oct 2, 2022

FoamyGuy left a comment

Choose a reason for hiding this comment

shulltronics commented Oct 3, 2022

tekktrik Sep 30, 2022 •

edited

Loading