Skip to content

Conversation

jepler
Copy link

@jepler jepler commented Jul 25, 2019

When nrf pwm audio is introduced, it will be called audiopwmio. To enable code sharing with the existing (dac-based) audioio, factor the sample and mixer types to audiocore.

INCOMPATIBLE CHANGE: Now, Mixer, RawSample and WaveFile must be imported from audiocore, not audioio.

Testing performed: On a metro m4 express, I imported both modules and (without a speaker attached) played a RawSample out the A0 pin

When nrf pwm audio is introduced, it will be called `audiopwmio`.  To
enable code sharing with the existing (dac-based) `audioio`, factor
the sample and mixer types to `audiocore`.

INCOMPATIBLE CHANGE: Now, `Mixer`, `RawSample` and `WaveFile` must
be imported from `audiocore`, not `audioio`.
@jepler jepler requested a review from tannewt July 25, 2019 11:51
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.

Thanks for doing this! I removed unneeded headers from shared-bindings/audioio/init.c and looks good otherwise! We'll need to fix example code when we release a 5.0 beta. We should also get the mixer changes in since we're changing things.

@tannewt tannewt merged commit d99d3bd into adafruit:master Jul 25, 2019
@jerryneedell
Copy link
Collaborator

jerryneedell commented Jul 28, 2019

FYI -- this is a breaking change for the CircuitPlayground Express express library since it uses audioio to call RawSample and WaveFIle.

@jepler
Copy link
Author

jepler commented Jul 28, 2019

fwiw, I think that it would be possible to keep the classes in audioio either for a limited time (like, would appear in both places in 5.x, and be only in audiocore in 6.x) or even forever. The "price" would be just a few dozen bytes in the dictionary of audioio. I'd be happy to double check this and submit a pull request if that's what @tannewt would like.

Meanwhile, I would also be happy to help out with some import logic in cpx to adapt to either circuitpython version, if that would be helpful. Let me know if that would be useful to you, @jerryneedell

@jerryneedell
Copy link
Collaborator

@jepler its not a problem for me -- I have already implemented the changes I need for the test I was doing. I just wanted to make sure it was a known issues since it is likely to cause some problems for users.

@tannewt
Copy link
Member

tannewt commented Jul 29, 2019

@jepler My preference is to just move it. If you leave it in the old place there is no reason to update code. That being said, could someone post a sample import that is backwards compatible for folks? Thanks!

@jepler
Copy link
Author

jepler commented Jul 29, 2019

Untested, but I believe you would want to write, for any or all items that you wanted from audiocore,

try:
    from audiocore import RawSample
except:
    from audioio import RawSample

At this point, RawSample will be available as an unqualified name. You could also write (untested)

try:
    import audiocore
except:
    import audioio as audiocore

At this point audiocore.RawSample will be available as a qualified name.

When I'm at home, I have a CPX I can flash with 4.0.2 and current and verify that these both work in both cases.

Should this be written up somewhere in the docs? or in a guide?

@tannewt
Copy link
Member

tannewt commented Jul 29, 2019

I think this PR is fine and maybe release notes too. I'd tweak the except line to be except ImportError: to ensure it's only due to the module missing.

@jepler
Copy link
Author

jepler commented Jul 30, 2019

@tannewt you mentioned getting "the mixer" fixes -- if there's an issue or pull, can you link it?

I verified both workarounds,

try:
    import audiocore
except ImportError:
    import audioio as audiocore

and

try:
    from audiocore import RawSample
except ImportError:
    from audioio import RawSample

with a circutplayground express using 4.0.2 and tip-of-master.

@tannewt
Copy link
Member

tannewt commented Jul 31, 2019

@jepler This is the PR I'm thinking of: #1365

@jepler jepler deleted the audiocore branch November 3, 2021 21:09
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