RP2040 on tune #254
Merged
RP2040 on tune #254
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
the ideal value of the period
|
Memory usage change @ a5c2639
Click for full report table
Click for full report CSV |
Collaborator
Author
|
Fairly straightforward! EXTERNAL_TIMED/PWM was initially tested to see that the small patch corrected the detuning. PWM output has been switched to DMA buffered transfer using PWMAudio. Both mono and stereo have been tested. Crude EXTERNAL_TIMED (with pwm in user sketch) has been tested (not clean at all, the cycle is probably too low, but seems at the correct frequency). |
|
Memory usage change @ 7cd06ac
Click for full report table
Click for full report CSV |
|
Memory usage change @ b89e39f
Click for full report table
Click for full report CSV |
tfry-git
approved these changes
May 8, 2024
Collaborator
tfry-git
left a comment
tfry-git
left a comment
There was a problem hiding this comment.
Looks good to me (without any testing done). Some comments / questions added.
internal/MozziGuts_impl_RP2040.hpp
Outdated
| do { | ||
| defaultAudioOutput(); | ||
| next_audio_update = delayed_by_us(next_audio_update, micros_per_update); | ||
| flip_flop = !flip_flop;; |
Collaborator
There was a problem hiding this comment.
I was rather thinking along the following lines (pseudocode):
uint64_t next_audio_update_shifted = 0;
const uint64_t us_persample_shifted = (1000000l << 8) / MOZZI_AUDIO_RATE;
[...]
next_audio_update_shifted += us_per_sample_shifted;
while(hardware_alarm_set_target(alarm_num, delayed_by_us(0, next_audio_update_shifted >> 8));
Collaborator
Author
There was a problem hiding this comment.
Ah, I understand what you meant now!
I guess it would even more simpler be: while (hardware_alarm_set_target(audio_update_alarm_num, next_audio_update_shifted>>8)); // absolute time, so no need for delayed_by_us
Indeed, that would be more general, I can try. My version works only for this case where we are basically nearly exactly between two values. I have a hard time trying to intuitively figure out which one is faster but yours might be (you: 1 add, 1 shift, me: 2 add, 1 shift). I'll try! Thanks for that
Collaborator
Author
There was a problem hiding this comment.
@tfry-git What do you think of the new version? Note the needed addition to startMozzi()
internal/MozziGuts_impl_RP2040.hpp
Outdated
|
|
||
| static void startAudio() { | ||
| #if MOZZI_IS(MOZZI_AUDIO_MODE, MOZZI_OUTPUT_PWM) | ||
| // calling analogWrite for the first time will try to init the pwm frequency and range on all pins. We don't want that happening after we've set up our own, |
Collaborator
There was a problem hiding this comment.
I suppose this comment block is essentially obsolete, then?
| gpio_set_drive_strength(MOZZI_AUDIO_PIN_1, GPIO_DRIVE_STRENGTH_12MA); // highest we can get | ||
| # if (MOZZI_AUDIO_CHANNELS > 1) | ||
| # if ((MOZZI_AUDIO_PIN_1 / 2) != (MOZZI_AUDIO_PIN_1 / 2)) | ||
| # error Audio channel pins for stereo or HIFI must be on the same PWM slice (which is the case for the pairs (0,1), (2,3), (4,5), etc. Adjust MOZZI_AUDIO_PIN_1/2 . |
Collaborator
There was a problem hiding this comment.
Is this a requirement with PWMAudio, too?
Collaborator
Author
There was a problem hiding this comment.
Yup! (Can't find the reference ATM, but I read that somewhere in PWMAudio)
https://arduino-pico.readthedocs.io/en/latest/pwm.html#pwmaudio-pin-true
| # define MOZZI_AUDIO_PIN_2 1 | ||
| # endif | ||
| # define BYPASS_MOZZI_OUTPUT_BUFFER true | ||
| # define MOZZI_RP2040_BUFFERS 8 // number of DMA buffers used |
Collaborator
There was a problem hiding this comment.
It may be worth documenting: Is that 8 the result to experimentation, or would other values be expected to work, too?
Collaborator
Author
There was a problem hiding this comment.
I used the same values that the library example (which are the same than for I2S). Other values might be expected to work, but I did not test thoughtfully.
Closed
Collaborator
Author
|
PS: this incurs a quite penalty on flash, but is more beneficial for RAM in terms of percentage so I think this is worth it. There is no lack of flash on the RP… |
|
Memory usage change @ fc3e0f9
Click for full report table
Click for full report CSV |
tfry-git
reviewed
May 9, 2024
internal/MozziGuts_impl_RP2040.hpp
Outdated
| micros_per_update = 1000000l / MOZZI_AUDIO_RATE; | ||
| do { | ||
| next_audio_update = make_timeout_time_us(micros_per_update); | ||
| next_audio_update_shifted = to_us_since_boot(next_audio_update); |
Collaborator
There was a problem hiding this comment.
I think this is not correct (but wasn't before, either): startMozzi() may not necessarily be called a time 0. Rather (I think):
next_audio_update_shifted = make_timeout_time_us(delayed_by_us(time_us_64(), micros_per_update) << 8;
Collaborator
Author
There was a problem hiding this comment.
Are you sure? I am okay to admit that I forgot a shift there, so more like:
next_audio_update_shifted = to_us_since_boot(next_audio_update) <<8;
But, the next_audio_update = make_timeout_time_us(micros_per_update); increment from the time it is called on (does not need to be zeros), but returns an absolute time. delayed_by_us will return an absolute time, in the future already, and the make_timeout_time_us will give a "timestamp a number of microseconds from the current time.", hence a absolute time, starting at the current time and delayed by (current_time + micros_per_update), returned by the delayed_by_us. I might be wrong though because I am not sure I understand all the absolute_time_t correctly. The shift is missing though.
Collaborator
There was a problem hiding this comment.
You are right, I got confused with relative and absolute timestamps!
|
Memory usage change @ 2b0d752
Click for full report table
Click for full report CSV |
Collaborator
Author
|
Ok for merging this? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #251
Draft for now, until DMA driven PWM is implemented.