Skip to content

CPython compatibility fix #31

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Oct 3, 2022
Merged

CPython compatibility fix #31

Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
30c0988
Fixed CPython compatibility
shulltronics Sep 28, 2022
b197cd6
Added author info and ran through black
shulltronics Sep 28, 2022
0161017
Actually added author info
shulltronics Sep 28, 2022
cf9a11a
Fixed test errors by implementing __getitem__ method for Vec2D
shulltronics Sep 28, 2022
8c32679
Removed author info
shulltronics Oct 1, 2022
017591b
try has-a tuple instead of is-a tuple for Vec2D
FoamyGuy Oct 1, 2022
5acb2d0
only import board if we are going to try to use builtin display
FoamyGuy Oct 1, 2022
c146d6b
allow import outside top for board
FoamyGuy Oct 1, 2022
87ee248
remove unused super init
FoamyGuy Oct 1, 2022
12e4ec2
Merge remote-tracking branch 'shulltronics/cpython-compatibility' int…
FoamyGuy Oct 1, 2022
47f94f2
Merge pull request #1 from FoamyGuy/foamyguy_cpython_compat
shulltronics Oct 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions adafruit_turtle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,13 @@
import gc
import math
import time
import board
import displayio

try:
import board
Copy link
Member

@tekktrik tekktrik Sep 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

except NotImplementedError:
print("[adafruit-turtle.py]: Couldn't import board module.")

__version__ = "0.0.0+auto.0"
__repo__ = "https://github.com/adafruit/Adafruit_CircuitPython_turtle.git"

Expand Down Expand Up @@ -80,7 +84,7 @@ def __init__(self):
pass


class Vec2D(tuple):
class Vec2D:
"""A 2 dimensional vector class, used as a helper class
for implementing turtle graphics.
May be useful for turtle graphics programs also.
Expand All @@ -94,8 +98,11 @@ class Vec2D(tuple):
# k*a and a*k multiplication with scalar
# |a| absolute value of a
# a.rotate(angle) rotation
def __init__(self, x, y):
super().__init__((x, y))
def __new__(cls, x, y):
return (x, y)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def __getitem__(self, index):
return getattr(self, index)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def __add__(self, other):
return Vec2D(self[0] + other[0], self[1] + other[1])
Expand Down