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

Adafruit funhouse #4439

Merged
merged 9 commits into from
Mar 19, 2021
Merged

Adafruit funhouse #4439

merged 9 commits into from
Mar 19, 2021

Conversation

dhalbert
Copy link
Collaborator

  • Add FunHouse board
  • Make PWMOut constructor return status values instead of throwing exceptions. PWMOut may be created before exceptions can be thrown for backlight, etc. Changes in esp32s2 and stm due to this (hence tagging @hierophect for a look).

@dhalbert dhalbert requested review from ladyada and hierophect March 19, 2021 03:28
@dhalbert dhalbert marked this pull request as draft March 19, 2021 04:15
@dhalbert
Copy link
Collaborator Author

draft for now; backlight issues

@dhalbert
Copy link
Collaborator Author

I tested STM changes on a Feather STM32405, creating PWMOut's in the REPL to check the new error handling.

@dhalbert dhalbert marked this pull request as ready for review March 19, 2021 16:05
Copy link
Member

@ladyada ladyada left a comment

Choose a reason for hiding this comment

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

dotstars/LED/backlight/TFT works. i have more to test but this is good for now

@dhalbert dhalbert merged commit 0615876 into adafruit:main Mar 19, 2021
@dhalbert dhalbert deleted the adafruit_funhouse branch March 19, 2021 16:35
Copy link
Collaborator

@hierophect hierophect left a comment

Choose a reason for hiding this comment

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

Retroactive LGTM. The only thing we lose here is telling users that they might want to reshuffle certain initializations on STM32 so the dynamic timer allocation doesn't reserve timers on pins that require PWMOut. That was an STM32 specific issue, so I didn't want to abstract it to the shared bindings level. That only comes up if you're using PWMOut and PulseIO/Protomatter simultaniously though, so it's probably fine to leave debugging to the user.

Remind me the incentive for standardizing these exceptions again, does it have a benefit for displays? It's been a while since I touched it.

@dhalbert
Copy link
Collaborator Author

dhalbert commented Mar 19, 2021

Remind me the incentive for standardizing these exceptions again, does it have a benefit for displays? It's been a while since I touched it.

The reason to remove the exceptions and pass back status codes was that the on-board display initialization in boards/<someboard>/board.c is done before exceptions will work or can be reported. common_hal_displayio_display_construct() was calling common_hal_pwmio_pwmout_construct() to control the backlight pin, and when the latter failed, an exception was getting raised. So I made it uniformly return a status code instead, so that common_hal_displayio_display_construct() could decide what to do.

There are a lot more cases for STM32 PWMOut creation failure than some other ports, so I added some, and unified a few others, mostly trying to differentiate the ones that would be meaningful to the end user. The main disadvantage of this change I saw is that it grows the code a bit for all ports a bit because now shared-bindings/pwmio/PWMOut.c knows how to deal with all the possible failures across all ports.

@hierophect
Copy link
Collaborator

hierophect commented Mar 19, 2021

@dhalbert that's right, I remember that now. I thought I had a custom handler for that but I guess not. I guess we could make common_hal_pwmio_pwmout_construct() aware of whether it's being called from board.c or the python code, returning more generic errors in the former case, but that's maybe too much work for some minor flash savings and failure conditions.

This was referenced Mar 27, 2021
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.

3 participants