-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Implement PWM audio out for the nrf port #2000
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
Conversation
I'd be happy to merge this early with bugs but would like it to be in it's own module and class rather than AudioOut. At some point I'd like PWM AudioOut on SAMDs as well for pin flexibility. |
@jepler, there are merge conflicts, so it won't run a build until they're resolved. |
I've updated the branch, and revised the description of the patch to reflect progress since I filed it. @tannewt |
@tannewt While this is unfinished due to pause/resume being missing, I think the renaming and the bulk of the work is done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the rename! This is very close. One more name suggestion and a build to fix. (Looks like builds that disable audiobusio.) Thanks!
|
||
const mp_obj_type_t audiopwmio_audioout_type = { | ||
{ &mp_type_type }, | ||
.name = MP_QSTR_AudioOut, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you rename this to PWMAudioOut or PulseAudioOut too? That way it'll print differently than audioio.AudioOut.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize this is super nitpicky but please rename the files and function names too from audioout
to pwmaudioout
. Future us will thank you for the consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this latest commit get the renames you were looking for, @tannewt ?
Without such a definition, this header is not self-contained, but requires whoever included it to also include vfs_fat.h
This implements AudioOut, with known caveats: * pause/resume are not yet implemented (this is just a bug) * at best, the sample fidelity is 8 bits (this is a hardware limitation) Testing performed: My test system is a Particle Xenon with a PAM8302 op-amp https://www.adafruit.com/product/2130 and 8-ohm speaker. There's no analog filtering between the Xenon's PWM pin and the "A+" input of the amplifier; the "A-" pin is disconnected. It is powered from VUSB. I used pin D4, which is *NOT* listed as a low-speed-only pin, but the code does NOT switch the pin to high drive. This is related to an open issue for general inability to set drive level for pins being used by a "special function" on nrf: adafruit#1270 Nothing about the code I've written should limit the usable pins. All samples I played were 16-bit, generally monophonic at 11025Hz and 22050Hz from the Debian LibreOffice package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more naming consistency nitpick then it should be good to go. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! Looks good to me. Thanks!
.. and also incidentally fix a problem where a RawSample could only be looped 131070 times.
@tannewt I think this is ready for another review. I've updated the initial text of the pull request to reflect the state of testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple questions and then it should be good to go.
The original formulation was because I saw the need to avoid a transition from playing to stopped exactly when a resume was taking place. However, @tannewt was concerned about this pause causing trouble, because it could be relatively lengthy (several ms even in a typical case). After reflection, I've convinced myself that updating the registers in this order in resume avoids a window where a "stopped" event can be missed as long as the shortcut is updated first. Testing re-performed: pause/resume testing of looped RawSample and WaveFile audio sources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, looks good to me. Thanks!
This implements PWMAudioOut. Besides the limitation to 7.5 - 8 bit fidelity, I believe it is now complete.
Testing performed:
My test system is a Particle Xenon with a PAM8302 op-amp https://www.adafruit.com/product/2130 and 8-ohm speaker. There's no analog filtering between the Xenon's PWM pin and the "A+" input of the amplifier; the "A-" pin is disconnected. It is powered from VUSB.
I used pin D4, which is NOT listed as a low-speed-only pin, but the code does NOT switch the pin to high drive. This is related to an open issue for general inability to set drive level for pins being used by a "special function" on nrf: #1270
Nothing about the code I've written should limit the usable pins.
All samples I played were 16-bit, generally monophonic at 11025Hz and 22050Hz from the Debian LibreOffice package.
OK RawSample (8000Hz, 1 channel)
OK carrier frequency 62.5kHz
OK Frequency close to 440Hz (measured via o'scope)
OK Single Play
OK (too short to do stop/playing test)
OK Loop
OK Pause/Resume
OK Stop/Playing
OK WaveFile (11025 and 22050Hz, 1 channel)
OK carrier frequency close to 62.5kHz
OK Duration of sample close to when played on PC
OK Single Play
OK Stop/Playing
OK Loop
OK Pause/Resume
OK Stop/Playing