Magic Lantern Forum

Developing Magic Lantern => General Development => Topic started by: kitor on July 23, 2015, 11:41:20 AM

Title: Digging into audio code
Post by: kitor on July 23, 2015, 11:41:20 AM
As I had hardware problem with right mic channel in my 5d2 (http://www.magiclantern.fm/forum/index.php?topic=15541.0), I played with source code a bit.
My first thoughts - while audio-lapis.c is using tons of obvious named constants, old "audio-ak.c" is documented very poorly. They have even identical headers, so for "new" person like me it took a while
to understand why they both exist.
Also, audio-ak has different styles of coding mixed, eg function declared in single line:
static inline unsigned mgain_index2gain(int index) // sorted mgain values
while a bit later
static inline uint8_t
audio_gain_to_cmd(
                  int                   gain
                  )
{


I found also things like (in void audio_configure( int force ) )
#ifdef CONFIG_500D
    audio_ic_write( AUDIO_IC_SIG4 | pm3[input_source] );
#else
    //PM3 is set according to the input choice
    audio_ic_write( AUDIO_IC_PM3 | pm3[input_source] );
#endif
   

and guess what... AUDIO_IC_SIG4 and AUDIO_IC_PM3 have the same addresses (0x3000), and that's the only one reference of this register I could find in source...
ofc I get it, by registers definition I belive that 500d uses other (yet similar) controller and thus different register names, but i can't find information what controller is used.

As I had to (first understand at all and then) modify audio_configure function, now it looks more like (ofc it's not final form, and only part of function, note comments with what each bit of register does)

//this should be moved to header file

//left channel path is controlled by D0 of AUDIO_IC_PM1
#define ML_RECORD_PATH_MICL2LCH 0x01
#define ML_RECORD_PATH_MICR2LCH 0x00
//right channel path is controlled by D0 of AUDIO_IC_PM3
#define ML_RECORD_PATH_MICL2RCH 0x00
#define ML_RECORD_PATH_MICR2RCH 0x01
//right channel is defined by D2 of AUDIO_IC_PM3
#define ML_RCH_MIXER_INPUT_SINGLE_INT     0x00
#define ML_RCH_MIXER_INPUT_SINGLE_EXT     0x04

//left channel is defined by D1 of AUDIO_IC_PM3
#define ML_LCH_MIXER_INPUT_SINGLE_INT     0x00
#define ML_LCH_MIXER_INPUT_SINGLE_EXT     0x02

//differential on left is
#define ML_MIC_IF_CTL_SINGLE  0x00
#define ML_MIC_IF_CTL_DIFFER  0x10

#define ML_AUDIO_ENABLE_ADC_DAC 0x6C

#define ML_SPK_ENABLE 0x10
#define ML_MIC_POWER_ON 0x04
#define ML_MIC_POWER_OFF 0x00

#define ML_MIXER_MONO_INPUT 0x04

    if( !force )
        {
            // Check for ALC configuration; do nothing if it is
            // already disabled
            if( audio_ic_read( AUDIO_IC_ALC1 ) == gain.alc1
                &&  audio_ic_read( AUDIO_IC_SIG1 ) == gain.sig1
                &&  audio_ic_read( AUDIO_IC_SIG2 ) == gain.sig2
                )
                return;
            DebugMsg( DM_AUDIO, 3, "%s: Reseting user settings", __func__ );
        }

    audio_ic_write( AUDIO_IC_PM1 | ML_AUDIO_ENABLE_ADC_DAC | ML_RECORD_PATH_MICL2LCH ); // power up ADC and DAC

#ifdef CONFIG_500D //500d only has internal mono audio :(
    int input_source = 0;
    int mic_pow = 1;
#else
    int input_source = get_input_source();
    #ifdef FEATURE_MIC_POWER
        //mic_power is forced on if input source is 0 or 1
        int mic_pow = get_mic_power(input_source);
    #else
        int mic_pow = 1;
    #endif
#endif
    audio_ic_write( AUDIO_IC_SIG1 | ML_SPK_ENABLE | ( mic_pow ? ML_MIC_POWER_ON : ML_MIC_POWER_OFF )); // power up, no gain
    audio_ic_write( AUDIO_IC_SIG2 | ML_MIXER_MONO_INPUT | ( lovl & 0x3) << 0 // line output level
                    );

    //PM1 holds ADC & DAC enable + LR select on L channel
    //PM3 holds INT/EXT select for L and R channels, balanced mode and LR select on R channel
    switch (input_source)
        {
        case 0: //LR internal mic
            audio_ic_write( AUDIO_IC_PM3 | ML_LCH_MIXER_INPUT_SINGLE_INT | ML_RCH_MIXER_INPUT_SINGLE_INT | ML_RECORD_PATH_MICL2RCH);
            audio_ic_write( AUDIO_IC_PM1 | ML_AUDIO_ENABLE_ADC_DAC | ML_RECORD_PATH_MICL2LCH );
            break;
        case 1:// L internal R extrenal
            audio_ic_write( AUDIO_IC_PM3 | ML_LCH_MIXER_INPUT_SINGLE_INT | ML_RCH_MIXER_INPUT_SINGLE_EXT | ML_RECORD_PATH_MICR2RCH);
            audio_ic_write( AUDIO_IC_PM1 | ML_AUDIO_ENABLE_ADC_DAC | ML_RECORD_PATH_MICL2LCH );
            break;
        case 2:// LR external
            audio_ic_write( AUDIO_IC_PM3 | ML_LCH_MIXER_INPUT_SINGLE_EXT | ML_RCH_MIXER_INPUT_SINGLE_EXT | ML_RECORD_PATH_MICR2RCH);
            audio_ic_write( AUDIO_IC_PM1 | ML_AUDIO_ENABLE_ADC_DAC | ML_RECORD_PATH_MICL2LCH );
            break;
        case 3:// L internal R balranced (used for test)
            audio_ic_write( AUDIO_IC_PM3 | ML_MIC_IF_CTL_DIFFER | ML_RECORD_PATH_MICR2RCH); //0x11
            audio_ic_write( AUDIO_IC_PM1 | ML_AUDIO_ENABLE_ADC_DAC | ML_RECORD_PATH_MICL2LCH );
            break;
        case 5:// External mono left
            audio_ic_write( AUDIO_IC_PM3 | ML_LCH_MIXER_INPUT_SINGLE_EXT | ML_RCH_MIXER_INPUT_SINGLE_EXT | ML_RECORD_PATH_MICL2RCH);
            audio_ic_write( AUDIO_IC_PM1 | ML_AUDIO_ENABLE_ADC_DAC | ML_RECORD_PATH_MICL2LCH );
            break;
        case 6:// External mono right
            audio_ic_write( AUDIO_IC_PM3 | ML_LCH_MIXER_INPUT_SINGLE_EXT | ML_RCH_MIXER_INPUT_SINGLE_EXT | ML_RECORD_PATH_MICR2RCH);
            audio_ic_write( AUDIO_IC_PM1 | ML_AUDIO_ENABLE_ADC_DAC | ML_RECORD_PATH_MICR2LCH );
            break;
        }


Yep, I added myself two possibilities of external left and right channel mono, but this I want to discuss later :)

What I want to discuss here is cleanup of this code. If that will be approved (it has chances to be included in main repo), i'm thinking about doing following things:
- Move all controller-specific constants from audio.h to audio-ak.h and audio-lapis.h, as they differs due to registers.
- Cleanup audio-ak.c to have one style of coding
- Where possible, replace hardcoded values with constants, like in my example code, that is based on how audio-lapis.c looks (it's much more human-readable this way)
- Add comments about functions of each register used, as at least in ak4646 they are mixed.
- Optional: Add "external mono left" and "external mono right" - they are useful also when you have eg. xlr to stereo jack cable connected to mono microphone, or have y-adapter with two mics each on own channel, and want to select it (already tested by me on 5d2 and works ok)

But of course I need approval so I don't waste my time :), and some suggestions if there are some internal rules of how code should look like.



Title: Re: Digging into audio code
Post by: dmilligan on July 23, 2015, 09:48:09 PM
have you looked at: https://bitbucket.org/hudson/magic-lantern/branch/new-sound-system
Title: Re: Digging into audio code
Post by: kitor on July 24, 2015, 04:14:07 PM
No, and that's one of reasons why I asked :) So looks like EOT in terms of old code, what's left from my post is
Quote- Optional: Add "external mono left" and "external mono right" - they are useful also when you have eg. xlr to stereo jack cable connected to mono microphone, or have y-adapter with two mics each on own channel, and want to select it (already tested by me on 5d2 and works ok)
but it seems to be easy to implement in new branch.