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

validate various displayio args; new pin validation routines; don't use mp_const_none if NULL will do #2666

Merged

Conversation

dhalbert
Copy link
Collaborator

@dhalbert dhalbert commented Feb 29, 2020

There was some missing pin argument validation in displayio, and when starting to fix that, I decided to simplify how pin argument validation and free pin checking was done.

  • Replace assert_pin completely, and subsume most uses of assert_free_pin() by adding the new routines validate_is_free_pin() and validate_is_free_pin_or_none(), which check whether an mp_obj_t is a pin and return an mcu_pin_obj_t * if it is. This reduces several lines of code to one, improving readability and reducing boilerplate.
  • Be consistent that optional *_obj_t arguments passed to common_hal_... (mostly pins) are passed as NULL instead of mp_const_none. Also, use of mp_const_none in common-hal is minimized.

The changes save about 260 bytes in the Trinket M0 build.

@dhalbert dhalbert added this to the 5.x.x - Bug Fixes milestone Feb 29, 2020
@tannewt
Copy link
Member

tannewt commented Mar 3, 2020

I didn't look at the code (add me as a reviewer or @ me when ready) but I think validate_pin_is_free would read better.

@dhalbert
Copy link
Collaborator Author

dhalbert commented Mar 4, 2020

I didn't look at the code (add me as a reviewer or @ me when ready) but I think validate_pin_is_free would read better.

The old assert_pin_free() did not actually check that the object was a pin, so I was trying to make that clearer. But I agree the name was a bit awkward. I think maybe validate_obj_is_pin() and validate_obj_is_free_pin() would be more grammatical and clearer.

@tannewt
Copy link
Member

tannewt commented Mar 4, 2020

validate_obj_is_free_pin works for me. Thanks!

@dhalbert dhalbert requested a review from tannewt March 5, 2020 21:34
@dhalbert dhalbert marked this pull request as ready for review March 5, 2020 21:35
@dhalbert dhalbert changed the title new pin validation routines; don't use mp_const_none if NULL will do validate various displayio args; new pin validation routines; don't use mp_const_none if NULL will do Mar 5, 2020
@dhalbert
Copy link
Collaborator Author

dhalbert commented Mar 6, 2020

The churn of trying checkout@v2 (see #2683) and then reverting it fixed the build problems. (EDIT: not really)

@dhalbert
Copy link
Collaborator Author

dhalbert commented Mar 6, 2020

Now I am getting a "ERROR: THESE PACKAGES DO NOT MATCH THE HASHES FROM THE REQUIREMENTS FILE." errors on a few random builds, perhaps because of VM problems??

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

One minor improvement. Good otherwise. Is the CI simply busted now?

shared-bindings/displayio/ParallelBus.c Show resolved Hide resolved
@dhalbert dhalbert requested a review from tannewt March 6, 2020 20:06
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thank you!

@tannewt tannewt merged commit df88939 into adafruit:master Mar 9, 2020
@tannewt
Copy link
Member

tannewt commented Mar 17, 2020

FYI this breaks Trinket M0 and likely any other boards that use a DotStar as the status LED. The SPI constructor call needs to be updated to pass NULL instead of mp_const_none here: https://github.com/adafruit/circuitpython/blob/master/supervisor/shared/rgb_led_status.c#L128

@dhalbert dhalbert deleted the assert_pin-and-mp_const_none-cleanup branch March 17, 2020 19:43
@dhalbert
Copy link
Collaborator Author

I looked through all the mp_const_none uses, but clearly not for all values of "all". :)

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 this pull request may close these issues.

None yet

2 participants