4
\$\begingroup\$

I have a program that works with a hardware via an ActiveX API. The API have events that is triggered by the hardware. In the hardware there is an IO card. There is 16 in and 16 out. Changes in input signal triggers the event handler below. The event uses an int to show which signal is set/unset. This by using binary addresses for each signal

Signal          Mask
INPUT_0         0x01;
INPUT_1         0x02;
INPUT_2         0x04;
INPUT_3         0x08;
... 
INPUT_11        0x800

(there are some signals that are used for something else and can't be used as Input)

The signals can be set/unset in any order, but the event will trigger every time a change is done! The value, p_nBits, will include all signals state. This means that if INPUT_0 is set and then INPUT_1 is set then p_nBits will be 3, i.e. both signals! But INPUT_0 has already been handled and should not be handled again until it is unset. I have an integer currentBits that is the current state of the input signals.

Any error in logic? What can be improved? Is it possible to make it faster?

// Current state of Input signals
int currentBits = 0;

public void IoPort_sigInputChange(int p_nBits)
{
    lock (lockObj)
    {

        if ((p_nBits & INPUT_0) == INPUT_0)
        {
            if ((currentBits & INPUT_0) != INPUT_0)
            {
                currentBits |= INPUT_0;
                RaiseRunModeEvent(true);
            }
        }
        else
        {
            if ((currentBits & INPUT_0) == INPUT_0)
            {
                currentBits &= ~INPUT_0;
                RaiseRunModeEvent(false);
            }
        }

        if ((p_nBits & INPUT_1) == INPUT_1)
        {
            if ((currentBits & INPUT_1) != INPUT_1)
            {
                currentBits |= INPUT_1;
                RaiseAllPositionsSetEvent(true);
            }
        }
        else
        {
            if ((currentBits & INPUT_1) == INPUT_1)
            {
                currentBits &= ~INPUT_1;
                RaiseAllPositionsSetEvent(false);
            }
        }

        if ((p_nBits & INPUT_2) == INPUT_2)
        {
            if ((currentBits & INPUT_2) != INPUT_2)
            {
                currentBits |= INPUT_2;
                RaiseOrderEndedEvent(true);
            }
        }
        else
        {
            if ((currentBits & INPUT_2) == INPUT_2)
            {
                currentBits &= ~INPUT_2;
                RaiseOrderEndedEvent(false);
            }
        }

        // more checks may occur
    }
}
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Firstly, you can simplify the outer else statements:

...
    else
    {
      if ((currentBits & INPUT_0) == INPUT_0)
      {
        ...
      }
    }
...

to

...
else if ((currentBits & INPUT_0) == INPUT_0)
{
} ...

Secondly, if the check of all masks follows the same pattern as showed, you can create a mask checker method like this one (in C# 7.0):

private void HandleMask(int bitPattern, (int mask, Action<bool> action) maskItem)
{
  if ((bitPattern & maskItem.mask) == maskItem.mask)
  {
    if ((currentBits & maskItem.mask) != maskItem.mask)
    {
      currentBits |= maskItem.mask;
      maskItem.action(true);
    }
  }
  else if ((currentBits & maskItem.mask) == maskItem.mask)
  {
    currentBits &= ~maskItem.mask;
    maskItem.action(false);
  }
}

public void IoPort_sigInputChange(int p_nBits)
{
  lock (lockObj)
  {
    var maskItems = new(int mask, Action<bool> action)[]
    {
      (mask: INPUT_0, action: RaiseRunModeEvent),
      (mask: INPUT_1, action: RaiseAllPositionsSetEvent),
      (mask: INPUT_2, action: RaiseOrderEndedEvent),
    };

    foreach (var maskItem in maskItems)
    {
      HandleMask(p_nBits, maskItem);
    }
  }
}

Update

In .net 3.5 you should/could do it in a way like this:

private void HandleMask(int bitPattern, int mask, Action<bool> action)
{
  if ((bitPattern & mask) == mask)
  {
    if ((currentBits & mask) != mask)
    {
      currentBits |= mask;
      action(true);
    }
  }
  else if ((currentBits & mask) == mask)
  {
    currentBits &= ~mask;
    action(false);
  }
}


public void IoPort_sigInputChange(int p_nBits)
{
  lock (lockObj)
  {
    var maskItems = new[]
    {
      new { mask = INPUT_0, action = (Action<bool>)RaiseRunModeEvent },
      new { mask = INPUT_1, action = (Action<bool>)RaiseAllPositionsSetEvent },
      new { mask = INPUT_2, action = (Action<bool>)RaiseOrderEndedEvent },
    };

    foreach (var maskItem in maskItems)
    {
      HandleMask(p_nBits, maskItem.mask, maskItem.action);
    }
  }
}
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the answer (sorry about the delay in responding). The use of Action is very clever and I will use it later. Unfortunately at the moment my program must run with .NET 3.5. Do you have a version that works with this old .NET? \$\endgroup\$
    – Andis59
    Commented Apr 4, 2018 at 11:39

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.