[Audio library] control_SGTL5000.cpp documentation / improvements

C

CyborgEars

Active member
For our Audio project presented in this thread, we've been using the SGTL5000 hardware abstraction class control_sgtl5000.cpp (part of the audio library created by Paul).
We've found several minor and a few major things that could be improved.

Lacking/misleading function documentation: GEQ settings
When one tries to set the GEQ equalizer ('graphical', 5 band) via
eqBand(uint8_t bandNum, float n), eqBands(float bass, float mid_bass, float midrange, float mid_treble, float treble) or eqBands(float bass, float treble)
one does not know which values correspond to the -11.75dB to +12dB range the GEQ can regulate.
Internally, those equalizer functions call
dap_audio_eq_band(uint8_t bandNum, float n)
which has the following parameter documentation: // by signed percentage -100/+100;
HoweverUsing -100 as an input for an equalizer band is wrong, as we had to find out the hard way!
Figuring out the required values from the datasheet and looking at the function reveal that the input range is in fact limited to -1 to 1.
Also, there is also no code example showing the use of the GEQ, which increases the necessity for better code documentation.

Depending on our project progress, we might be able to push a patch with some better documentation soon.

[more issues will be posted later]
 
Hello Paul,
I'll send you a pull request soon that addresses some of the issues.

I was aware of the GUI and the "helping information" there, but I didn't realize that the documentation for the audio functions would be maintained on an external page instead of in the code itself, so I never took a look at it - which caused some of the confusion that led to my last post.
There is NO reference to the external documentation page in the SGTL5000-code, as far as I know.

It makes sense, in a way, to put the information where most of the users will actually read it. However, when diving into the code to improve it or to borrow one or two subclasses for a different project, working with "undocumented" functions will lead to a lot of headaches (like it did for us).

The documentation list.html has the comment
TODO: generate some or all of this automatically from the C++ source
This seems like a good mid-term solution to me ;)

Further development
The SGTL5000 settings written in the enable() function as well as the AVC settings in autoVolumeControl() are dependent on the sample rate (44100Hz, 48000Hz, ..). Are you planning/using a global DEFINE in the audio library for this setting that one could check / overwrite if missing to make the functions more flexible, and avoid customization?

We've also changed the bit depth / I2S settings for the SGTL, I'ld put that option in an overloaded enable(<more settings>) .

Feature-wise, we've managed to get the dual input mixer working, for which there is no API function at the moment. Basically, we'll need 4 one-line functions that enable/disable the feature registers and set the main channel / mix channel volume.

Another thing: is there an official code style / guideline for the libraries? (Tabs vs. spaces for example)
 
Last edited:
@Paul:
The first pull request is online.
Regarding a more fine-tunable enable() function for the SGTL5000, I'm thinking about which CHIP_CLK_CTRL and CHIP_I2S_CTRL settings to expose:
  • SYS_FS, SCLKFREQ and DLEN are a must for 24-bit
  • MS is necessary to put the SGTL5000 into I2S master (might be useful to someone)
  • ? MCLK_FREQ, if someone doesn't run at 256*Fs
  • ?? format settings (right justified, align, polarity, ..)
 
We've found an important library error:
in the control_sgtl5000.cpp AudioControlSGTL5000::autoVolumeControl(..),
the audio volume limiter value is bogus due to a wrong cast:

uint8_t thresh=(pow(10,threshold/20)*0.636)*pow(2,15);

while the formula is fine - besides minor rounding issues -, the target register expects the full 16 bits result, which is not given!

Corrected code:
uint16_t thresh=(pow(10,threshold/20)*0.636)*pow(2,15);

Impact
This bug caused the upper bits of the DAP_AVC_THRESHOLD register to be zero.
If our calculations are correct, this limited the threshold to values below -38dB, which equals a limiter "stuck in a low volume setting" as an upper boundary of the sound volume. With AVC enabled, this probably led to distortions, clipping and unnecessary strong soft-limiter behavior.

At the moment, it does not look like the ear protection feature itself was negatively influenced, but we're not too sure about this point.
 
Last edited:
I've got to extend the report, things are somewhat worse:

AVC_ATTACK and AVC_DECAY are also affected (code source). They need 12 bit, but also get cast to uint8_t:
uint8_t att=(1-pow(10,-(attack/(20*44100))))*pow(2,19);
uint8_t dec=(1-pow(10,-(decay/(20*44100))))*pow(2,23);
Again, the solution is to use uint16_t instead.

Impact - assessment
As with the AVC_THRESHOLD, this error resulted in the most significant bits cut off, which put an artifical upper bound on the ATTACK and DECAY values as well as writing unexpectedly lower settings to the register.
The bad news here is that this severely limited the effectiveness of the limiter in preventing hearing damage. If the ATTACK value is low, the limiter may take multiple hundreds of milliseconds until it has limited the volume to non-dangerous levels, while the wearer/programmer feels in false security due to his belief in this safety system.
Example: instead of 2500dB/s attack we've had configured, we were left with 50dB/s (value in the lower 8 bits), which was insufficient as protection!


Therefore, I'ld say this requires urgent fixing!
 
Last edited:
Pretty sure I contributed the majority of code you are flagging there, having just reviewed the relevant parts of the datasheet I have to admit that I made bad casts there; would like to believe I didn't but it looks fairly black and white to me - if you are not going to add these corrections to your pull request(s) then I will.
 
You must log in or register to reply here.
Back
Top