Array of Structs overwrite problem

Bruce Coward

I have been chasing this bug for three days now and wondered if anybody out there could point me in the right direction. I have a circular array generated from a structure. We are writing data to this array and found that the second write was overwriting part of the first record. To simplify debugging we have put some test code that changes what is to be written and then does a second write. Here is the code:

class CAN
{
    // Members
    public struct CANTXSTRUCT
    {
        public int    PGN;
        public byte   DLC;
        public byte\[\] TxData;
    }
    
    private const int CANArraySize = 20;
    static private int CANFront = 1;
    static private int CANRear = 0;

    // Create static private circular array of CAN Tx messages
    static private CANTXSTRUCT\[\] arCANTx = new CANTXSTRUCT\[CANArraySize + 1\];

    // Methods
    //\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*
    // static public void QueueCANTxMsg()
    // This method is called to place a CAN Tx message on the rear of the
    // arCANTx circular array.
    //\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*
    static public void QueueCANTxMsg(int PGN, byte DLC, byte\[\]TxData)
    {
        // Just warn and return if arCANTx message array is full
        if ((CANRear + 2) % CANArraySize == CANFront)
        {
            MessageBox.Show("CAN Tx Array Overflow, Press OK to continue",
                                    "Error Warning", MessageBoxButtons.OK);
            return;
        }
        // Modulus Increment rear pointer
        CANRear = (CANRear + 1) % CANArraySize;
        
        // Store data in array
        CAN.arCANTx\[CANRear\].PGN = PGN;
        CAN.arCANTx\[CANRear\].DLC = DLC;
        CAN.arCANTx\[CANRear\].TxData = TxData;

        // Test Code
        TxData\[0\] = 0xFF;
        CANRear = (CANRear + 1) % CANArraySize;
        // Store test data in array
        CAN.arCANTx\[CANRear\].PGN = PGN;
        CAN.arCANTx\[CANRear\].DLC = DLC;
        CAN.arCANTx\[CANRear\].TxData = TxData;  // <---- Failing line
        
    } // End of QueueCANTxMsg()

When the failing line is executed it overwrites the first written CAN.arCANTx[CANRear].TxData value. The CANRear value appears to beworking correctly and I think the problem is associated with the

Luc 648011

Hi Bruce, having fun with a CAN bus? been there using C, not C#. Here are some remarks: 0. Not sure why you have one extra element in the CANTXSTRUCT array. 1. I am not sure why you want to store a message twice. Seems very odd. 2. your QueueCANTxMsg() method is queueing two messages, however they share all the data, which is fine for value types (PGN, DLC) but not for reference types: the TxData you are storing twice is the same array twice, so changing a byte in it (TxData[0] = 0xFF; ) will reflect in both copies, since you only are storing a reference. 3. your overflow test is not correct, if (CANRear + 1) % CANArraySize == CANFront you are also overflowing the circular buffer (adding two can make you look OVER the front node). 4. if you plan on using QueueCANTxMsg() from more than one thread (say a regular thread and some asynchronous datareceived handler) then you should lock otherwise the pointer adjustment (CANRear = (CANRear + 1) % CANArraySize; ) could be interrupted resulting in using the same location twice. :)

Bruce Coward

Hi Luc, The circular array handling in point 0 and 3 is an old standard technique I have used for years that runs great but requires an unused buffer address. The second message in the code sample was just test code so I could see the problem without leaving the function. Point 4 - Only ever calling from 1 thread but your suggestion is well taken. I am sure you are on the right lines with point 2 but how do I change this to store real individual values in a real circular array? Thanks for your input. After 3 days it sure is appreciated. Cheers, Bruce :thumbsup:

Luc 648011

Hi Bruce,

Bruce Coward wrote:

an unused buffer address

I agree a simple circular buffer wastes one slot, since you must discriminate the empty case (getptr=putptr) and the full case (getptr=putptr), which is easiest by never filling the buffer completely, so there will always be an empty slot, however it moves around: the pointer arithmetic should use the actual array size, whatever extra elements get allocated are simply wasted. So if you do array[DIM+1} then you should also do PTR=(PTR+1)%(DIM+1) otherwise PTR will never point to ARRAY[DIM].

Bruce Coward wrote:

how do I change this to store real individual values

You must make sure the data either resides in a different array each time (so the caller must provide a new array each time), or you must copy the data into an array you allocate yourself. I prefer the latter, and I would try and allocate all the buffers just once (when intializing the circular buffer) and copy the data in them. Depending on how you actually obtain the data from CAN, there is or isn't a way of saving one copy operation: - split the queueMessage method in two parts; - use first part to obtain the next array (the array preallocated for the next slot in the circular buffer) but don't advance the put pointer - call "driver" to fill that buffer - call remaining part of queueMessage to stuff value types, and advance slot pointer which makes it readable for the CAN message consumer. :)

