Skip to content

Incorrect parameter names and docstrings #15

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

Closed
hex-yes opened this issue Nov 26, 2019 · 6 comments · Fixed by #16
Closed

Incorrect parameter names and docstrings #15

hex-yes opened this issue Nov 26, 2019 · 6 comments · Fixed by #16
Assignees

Comments

@hex-yes
Copy link

hex-yes commented Nov 26, 2019

First, the RA8875 driver board and library helped me complete my project, and I sincerely appreciate the effort that went into both.

I discovered that the parameter names for the _rect_helper method of the RA8875 class are inconsistent with the actual drawing behavior. The method signature is:

_rect_helper(self, x, y, width, height, color, filled)

However, the width and height parameters should really be the x- and y-coordinate of the endpoint--they are a second absolute x-y screen coordinate, rather than a width and height offset applied to the first screen coordinate. Page 120 of the RA8875 datasheet that is linked from the Adafruit product page confirms the registers written by this method are an endpoint coordinate, not width/height.

This parameter mislabeling carries through to the public methods of the same class that rely on the _rect_helper method, including at least the rect and fill_rect methods (but possibly others...these are just the ones I happened to use with confusing results).

I would suggest that the signature of the _rect_helper method be changed to be consistent with other methods like _triangle_helper:

_rect_helper(self, x1, y1, x2, y2, color, filled)

I would then suggest that the parameters and documentation for other methods like rect and fill_rect that currently specify a width/height be updated accordingly. By changing the parameter names and documentation, existing code (including the examples) that relies on the existing behavior will not be broken.

@makermelissa
Copy link
Collaborator

Good catch. Yeah, I think I based it off the Arduino driver, but taking a closer look at that, that takes the x and y offset into account. I'll update it to behave as the Arduino driver does and just release as a version 3.0.0, because it may break code.

@hex-yes
Copy link
Author

hex-yes commented Nov 26, 2019

In the Arduino driver, it looks like (and check me on this) that the rectHelper method has mislabeled parameters. It does not actually take a width and height, but instead takes the endpoint coordinate. However, in the Arduino driver, this is compensated for by all the methods that call rectHelper adding the passed width and height to the x-y start point coordinate, so it doesn't cause any public-facing grief.

@makermelissa
Copy link
Collaborator

That sounds correct.

@hex-yes
Copy link
Author

hex-yes commented Nov 26, 2019

With that in mind, if you update all the calls to _rect_helper in the Python driver like so:

self._rect_helper(x, y, width, height, color, False)
to
self._rect_helper(x, y, x+width, y+height, color, False)

It should then work like the Arduino driver (with the same mislabeled params in the helper method, but compensated for in the public-facing methods). The example code in the Python driver would also need to be updated for the new behavior.

@makermelissa
Copy link
Collaborator

I'm updating them to:
self._rect_helper(x, y, x + width - 1, y + height - 1, color, False)

@hex-yes
Copy link
Author

hex-yes commented Nov 26, 2019

Ah, right you are. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants