Efficient Average of Three Values in C

Question

We are reading some signals from pins and setting some more events based on this reading.

To be safe, I want to sample the pins 3 times, compare the three values and use the most common value (i.e. sample A is 1, B is 3 and C is 1, I want to use 1, if A B and C are all 2 then use 2 but if A is 1 , B is 2 and C is 3, I want to capture the three samples again).

Currently I'm using:

int getCAPValues (void)
{
//  Get three samples to check CAP signals are stable:  

        uint32_t x = (PORT->Group[IN_PORT_CAP].IN.reg & IN_PORT_CAP_MASK) >> IN_PORT_CAP_PIN;           // First set of CAP values
        for (uint32_t i = 0; i < 7; i++) dummy = i;                                                     // Pause
        uint32_t y = (PORT->Group[IN_PORT_CAP].IN.reg & IN_PORT_CAP_MASK) >> IN_PORT_CAP_PIN;           // second set
        for (uint32_t i = 0; i < 7; i++) dummy = i;                                                     // Pause
        uint32_t z = (PORT->Group[IN_PORT_CAP].IN.reg & IN_PORT_CAP_MASK) >> IN_PORT_CAP_PIN;           // third set

        if (x == y) || (x == z)
        {
            //use the x value
        }
        else if (y == z)
        {
            // use the y value
            x = y;
        }
        else
        {           
            x = -1;
        }

    return x;
}

But this doesn't seem very efficient to me, is there a better way to do this?

This is on a SAMD21 Xplained Pro development board in C.

EDIT:

I have changed the code as per answers, only reading the "z" value if it will be used, and using delay_us() instead of the dummy loops:

int getCAPValues (void)
{
//  Get three samples to check CAP signals are stable:  

        uint32_t x = (PORT->Group[IN_PORT_CAP].IN.reg & IN_PORT_CAP_MASK) >> IN_PORT_CAP_PIN;           // First set of CAP values
        delay_us(1);
        //for (uint32_t i = 0; i < 7; i++) dummy = i;                                                   // Pause
        uint32_t y = (PORT->Group[IN_PORT_CAP].IN.reg & IN_PORT_CAP_MASK) >> IN_PORT_CAP_PIN;           // second set
        // Using most common value, or error code of -1 if all different

        if (!(x == y))
        {
            delay_us(1);
            //for (uint32_t i = 0; i < 7; i++) dummy = i;                                               // Pause
            uint32_t z = (PORT->Group[IN_PORT_CAP].IN.reg & IN_PORT_CAP_MASK) >> IN_PORT_CAP_PIN;       // third set
            if (x == z)
            {
                // use the x/z value
                return x;
            }
            else if (y == z)
            {
                // use the y/z value
                return y;
            }
            else
            {           
                return -1;
            }
        }
    return x;
}

Arthur

compiler might optimise out for (uint32_t i = 0; i < 7; i++) dummy = i; as it's a no-op. Use a sleep instead. – Multifold

The tests seems efficient, though not clear on their purpose. Rather than saying //use the x value, have a single comment before //use the most common value among x,y,z and store it in z. It's -1 if all values are different. – Cropper

sounds like more suited for codereview.stackexchange.com – Gatias

I think the logic for value selection cannot be further improved. Why do you think it is inefficient? – Stepmother

There's no point in testing z if x == y, but aside from that, it looks pretty optimal to me. Perhaps though the OP wants the function to take broadly the same amount of time on each iteration. Although, you need to fix that "delay" loop. – Multifold

After a bit of hunting, I'm not sure if Atmel Studio/SAMD21 work with sleep() for some reason, the closest I can find is "delay_us()". I thought C had sleep(), but the compiler doesn't recognise it, hence the original dummy loop. – Arthur

You're computing the "mode", not an "average". – Hilversum

@aschepler: The mode is one type of average. Other common types are the median and the arithmetic mean. – Senator

Answer 1

If x==y you're going to use the value of x. So in that case you can dodge the third reading.

I don't know how volatile your values are but it could effectively almost double peformance to avoid that second delay if disputed values are in fact rare.

Indeed if they're not rare the whole rationale might be invalid.

int getCAPValues (void)
{
//  Get three samples to check CAP signals are stable:  

        uint32_t x = (PORT->Group[IN_PORT_CAP].IN.reg & IN_PORT_CAP_MASK) >> IN_PORT_CAP_PIN;           // First set of CAP values
        for (uint32_t i = 0; i < 7; i++) dummy = i;                                                     // Pause
        uint32_t y = (PORT->Group[IN_PORT_CAP].IN.reg & IN_PORT_CAP_MASK) >> IN_PORT_CAP_PIN;           // second set
        if(x!=y){
            //x & y are different. Get a tie-breaker...
            for (uint32_t i = 0; i < 7; i++) dummy = i;                                                     // Pause
            uint32_t z = (PORT->Group[IN_PORT_CAP].IN.reg & IN_PORT_CAP_MASK) >> IN_PORT_CAP_PIN;           // third set
            if (y == z) {
                // use the y value
                x = y;
            } else if(x!=z){
                //tie-breaking failed...
                x=-1;
            }
        }
        return x;
}

PS: I also think you should use `usleep()' rather than dummy loops. It depends what is available on your platform.

Coalfield

maybe usleep() instead of sleep(). Besides you could directly return y when y==z (without assigning x). Same for -1. – Blesbok

@AndyHall I missed a last tiny tune-up avoiding the final if statement when y!=z. Enjoy. – Coalfield

Answer 2

If you can assume that x, y and z are either 0 or 1. Then you can just use x+y+z>1 (if most of them are 1 then the sum is larger than 1 and the comparison evaluates to 1 and otherwise it evaluates to 0).

Otherwise your (second) solution is probably the most efficient. At least if you don't know the probabilities of the outcomes. Besides by sleeping for one microsecond that would be the most time consuming operation.

What you should consider though is that as the reads are probably non-volatile it might differ if you actual do the third read or not. Also you should maybe consider if you need to do complete re-read. If you for example figure that if the first three reads from the ports are all different and the fourth read are equal to the second or third you could go for that you could use that to as an optimization. For example:

 x = read_port();
 y = read_port();

 if( x == y )
    return x;

 for(;;) {
    z = read_port();
    if( z == x || z == y )
        return z;
    x = y;
    y = z;
 }

Caduceus