adafruit / Adafruit_CircuitPython_RTTTL

Play RTTTL tunes in CircuitPython
MIT License
4 stars 12 forks source link

Print error about missing waveform library #29

Closed FoamyGuy closed 2 years ago

FoamyGuy commented 2 years ago

This resolves: #28

The root cause was that audioio did exist but I did not properly load the adafruit_waveform library which gets caught by the same except block that is checking for audioio. That caused the library to fall back to pwm which is invalid for the PyPortals pin so raising the exception from that makes sense.

This PR adds a more clear message to let the user know specifically that they do have audioio but are missing the adafruit_waveform library.

FoamyGuy commented 2 years ago

After sleeping on it I think it may be best to re-raise the ImportError instead of just printing it out. This will cause it to crash and give the exact reason as to why which is the typical behavior when missing an import.

I'll add a new commit today to raise instead of print the ImportError.

FoamyGuy commented 2 years ago

I'm not sure why Sphinx was failing with:

Warning, treated as error:
autodoc: failed to import module 'adafruit_rtttl'; the following exception was raised:
No module named 'adafruit_waveform'

The waveform library is listed in requirements.txt and seems to have gotten installed correctly according to the logs from the actions run. But for some reason Sphinx was unable to to use it from there it seems.

I've added adafruit_waveform to the list of mocked packages in the docs/conf.py file to resolve that issue for now.

After the latest commits this change will now raise the ImportError in cases where audioio does exist but adafruit_waveform was not found:

code.py output:
Traceback (most recent call last):
  File "code.py", line 30, in <module>
  File "/lib/adafruit_rtttl.py", line 36, in <module>
  File "/lib/adafruit_rtttl.py", line 25, in <module>
ImportError: no module named 'adafruit_waveform'

Tested successfully on PyPortal with 7.1.0 beta.

FoamyGuy commented 2 years ago

@KeithTheEE good catch, thanks for the review!

Latest commit moves setting AUDIOIO_AVAILABLE to immediately after the import succeeds.

I also figured it would make sense to be consistent in the naming of the variables so I renamed the waveform one to WAVEFORM_AVAILABLE to match the existing audioio one.

KeithTheEE commented 2 years ago

Could you try one more thing for me? Don't include audioio in your lib folder.

I'm afraid it'll continue like everything is normal and crash much later on because the only reason this raises the exception is if adafruit_waveform isn't there, not if audioio isn't there

KeithTheEE commented 2 years ago

Nevermind, that's true to the original behavior, my bad. This looks good to me!