Gideon Engelberth

The line you pointed out does not copy the array. Array assignments never perform an implicit copy, even in structures. As a side note, if you could do sizeof(CANTXSTRUCT), it would be the same regardless of the size of TxData because the structure only holds a reference to the array not the actual bytes. The problem is that you are assigning CAN.arCANTx[CANRear].TxData variable to hold a reference to the parameter TxData. If you check the value after the first line of "Test Code", you should be seeing overwrite because doing (parameter)TxData[4] = 0x87 is equivalent to CAN.arCANTx[CANRear].TxData[4] = 0x87 since both arrays reference the same bytes. If you want to copy the data, you will need to create a new array to assign to the structure and call Array.Copy yourself. Also, I would suggest using the Queue class instead of making your own circular array.

Luc 648011

Gideon Engelberth wrote:

I would suggest using the Queue class instead of making your own circular array.

I beg to differ. There may be very good reasons to have a pre-allocated data structure to handle all traffic, since it avoids a lot of object creation and destruction, and it provides an upper bound to the amount of data that can be stored. In fact I did some similar CAN drivers before, on embedded systems which did not have really dynamic memory, but even if they had I probably would have gone with the circular buffer anyway. BTW: when designed carefully one can forego all locking, which is quite useful when part of the code runs in an interrupt service routine. :)

Alan N

Hi Bruce, I find a circular buffer much easier to understand and manage with a control variable storing the current data amount. With this additional piece of information in place the buffer full/empty state is obvious and the empty slot can be discarded. Not tested but something like this:

class CAN {
// Members
public struct CANTXSTRUCT {
public int PGN;
public byte DLC;
public byte[] TxData;
}

private const int CANArraySize = 20;
// MODIFIED initial value
static private int CANFront = 0;
static private int CANRear = 0;
// NEW control variable
static private int currentCount = 0;

// Create static private circular array of CAN Tx messages
// MODIFIED
static private CANTXSTRUCT[] arCANTx = new CANTXSTRUCT[CANArraySize];

// Methods
//*********************************************************************
// static public void QueueCANTxMsg()
// This method is called to place a CAN Tx message on the rear of the
// arCANTx circular array.
//*********************************************************************
static public void QueueCANTxMsg(int PGN, byte DLC, byte[] TxData) {
// Just warn and return if arCANTx message array is full

// MODIFIED
if (currentCount < arCANTx.Length) {
  // Modulus Increment rear pointer
  CANRear = (CANRear + 1) % arCANTx.Length;

  // Store data in array
  // ...

  // NEW update contents counter
  currentCount++;
} else {
  // you screwed up!
}

}

static public void RemoveMsg() {
if (currentCount > 0) {
// Remove data here
// ...

  // Modulus increment front pointer
  CANFront = (CANFront + 1) % arCANTx.Length;
  // update current contents counter
  currentCount--;
} else {
  // you screwed up!
}

}
}

Alan.

Luc 648011

Hi Alan, while I agree on two pointers plus amount being easier to understand, the nice thing about a circular buffer with only two pointers (and an empty slot) is that you don't need to lock if there is only one producer and one consumer: each pointer will have one writer and two readers, so when writing is atomic all is fine (when implemented carefully that is). Adding a currentCount makes the state redundant and prohibits atomic operations. Remember, in a typical application, the consumer would be the app itself, the producer of incoming messages would be the CAN driver, which operates from an interrupt service routine (an asynchronous handler in .NET speak). :)