0
\$\begingroup\$

I'm working on a piece of code that remaps a set of signals (1-D integer arrays) onto a set of multi channel samples. I hope the typedefs and the remapping clearifies what I mean. If not, please let me know.

#include <stdint.h>

#define SIGNAL_LENGTH 1024


/**
* Multiple linear input signals as read by a previous input stage 
*/
typedef struct {
    // Mics inside
    int32_t micInside0[SIGNAL_LENGTH];
    int32_t micInside1[SIGNAL_LENGTH];
    int32_t micInside2[SIGNAL_LENGTH];
    int32_t micInside3[SIGNAL_LENGTH];
    // Mics aux
    int32_t micAux0[SIGNAL_LENGTH];
    int32_t micAux1[SIGNAL_LENGTH];
    int32_t micAux2[SIGNAL_LENGTH];
    int32_t micAux3[SIGNAL_LENGTH];
} InputSignals_t;


/**
* Single input struct to be passed to the following filtering stage 
*/
typedef struct {
    // Mic inside 0
    int32_t micInside0a;
    int32_t micInside0b;
    int32_t micInside0Select;
    // Mic inside 1
    int32_t micInside1a;
    int32_t micInside1b;
    int32_t micInside1Select;
    // Mic inside 2
    int32_t micInside2a;
    int32_t micInside2b;
    int32_t micInside2Select;
    // Mic inside 3
    int32_t micInside3a;
    int32_t micInside3b;
    int32_t micInside3Select;
} FilterSample_t;

/**
* Map the linear input signals to interleved multi-channel filter input signals. 
*/
void mapMicFilters(InputSignals_t *inputSignals, FilterSample_t filterSamples[SIGNAL_LENGTH]) {
    for(int i = 0; i < SIGNAL_LENGTH; i++) {
        FilterSample_t *filterSample = &filterSamples[i];
        
        // I am rather unhappy with the look of the following 
        // code section where the mapping is done.
        
        // Mic inside 0
        filterSample->micInside0a = inputSignals->micInside0[i];
        filterSample->micInside0b = 0;
        filterSample->micInside0Select = 1;

        // Mic inside 1
        filterSample->micInside1a = inputSignals->micInside0[i];
        filterSample->micInside1b = 0;
        filterSample->micInside1Select = 1;

        // Mic inside 2
        filterSample->micInside2a = inputSignals->micInside0[i];
        filterSample->micInside2b = 0;
        filterSample->micInside2Select = 1;

        // Mic inside 3
        filterSample->micInside3a = inputSignals->micInside0[i];
        filterSample->micInside3b = 0;
        filterSample->micInside3Select = 1;
    }
}

I am rather unhappy with the look of the code section where the mapping is done.The reason for this is that the lines become very long when the signal names become more specific (therefore longer). But a line break within the statements would really mess up the readability:

filterSample->micInside0a = 
    inputSignals->micInside0[i];
filterSample->micInside0b = 
    0;
filterSample->micInside0Select = 
    1; 

Do you have any suggestions on how to format the code? In dynamic languages I would just take a hashmap with the variable names and loop through it. But I have (and want) to use C.

Thank you!

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Whenever I see (e.g.):

int foo0, foo1, foo2, foo3, ..., fooN;

I think that can be replaced more cleanly with an array. So, I'd rather have:

int foo[N];

Thus, I'd change:

int32_t micInside0[SIGNAL_LENGTH];

(et. al.) into:

int32_t micInside[4][SIGNAL_LENGTH];

That sort of thing occurs in several more places for different variables as well.


A good style rule is to use shorter names for argument names and stack/automatic variables [that have shorter scope/duration] and longer names for globals.

For example: FilterSample_t filterSamples[SIGNAL_LENGTH] is a bit redundant and could be shortened to (e.g.) FilterSample_t fslist[SIGNAL_LENGTH]

Likewise, FilterSample_t *filterSample = &filterSamples[i]; is a bit long and could be shortened to: FilterSample_t *fscur = &fslist[i];

To me, the longer names are "too much eye candy".

Also, with the shorter names, the lines will be shorter, so they're less likely to need to be wrapped to multiple lines [with the indenting].


If you add an additional struct, you can reorganize/simplify micInside* and micAux*.


As to the indenting of the second [wrapped] line that you didn't like, personally, I do that 10 times / day. I've found that to be very clean. But, I've been doing that for 20+ years, so I'm used to it.


Note that you did [for all "X", where "X" is one of 0,1,2,3]:

filterSample->micInsideXa = inputSignals->micInside0[i];

But, I think that was a typo and you meant to do:

filterSample->micInsideXa = inputSignals->micInsideX[i];

I realize that school classes teach the use of i, j, etc. But, I've found that more descriptive names can make the code clearer.


Anyway, I've refactored your code incorporating these ideas.

Caveat: I had some difficulty with the remapping to arrays and I had to guess a bit about some of the relationships. (e.g.) CHANMAX for "max # of channels" was a guess, based on my experience with video and audio processing and seeing "mic" (as in "microphone") and "aux" (as in "auxiliary audio").

And I may have messed this up by transposing the array indexes, but it should give you an idea of what I'm driving at:

#include <stdint.h>

#define SIGNAL_LENGTH 1024

#define CHANMAX     4

/**
* Multiple linear input signals as read by a previous input stage
*/
typedef struct {
    // Mics inside
    int32_t micInside[CHANMAX][SIGNAL_LENGTH];

    // Mics aux
    int32_t micAux[CHANMAX][SIGNAL_LENGTH];
} InputSignals_t;

typedef struct {
    int32_t mic_a;
    int32_t mic_b;
    int32_t mic_select;
} micfilter_t;

/**
* Single input struct to be passed to the following filtering stage
*/
typedef struct {
    micfilter_t filt_micinside[CHANMAX];
} FilterSample_t;

/**
* Map the linear input signals to interleved multi-channel filter input signals.
*/
void
mapMicFilters(InputSignals_t *inputSignals, FilterSample_t fslist[SIGNAL_LENGTH])
{
    for (int isamp = 0; isamp < SIGNAL_LENGTH; isamp++) {
        FilterSample_t *fscur = &fslist[isamp];
        InputSignals_t *inpbase = &inputSignals[isamp];

        // I am rather unhappy with the look of the following
        // code section where the mapping is done.

        for (int ichan = 0;  ichan < CHANMAX;  ++ichan) {
            micfilter_t *fchan = &fscur->filt_micinside[ichan];
            int32_t *inpcur = inpbase->micInside[ichan];
            fchan->mic_a = inpcur[isamp];
            fchan->mic_b = 0;
            fchan->mic_select = 0;
        }
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ I like the loop-y thing. The problem with the indexing is that the numbers do not exactly represent my actual code. I might have abstracted it a bit to generically. There are actual names representing the fixed position of the mics. My guess is, the only way to achieve a loop is an extra relation-datastructure that says something like {{.from=MIC_ABC, .to=CHANNEL_XYZ}, {.from=MIC_123, .to=CHANNEL_456}}. That's what I meant by using a hashmap. Maybe I should just give it a try :D Thank you! \$\endgroup\$ Jan 14, 2021 at 10:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.