libDaisy
libDaisy copied to clipboard
electro-smith
Adds support for averaging to ADC reads
This patch adds support for averaging the ADC reads to improve the stability. This approach is preferable to the current running-average solution because it is completely internal to the driver and provides predictable performance with low CPU overhead. The current patch only support averaging for non-muxed ADC channels because I have no way to test them.
Signed-off-by: Greg Burns [email protected]
Nice, this looks solid, though I think this should be optional at init time. This significantly reduces the effective sample rate of each input channel, which may not be desirable in all use-cases.
Just a few other notes:
I think it would make more sense to move the check for mux into the adc_internal_callback function, and have your additional logic happen there for clarity and ease of maintenance down the road.
the MPU Init in system should be checked to make sure that the new (max_channels * 2* averaging) sized arrays will fit in the Cache-less memory section and not push any other existing DMA buffers outside of that region (It can safely be increased to 64kB or more if needed, there's nothing else currently in that section of RAM other than DMA buffers).
Otherwise everything looks fine. That said, have you compared the stability of this with just using a higher oversampling on the ADC init itself since it would do a similar thing without any cpu overhead or time in an interrupt?
I think your default 128 with the libdaisy oversampling default of 32 isn't really achievable with the hardware oversampling (since that would be 4096 samples) but you could compare ovs=32 and averaging=32 with with just ovs=1024 and they should act fairly similarly.
I did more benchmarking tweaking the oversample rate, sample time, and even clock rate. None of these changes get me the stable 8 bits I am looking for. Even oversampling at 1024 only got me to 7 bits. I suspect a noise source that is relatively low frequency compared to the ADC sample rate, I would be interested on your thoughts about this.
I tested doing an exponential moving average inside the DMA callback and I get much the same result as sum-and-average with less CPU and memory overhead. I will update my patch accordingly. The “smoothing” factor can be passed as an optional parameter to AdcHandle::Start() and DaisyPatch::StartAdc(). This should probably replace the similar exponential moving average in ctrl.cpp which depends on the audio callback frequency. With large audio block you get heavy portamento when doing volt-per-octave tracking. There is still some portamento effect computing the moving average in the DMA callback but it is consistent.
On Mon, Jan 25, 2021, 9:42 AM Stephen Hensley <[email protected] mailto:[email protected] > wrote:
Nice, this looks solid, though I think this should be optional at init time. This significantly reduces the effective sample rate of each input channel, which may not be desirable in all use-cases.
Just a few other notes:
I think it would make more sense to move the check for mux into the adc_internal_callback function, and have your additional logic happen there for clarity and ease of maintenance down the road.
the MPU Init in system should be checked to make sure that the new (max_channels * 2* averaging) sized arrays will fit in the Cache-less memory section and not push any other existing DMA buffers outside of that region (It can safely be increased to 64kB or more if needed, there's nothing else currently in that section of RAM other than DMA buffers).
Otherwise everything looks fine. That said, have you compared the stability of this with just using a higher oversampling on the ADC init itself since it would do a similar thing without any cpu overhead or time in an interrupt?
I think your default 128 with the libdaisy oversampling default of 32 isn't really achievable with the hardware oversampling (since that would be 4096 samples) but you could compare ovs=32 and averaging=32 with with just ovs=1024 and they should act fairly similarly.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/electro-smith/libDaisy/pull/298#issuecomment-766991825 , or unsubscribe https://github.com/notifications/unsubscribe-auth/ACESB67GDWCXOLGVQONMKKLS3WUR7ANCNFSM4WP5JW3Q . https://ci3.googleusercontent.com/proxy/VXwx5r1eESrq0AF37FxIujCFF0tvn1VVkf0EcZ_Rq2DfpqHevEPRyOI9U_2mxMMNrmaxYzF0q0_BolwC8dZUgUqP-GTqFKQGCndqJR7YTQRREwixVd4ArrsKTEUqVdiAoPoyb4NgH43u9I6rcVash4rdPG_ku9D8M0xbGfSTdjD6gzcLtNPd7IVC1W1g9lNRxnbKUDpxGTWjPsWItqefbNz
Thanks for the thorough report, I look forward to seeing the update.
It's good to know you're testing on the patch as well. Due to the summing topology it is probably more susceptible to noise than the other platforms that have discreet ADC reads for pots/CVs.
I have gotten used to having to do a fair amount of clean up for stable values in software (especially in regards to pitch/delay time parameters). Noisey ADCs have been an issue I've noticed on many STM32 boards, not just the daisy. So I don't think its entirely uncommon, but I agree there is more we can do to clean it up.
But usually settle on some amount of hysteresis/filtering so that slew can be kept to a minimum.
Regarding a low-frequency noise source, I'm not sure that I have any profiling or measurements that necessarily agree. However, I wonder if some the processor workload during audio callbacks/heavy ram usage could be causing enough of a fluctuation to dirty up the 16-bit reads.
Have you done your same tests without starting the Audio callback?
Also, just curious, have you tried changing the data size in the HAL to be 8-bit since that's all you're trying to achieve? Not that's a good solution, but may have better results.
I just pushed the update to replace the box-car averaging with an exponential moving average. I did some modelling with a small C program and excel and came up with a slew mitigation solution that I haven’t seen in the literature. A threshold is computed from the smoothing factor and changes above the threshold are passed through. This worked well with my model but works remarkably well on the Patch hardware. With a smoothing factor of 0.04 I am getting 16 bit stability which seems crazy.
I looked at putting the averaging into the muxed code but don’t feel confidant about getting it right without a hardware platform to test on. There also didn’t seem to be any benefit in moving the averaging code into adc_internal_callback() which should probably be better called adc_muxed_callback().
I benchmarked raw reads with and without the audio callback. With the audio callback bit stability is only 5-6 bits, without the audio callback it is 9-10 bits so a big difference.
BTW, I isn’t that I only wanted 8 bit stability, that was the minimum I could possibly live with. What I really want is 5 octaves accurate to within a few cents, so more like 12 bits.
My methodology for computing bit stability is to track the min and max ADC reads on each of the four channels for several seconds and subtract the bits needed to represent the min max delta from 16. I then write the results to the OLED screen.
From: Stephen Hensley [email protected] Sent: Tuesday, January 26, 2021 5:17 PM To: electro-smith/libDaisy [email protected] Cc: Greg Burns [email protected]; Author [email protected] Subject: Re: [electro-smith/libDaisy] Adds support for averaging to ADC reads (#298)
Thanks for the thorough report, I look forward to seeing the update.
It's good to know you're testing on the patch as well. Due to the summing topology it is probably more susceptible to noise than the other platforms that have discreet ADC reads for pots/CVs.
I have gotten used to having to do a fair amount of clean up for stable values in software (especially in regards to pitch/delay time parameters). Noisey ADCs have been an issue I've noticed on many STM32 boards, not just the daisy. So I don't think its entirely uncommon, but I agree there is more we can do to clean it up.
But usually settle on some amount of hysteresis/filtering so that slew can be kept to a minimum.
Regarding a low-frequency noise source, I'm not sure that I have any profiling or measurements that necessarily agree. However, I wonder if some the processor workload during audio callbacks/heavy ram usage could be causing enough of a fluctuation to dirty up the 16-bit reads.
Have you done your same tests without starting the Audio callback?
Also, just curious, have you tried changing the data size in the HAL to be 8-bit since that's all you're trying to achieve? Not that's a good solution, but may have better results.
— You are receiving this because you authored the thread. Reply to this email directly, https://github.com/electro-smith/libDaisy/pull/298#issuecomment-767940023 view it on GitHub, or https://github.com/notifications/unsubscribe-auth/ACESB63QTESA3XJHVHIUZXDS35SQVANCNFSM4WP5JW3Q unsubscribe. https://github.com/notifications/beacon/ACESB655RVC3NK3W4BDO5PDS35SQVA5CNFSM4WP5JW32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOFXC5LNY.gif
Wow, impressive improvements! This all looks like its pretty much mergeable as is. I'll look it over a bit more later today, and can hopefully squeeze out some time to test it today or Monday.
This worked well with my model but works remarkably well on the Patch hardware. With a smoothing factor of 0.04 I am getting 16 bit stability which seems crazy.
That does seem crazy! I'm looking forward to testing this out. I can test on the Petal/Pod as well (with and without the AnalogControl class).
I benchmarked raw reads with and without the audio callback. With the audio callback bit stability is only 5-6 bits, without the audio callback it is 9-10 bits so a big difference.
Yeah, I suspected something similar. Not necessarily with that magnitude, but good to know.
I looked at putting the averaging into the muxed code but don’t feel confidant about getting it right without a hardware platform to test on. There also didn’t seem to be any benefit in moving the averaging code into adc_internal_callback() which should probably be better called adc_muxed_callback().
Yeah, that's fine for now. I mostly just wanted it abstracted to a central function for readability and for when we remove some of the HAL bits from stuff. This was more of a concern if it was going to be using both the Cplt and HalfCplt callbacks, but without repetitive code it is fine as is for now.
I can look into adding support to this for the muxed code reads as well once its merged, so no worries there.
Anyway I look forward to testing this and getting it merged! Thanks for the contribution!
Hey @GregBurns
Finally got around to testing this a bit, and it's definitely promising!
I've added the following test code into an existing project and am toggling between your fork, and the current master branch with the following results:
if(daisy::System::GetNow() - adc_t > 1000)
{
adc_t = daisy::System::GetNow();
adc_min = adc_max = adc_raw;
}
adc_raw = hw.seed.adc.Get(0);
adc_min = adc_raw < adc_min ? adc_raw : adc_min;
adc_max = adc_raw > adc_max ? adc_raw : adc_max;
adc_delta = (adc_max - adc_min);
With default setting of the ADC (smoothing = 1.0) the results are pretty much the same.
Decreasing the smoothing by only a small amount to anything between say, 0.2 to 0.9 results in very steppy and a huge loss of resolution.
Going further to your stable value of 0.04 I start to see the benefits. While there is still a loss in resolution it does become stable, and much more responsive than at the higher settings I tried.
Using this on the hardware I have on hand right now, I'm testing with an ADC that's connected to a single pot (no mux, no CV summing circuit) I can reduce smoothing all the way down to 0.001 and still have monostable values. On the same board connected to a bi-polar CV input there is a little bit more of a settling time before the monostability kicks in with the same settings, but it does become stable.
I imagine on a board with summed controls and a display (i.e. daisy patch) the values may need to be a bit higher for stability.
I agree that this does make the ADCs usable without the one pole filtering in the AnalogControls, but I think some smoothing is should still be applied to avoid some of the steppy-ness, especially at higher smoothing settings.
I think this should be good to merge, but I'm going to do a few more tests later (profile the callback timing, etc.), and make sure there's nothing I'm overlooking.
Overall, though this is awesome. Thanks for the contribution!
Hello,
@GregBurns :
I just pushed the update to replace the box-car averaging with an exponential moving average. I did some modelling with a small C program and excel and came up with a slew mitigation solution that I haven’t seen in the literature. A threshold is computed from the smoothing factor and changes above the threshold are passed through.
This is a basic hysteresis filter : it's very good to cut noise, but output can jump from 1 value to an other unexpectedly (if the noise follow a Gaussian curve). Moreover, if the hysteresis value is too high, the output will not be the same depending on the previous value. I.E : going from 0 to 1V will gives a different output than going from 2V to 1V. This is not necessary a problem for a potentiometer control , but must be avoid for a CV controlling a pitch.
Anyway, for my nozori (nozoid.com) modules, I get 7.5 stable bit from the 12 bit ADC. I wanted a filter with :
- low portamento effect
- no hysteresis effect
- no unexpected jump
- more than 12 bit real accuracy (go through all value)
I finally made a strange Frankenstein stuff, mixing FIR, hysteresis and IIR filter : https://github.com/ch-nry/nozori/blob/master/c_macro.h#L464-L485 (order is 6 in my application) It get the min and max from the last 6 values and use this as the start of the hysteresis filter mixed with a IIR filter. All computation are made using int, since the hardware did not have any floating point instruction, REG_pot is a 12 bit value, so input is a 16 bit value. 60 is the hysteresis value: it look high, but it's only 6 bit from a 16 bit value. the FIR is because of the >> 4. I did not spend much time optimizing it and tweaking it, since it does what I wanted.
The result are impressive : it' smooth, fast and accurate. The only "problem" is it's a bit CPU intensive, but "you can't make an omelet without breaking some eggs"! One can probably make something simpler using a median filter followed by a Hysteresis/IIR filter. Using a median first is important as it remove extreme value of the noise.
@stephenhensley : I did not have time to make a patch for libdaisy, but feel free to test and adapt this filter to your need. (licence is GPL v3)
@ch-nry thanks for your thoughts and feedback!
I too usually hack together some hybrid solution in software for my needs. Unfortunately, libdaisy being licensed MIT we can't adapt your filter.
That said, I did have a thought on this being a bit more flexible, in one of two ways @GregBurns I'll be curious on your thoughts here as well.
- We move the "smoothing" factor to the AdcChannelConfig so that each channel can have it's own smoothing value (and probably rename it "hyst" or something a bit more fitting. (An optional argument could be added to the InitSingle function that has this disabled by default)
- We move the filtering aspect of this out to the AnalogControl, and add the hysteresis filtering before the existing one pole. At that point it would be operating on the same "value" as inside, but inside the isolated control class that has a bit more flexibility.
The problem with putting the filtering in the analog control is that is then depends on the rate that the Process method is called. This was not working at all well for my use case with 256 sample block size and 32kHz sample rate.
I have made one small addition to the hysteresis side which I am still working on. Rather than simply setting the avg to the input value I take the mean of the current avg and the input value. This has little impact on CPU usage and is closer to the @ch-nry approach. In my modelling this is less susceptible to large noise spikes and tracks slow changes better. In my modelling, even of my current version I don't see any large steps, in fact I'm not sure how they would arise.
On Mon, Feb 8, 2021, 8:48 AM Stephen Hensley [email protected] wrote:
@ch-nry https://github.com/ch-nry thanks for your thoughts and feedback!
I too usually hack together some hybrid solution in software for my needs. Unfortunately, libdaisy being licensed MIT we can't adapt your filter.
That said, I did have a thought on this being a bit more flexible, in one of two ways @GregBurns https://github.com/GregBurns I'll be curious on your thoughts here as well.
- We move the "smoothing" factor to the AdcChannelConfig so that each channel can have it's own smoothing value (and probably rename it "hyst" or something a bit more fitting. (An optional argument could be added to the InitSingle function that has this disabled by default)
- We move the filtering aspect of this out to the AnalogControl, and add the hysteresis filtering before the existing one pole. At that point it would be operating on the same "value" as inside, but inside the isolated control class that has a bit more flexibility.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/electro-smith/libDaisy/pull/298#issuecomment-775283031, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACESB67TGDECWNV6SUFSINDS6AIWXANCNFSM4WP5JW3Q .
Correction, the averaging prevents overshoot/undershoot on step changes. It doesn't have any noticeable effect on smooth changes. Also this isn't traditional hysteresis which supressess changes that fall below some threshold, the threshold in this case lets changes larger than the threshold to bypass the moving averaging to prevent portamento.
On Mon, Feb 8, 2021, 9:50 AM [email protected] wrote:
The problem with putting the filtering in the analog control is that is then depends on the rate that the Process method is called. This was not working at all well for my use case with 256 sample block size and 32kHz sample rate.
I have made one small addition to the hysteresis side which I am still working on. Rather than simply setting the avg to the input value I take the mean of the current avg and the input value. This has little impact on CPU usage and is closer to the @ch-nry approach. In my modelling this is less susceptible to large noise spikes and tracks slow changes better. In my modelling, even of my current version I don't see any large steps, in fact I'm not sure how they would arise.
On Mon, Feb 8, 2021, 8:48 AM Stephen Hensley [email protected] wrote:
@ch-nry https://github.com/ch-nry thanks for your thoughts and feedback!
I too usually hack together some hybrid solution in software for my needs. Unfortunately, libdaisy being licensed MIT we can't adapt your filter.
That said, I did have a thought on this being a bit more flexible, in one of two ways @GregBurns https://github.com/GregBurns I'll be curious on your thoughts here as well.
- We move the "smoothing" factor to the AdcChannelConfig so that each channel can have it's own smoothing value (and probably rename it "hyst" or something a bit more fitting. (An optional argument could be added to the InitSingle function that has this disabled by default)
- We move the filtering aspect of this out to the AnalogControl, and add the hysteresis filtering before the existing one pole. At that point it would be operating on the same "value" as inside, but inside the isolated control class that has a bit more flexibility.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/electro-smith/libDaisy/pull/298#issuecomment-775283031, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACESB67TGDECWNV6SUFSINDS6AIWXANCNFSM4WP5JW3Q .
Cool, I can test with the latest changes either later today or tomorrow some time.
How do you feel about moving this the ChannelConfig struct and having it be configurable per-channel so that there is some flexibility wrt applying this only when needed on certain inputs?
@GregBurns So I just merged #303 which adds the ability to override the internally computed filter coefficient with your own value (and also get the raw ADC values).
Might be worth seeing if you can run your filter inside the AnalogControl class with coeff = 1.0 with similar results to what you see in the ADC callback.
The main benefit to being able to have this be an optional control within the AnalogControl is it eliminates the need to work in the same thing for mux inputs.
That said, I'd be just as happy having it be in the channel configuration struct. That way you could have 8 pots connected to a mux with no hysteresis/smoothing factor, and then 4 CVs with values well tuned for their requirements.
I am not clear if you saying put my filter inside AnalogControl::Process() instead of in the DMA callback? I have tried reading raw values and doing my own filtering inside the audio callback but the sample rate is too low to get decent smoothing. At the default audio configuration (48 samples at 48KHz) AnalogControl::Process() is called at 1KHz which might be ok for many applications. My application is a multi-channel phase-vocoder pitch shifter that processes 256 sample blocks running the audio clock 32KHz so AnalogControl::Process() is only called at 125Hz.
If you are ok with my filtering algorithm it could be enabled/disabled with a per channel bool (or bit) or just zeroing the smoothing/hysteresis coefficient. If you think maximum flexibility is needed it could be handled by registering a per-channel (or muxed channel) filter callback function. No callback, no filtering.
From: Stephen Hensley [email protected] Sent: Wednesday, February 10, 2021 10:07 AM To: electro-smith/libDaisy [email protected] Cc: Greg Burns [email protected]; Mention [email protected] Subject: Re: [electro-smith/libDaisy] Adds support for averaging to ADC reads (#298)
@GregBurns https://github.com/GregBurns So I just merged #303 https://github.com/electro-smith/libDaisy/pull/303 which adds the ability to override the internally computed filter coefficient with your own value (and also get the raw ADC values).
Might be worth seeing if you can run your filter inside the AnalogControl class with coeff = 1.0 with similar results to what you see in the ADC callback.
The main benefit to being able to have this be an optional control within the AnalogControl is it eliminates the need to work in the same thing for mux inputs.
That said, I'd be just as happy having it be in the channel configuration struct. That way you could have 8 pots connected to a mux with no hysteresis/smoothing factor, and then 4 CVs with values well tuned for their requirements.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/electro-smith/libDaisy/pull/298#issuecomment-776904644 , or unsubscribe https://github.com/notifications/unsubscribe-auth/ACESB636EGILQH3NVID2BDDS6LDMNANCNFSM4WP5JW3Q . https://github.com/notifications/beacon/ACESB64UJHV6HHFYRW3QSWDS6LDMNA5CNFSM4WP5JW32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOFZHJ7RA.gif
@GregBurns sorry for the confusion:
I am not clear if you saying put my filter inside AnalogControl::Process() instead of in the DMA callback? I have tried reading raw values and doing my own filtering inside the audio callback but the sample rate is too low to get decent smoothing. At the default audio configuration (48 samples at 48KHz) AnalogControl::Process() is called at 1KHz which might be ok for many applications. My application is a multi-channel phase-vocoder pitch shifter that processes 256 sample blocks running the audio clock 32KHz so AnalogControl::Process() is only called at 125Hz.
I forgot that you had the callback running at that low frequency. That totally makes sense, I thought it was the fixed-slew rate that was getting in the way of trying your filter in that loop. No problem there.
What I was proposing was moving your global adc.Start(float smoothing) factor to the AdcChannelConfig struct so that each channel could have it's own smoothing factor. During the init function the necessary threshold value could be computed.
That would effectively change your callback loop to:
for(int c = 0; c < adc.channels; ++c)
{
int delta = adc.dma_buffer[c] - adc.out_buffer[c];
if(abs(delta) > adc.threshold[c]) // < change threshold to array of values (one per channel)
adc.out_buffer[c]
= (adc.out_buffer[c] + adc.dma_buffer[c]) / 2;
else
adc.out_buffer[c] = (uint16_t)(adc.out_buffer[c]
+ delta * adc.smoothing[c]); // < change smoothing to array of values (one per channel)
}
Ok it get it but I don’t see a nice way to do this via AdcChannelConfig. AdcChannelConfig is an automatic variable declared inside DaisyPatch::InitControls() so the smoothing coefficient would need to be set inside this method. But InitControls() is private and called from DaisyPatch::Init() and even if InitControls() was made public and parameterized it doesn’t look to me like it’s idempotent, or at least AdcHandle::Init() is not. AdcChannelConfig is use the same way for initializing the ADC for the Field, Pod, and Petal platforms.
It looks like the cleanest approach is to add a filtering configuration method to AdcHandle:
void ConfigFilter(uint8_t chn, float filterCoeff);
Then just call this for each channel from DaisyPatch::StartAdc()
void DaisyPatch::StartAdc(float filterCoeff)
{
for(size_t i = 0; i < CTRL_LAST; i++)
{
seed.adc.ConfigFilter(I, filterCoeff);
}
}
If you agree to this approach I can make the changes.
From: Stephen Hensley [email protected] Sent: Wednesday, February 10, 2021 4:36 PM To: electro-smith/libDaisy [email protected] Cc: Greg Burns [email protected]; Mention [email protected] Subject: Re: [electro-smith/libDaisy] Adds support for averaging to ADC reads (#298)
@GregBurns https://github.com/GregBurns sorry for the confusion:
I am not clear if you saying put my filter inside AnalogControl::Process() instead of in the DMA callback? I have tried reading raw values and doing my own filtering inside the audio callback but the sample rate is too low to get decent smoothing. At the default audio configuration (48 samples at 48KHz) AnalogControl::Process() is called at 1KHz which might be ok for many applications. My application is a multi-channel phase-vocoder pitch shifter that processes 256 sample blocks running the audio clock 32KHz so AnalogControl::Process() is only called at 125Hz.
I forgot that you had the callback running at that low frequency. That totally makes sense, I thought it was the fixed-slew rate that was getting in the way of trying your filter in that loop. No problem there.
What I was proposing was moving your global adc.Start(float smoothing) factor to the AdcChannelConfig struct so that each channel could have it's own smoothing factor. During the init function the necessary threshold value could be computed.
That would effectively change your callback loop to:
for(int c = 0; c < adc.channels; ++c) { int delta = adc.dma_buffer[c] - adc.out_buffer[c]; if(abs(delta) > adc.threshold[c]) // < change threshold to array of values (one per channel) adc.out_buffer[c] = (adc.out_buffer[c] + adc.dma_buffer[c]) / 2; else adc.out_buffer[c] = (uint16_t)(adc.out_buffer[c] + delta * adc.smoothing[c]); // < change smoothing to array of values (one per channel) }
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/electro-smith/libDaisy/pull/298#issuecomment-777135406 , or unsubscribe https://github.com/notifications/unsubscribe-auth/ACESB6YXN4242TKJMJTKKT3S6MRALANCNFSM4WP5JW3Q . https://github.com/notifications/beacon/ACESB65ELAJZ75YQEUMYXKLS6MRALA5CNFSM4WP5JW32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOFZJCKLQ.gif
I just pushed the change for setting the filter coefficient per channel. This is substantially the same as the filtering that the control class does. I abandoned the slew mitigation for now, it works on my model but there is something very different going on with the real hardware. The crazy 16 bit stability I thought I was seeing earlier was unfortunately just a bug in my code, I had somehow deleted a line and was effectively masking off the low 9 bits ☹ . I admit I was very suspicious , the old adage that “if is something looks too good to be true then it is” definitely applies here.
The filtering code is now in adc_internal_callback() which handles muxed and un-muxed pins. Filtering is only being applied to the un-muxed pins. I should note that the comment before the adc_internal_callback() declaration is now incorrect.
Earlier in this thread you had suggested testing with the audio loop disabled. With the filtering code in place this now makes an enormous difference. Running without the audio loop I get 13 stable bits, after starting the audio loop I get 7-8 stable bits. I thought this might be because my audio loop is very CPU intensive, I’m running 6 FFT’s on each call the audio callback but even doing nothing in the audio callback it’s back to 7-8 bits. I would seem to make more sense if CPU usage in the audio callback was an issue. Audio and adc both use DMA do you have any theories about how they might be interfering?
From: Stephen Hensley [email protected] Sent: Wednesday, February 10, 2021 7:22 PM To: electro-smith/libDaisy [email protected] Cc: Greg Burns [email protected]; Mention [email protected] Subject: Re: [electro-smith/libDaisy] Adds support for averaging to ADC reads (#298)
Agreed. That works for me, and is a clean solution.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/electro-smith/libDaisy/pull/298#issuecomment-777180757 , or unsubscribe https://github.com/notifications/unsubscribe-auth/ACESB62AQNF3ZUKDSNZB5N3S6NEPBANCNFSM4WP5JW3Q . https://github.com/notifications/beacon/ACESB66B3WMDKPKGWQKUDSLS6NEPBA5CNFSM4WP5JW32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOFZJNMVI.gif
I tried moving the ADC to DMA2 and also putting the ADC buffer into SDRAM. If there was AHB bus contention according to my reading of the various documentation this should have eliminated it but there was not change in behavior, still seeing only 7-8 bit stability. So I tried a different tack, I commented out the code that starts DMA on the external codec – bingo ADC reads go up to 12-13 bits stability. I cannot say definitely that this is not still a bus contention issue but I strongly doubt that because dropping the audio sample rate down to 8KHz actually makes things worse.
It doesn’t seem that the I/O pins are clocked when DMA is not enabled SAI2 so I am thinking the ADC noise may be coming from the external codec serial I/O. I say serial I/O because it doesn't make a difference when streaming silence vs signal so the noise is probably not on the analog side.
From: Greg Burns [email protected] Sent: Friday, February 12, 2021 6:11 PM To: 'electro-smith/libDaisy' < [email protected]> Subject: RE: [electro-smith/libDaisy] Adds support for averaging to ADC reads (#298)
I just pushed the change for setting the filter coefficient per channel. This is substantially the same as the filtering that the control class does. I abandoned the slew mitigation for now, it works on my model but there is something very different going on with the real hardware. The crazy 16 bit stability I thought I was seeing earlier was unfortunately just a bug in my code, I had somehow deleted a line and was effectively masking off the low 9 bits ☹ . I admit I was very suspicious , the old adage that “if is something looks too good to be true then it is” definitely applies here.
The filtering code is now in adc_internal_callback() which handles muxed and un-muxed pins. Filtering is only being applied to the un-muxed pins. I should note that the comment before the adc_internal_callback() declaration is now incorrect.
Earlier in this thread you had suggested testing with the audio loop disabled. With the filtering code in place this now makes an enormous difference. Running without the audio loop I get 13 stable bits, after starting the audio loop I get 7-8 stable bits. I thought this might be because my audio loop is very CPU intensive, I’m running 6 FFT’s on each call the audio callback but even doing nothing in the audio callback it’s back to 7-8 bits. I would seem to make more sense if CPU usage in the audio callback was an issue. Audio and adc both use DMA do you have any theories about how they might be interfering?
From: Stephen Hensley [email protected] Sent: Wednesday, February 10, 2021 7:22 PM To: electro-smith/libDaisy [email protected] Cc: Greg Burns [email protected]; Mention <[email protected]
Subject: Re: [electro-smith/libDaisy] Adds support for averaging to ADC reads (#298)
Agreed. That works for me, and is a clean solution.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/electro-smith/libDaisy/pull/298#issuecomment-777180757, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACESB62AQNF3ZUKDSNZB5N3S6NEPBANCNFSM4WP5JW3Q .
Sorry for the extreme delay getting back to this. I'm not ignoring it.
I still want to do a little more testing/profiling with the filters running, but this is probably fine as-is.
I'm also still considering some long term changes to the ADC driver that would further remedy the issue that caused this in the first place (basically control over sample rate and/or some sort of synchronization with the audio engine).
A few of the things with this addition may have unexpected behavior on previously functional firmware (due to the change in priority of the callback, and the added time spent in the callback for averaging the incoming signals).
Between these things, and the potential for an upcoming refactor of the ADC class in general (which will allow a user-exposed callback to either supplement, or replace the existing internal-only callback), I'm going to close this PR for the time being.
If you're interested in participating in the refactor, with an exposed user callback that would allow for averaging in the way it's been added here, let me know and we can start discussing what I have in mind, and what the steps toward implementation would involve.
There are a few other things upcoming that we're working on getting into libDaisy, and then the ADC rework would begin. This will most likely start in a few months time.