avr-gcc, pass struct by reference, optimization differences

Go To Last Post
14 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I am using the AVR Toolchain in Atmel Studio 6.2.  I have a function that has a struct pointer as an argument.  I typically use the -Os flag for optimization, but in this case, it seems to be "breaking" my code.  When I use fprintf to verify values of the struct inside the function, everything seems fine.  However, outside the function, not only are they not updated, the act of printing them causes the MCU to reset.  Note, I have tried implementing this both as described above, and as a function with no arguments, but a static struct inside which gets returned to the calling function.

 

Code (static version):

 

typedef struct {
    uint8_t packet_type;	//General Call or Addressed
    uint8_t msg_valid_flag;
    uint8_t msg_type;
    uint8_t msg_len;
    uint8_t msg_body[255];
} twiPacket_t;

//void checkTWIdata(twiPacket_t *packet) {
twiPacket_t checkTWIdata(void) {
	uint8_t i, numRxBytes;
	uint8_t buf[MAX_TWI_PAYLOAD];
	static twiRxState_t parseState = SYNC_1;
	static uint8_t payloadByteCount = 0;
	static uint8_t index = 0;

	static twiPacket_t packet;
	
	//Clean up from last time
	if(packet.msg_valid_flag == 1) {
		//Clear out everything (assume handled)
		clearPacketData(&packet);
		index = 0;
	}
	
	// Check if the TWI Transceiver has completed an operation.
	if(!TWI_Transceiver_Busy())	{
		// Check if the last operation was successful
		if(TWI_statusReg.lastTransOK) {
			// Check if the last operation was a reception
			if (TWI_statusReg.RxDataInBuf) {
				//Get data
				numRxBytes = TWI_Get_Data_From_Transceiver(&buf
, (MAX_TWI_PAYLOAD-index)); // Check if the last operation was a reception as General Call /* if (TWI_statusReg.genAddressCall) { packet.packet_type = GENERAL_CALL; } */ //Parse data for(i = index; i < (index+numRxBytes); i++) { switch(parseState) { case SYNC_1 : if(buf[i] == TWI_SYNC_1) { parseState = SYNC_2; if(DEBUG_EN) { fprintf_P(&com0_str, PSTR("Entering state SYNC_2\n")); } } break; case SYNC_2 : if(buf[i] == TWI_SYNC_2) { parseState = MSG_TYPE; if(DEBUG_EN) { fprintf_P(&com0_str, PSTR("Entering state MSG_TYPE\n")); } } else { if(DEBUG_EN) { fprintf_P(&com0_str, PSTR("WARNING - checkTwiData(): corrupted packet (SYNC_2)\n")); } parseState = SYNC_1; } break; case MSG_TYPE : packet.msg_type = buf[i]; parseState = MSG_LEN; if(DEBUG_EN) { fprintf_P(&com0_str, PSTR("Entering state MSG_LEN\n")); } break; case MSG_LEN : packet.msg_len = buf[i]; payloadByteCount = 0; //Reset counter parseState = PAYLOAD; if(DEBUG_EN) { fprintf_P(&com0_str, PSTR("Entering state PAYLOAD\n")); } break; case PAYLOAD : packet.msg_body[payloadByteCount] = buf[i]; payloadByteCount++; if(payloadByteCount >= packet.msg_len) { parseState = FRAME_TERM; if(DEBUG_EN) { fprintf_P(&com0_str, PSTR("Entering state FRAME_TERM\n")); } } else { parseState = PAYLOAD; } break; case FRAME_TERM : if(buf[i] == TWI_FRM_TERM) { //Successful packet packet.msg_valid_flag = 1; } else { //Clear out packet (for now... change this later?) clearPacketData(&packet); parseState = SYNC_1; if(DEBUG_EN) { fprintf_P(&com0_str, PSTR("WARNING - checkTwiData(): corrupted packet (FRAME_TERM)\n")); } } break; default : if(DEBUG_EN) { fprintf_P(&com0_str, PSTR("ERROR - checkTwiData() - default case in switch -- this should never happen...\n")); } break; } } index = index + numRxBytes; } else { // Ends up here if the last operation was a transmission if(DEBUG_EN) { fprintf_P(&com0_str, PSTR("Finished I2C transmission\n")); } } // Check if the TWI Transceiver has already been started. // If not then restart it to prepare it for new receptions. if(!TWI_Transceiver_Busy()) { TWI_Start_Transceiver(); } } else { // Ends up here if the last operation completed unsuccessfully TWI_Act_On_Failure_In_Last_Transmission( TWI_Get_State_Info() ); } } fprintf(&com0_str, "BOOYAH2 (%d, %d)\n", packet.msg_len, packet.msg_valid_flag); return(packet); }

 

Calling function (twi_event is set elsewhere in an ISR):

 

int main(void) {
	uint8_t ret;
	twiPacket_t packet = {0};

	setup();		//Peripheral setup, program initialization

    while(1) {
        wdt_reset();	//Kick the watchdog

		if(MR_getRxFlag()) {
			MR_handleMsg();
		}
		
		ret = checkExtRxData();
		if(ret) {
			decodeCommand();
		}
		
	
		if(twiEvent) {
fprintf(&com0_str, "BEFORE (%d, %d)\n", packet.msg_len, packet.msg_valid_flag);			
			//checkTWIdata(&packet);
			packet = checkTWIdata();

fprintf(&com0_str, "BOOYAH3 (%d, %d)\n", packet.msg_len, packet.msg_valid_flag);
			if(packet.msg_valid_flag == 1) {
				handleTWI();
			}
			twiEvent = 0;
		}
    }
}

 

With the -OS flag set, in both cases (the posted code, and the "pointer variant" of the code), the fprintf with BOOYAH2 is correct (it shows the valid flag being set, and the length properly).  The fprintf with BOOYAH3 shows both values being zero, followed by the MCU being reset immediately (with 0x00 in MCUSR).

 

Note, the act of initializing the struct in the main loop to {0} also has a large impact on performance of the "pointer variant" -- when the initialization is NOT there, the function does not seem to work at all -- i.e., assignments to the struct members have no effect -- immediate printout after assignment shows zeros.

 

Changing my compile options to no optimization results in the code operating as expected (struct is "readable" at main level).  It also works for -01.

 

Edit: It does NOT work for -O2 or -O3

 

Edit 2: Even with -O1, it only seems to work when I have the debug print statements in.  Removing the print statements results in unexpected behavior inside the function.  Is the struct somehow getting optimized away entirely?  Does it need to be volatile?

 

Am I doing something wrong in the code, or is this just a quirk of the optimizer?

Science is not consensus. Science is numbers.

Last Edited: Wed. Aug 12, 2015 - 03:45 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Can you strip it down to a more readable code? Right now it looks like you are passing 259 bytes by value -- lots of copying going on, so maybe it is broken somehow.

NOTE: I no longer actively read this forum. Please ask your question on www.eevblog.com/forum if you want my answer.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The array size is arbitrarily large.  It breaks for small transfers (5 bytes).  I used sizeof() to make sure the struct was getting properly allocated (as you say: 259)

Science is not consensus. Science is numbers.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

I'd still like to see the most minimal code that reproduces the problem. I'm sure all the I2C stuff is not relevant.

NOTE: I no longer actively read this forum. Please ask your question on www.eevblog.com/forum if you want my answer.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
twiPacket_t checkTWIdata(void) {
	:
	static twiPacket_t packet;

Did you really overshadow your variable?

int main(void) {
	uint8_t ret;
	twiPacket_t packet = {0};
   :
fprintf(&com0_str, "BEFORE (%d, %d)\n", packet.msg_len, packet.msg_valid_flag);			
			//checkTWIdata(&packet);
			packet = checkTWIdata();

And there too?  How was packet supposed to get filled in, if it's local to main()?

A smaller example would be nice, but could we at least have exactly the failing code, rather than some mish-mash of the broken code and the workaround?

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

westfw wrote:
How was packet supposed to get filled in, if it's local to main()?
The one in main() gets returned by value from a function. So there are two identical copies at any given time. The code is fine from that point of view.

NOTE: I no longer actively read this forum. Please ask your question on www.eevblog.com/forum if you want my answer.

Last Edited: Wed. Aug 12, 2015 - 06:30 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Why does the thread title say "by reference" when the code shows "by value"? I see some commented code there that would pass by ref instead, but it is commented. For 259 bytes I have no idea why you would want to have the compiler shuffling about 259 blocks of memory when you could just be passing a 16 bit pointer?!?

 

Which model of AVR is this anyway - does it really have room in its RAM for multiple blocks of 259 bytes anyway?

 

Oh and it's probably computer science sacrilege to suggest this but if "packet" were global you could access it everywhere and not even bother passing a pointer or use dereferencing code!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

@Cliff:  Sorry -- I posted the "static" version rather than the pointer version.  In desperation, I tried both.  The code for the pointer version is identical, except operators are -> rather than .  Both exhibited the same behavior.

 

It is on an ATMEGA256RFR2.  The project is using < 10% of the available memory.

 

I stripped the code down last night -- essentially put a hard coded value in the function for the array -- rather than extracting it from a buffer.  The behavior was similar, but slightly different.  It "worked as expected" the first iteration, but not any others.  If desired, I can post it here -- but because the behavior is slightly different, it may only cloud the issue.

 

This seems to have all the hallmarks of a stack trashing issue, which gets masked by a fprintf (see: https://www.avrfreaks.net/forum/unexpected-behavior-w-strtoul? ).  That one was resolved by completely overhauling the code, and going with different functionality.  I never did figure out why the fprintf statement masked the problem.

 

 

Pointer version:

void checkTWIdata(twiPacket_t *packet) {
	uint8_t i, numRxBytes;
	uint8_t buf[MAX_TWI_PAYLOAD];
	static twiRxState_t parseState = SYNC_1;
	static uint8_t payloadByteCount = 0;
	static uint8_t index = 0;
	
	//Clean up from last time
	if(packet->msg_valid_flag == 1) {
		//Clear out everything (assume handled)
		clearPacketData(packet);
		index = 0;
	}
	
	// Check if the TWI Transceiver has completed an operation.
	if(!TWI_Transceiver_Busy())	{
		// Check if the last operation was successful
		if(TWI_statusReg.lastTransOK) {
			// Check if the last operation was a reception
			if (TWI_statusReg.RxDataInBuf) {
				//Get data
				numRxBytes = TWI_Get_Data_From_Transceiver(&buf
, (MAX_TWI_PAYLOAD-index)); // Check if the last operation was a reception as General Call /* if (TWI_statusReg.genAddressCall) { packet->packet_type = GENERAL_CALL; } */ //Parse data for(i = index; i < (index+numRxBytes); i++) { switch(parseState) { case SYNC_1 : if(buf[i] == TWI_SYNC_1) { parseState = SYNC_2; if(DEBUG_EN) { fprintf_P(&com0_str, PSTR("Entering state SYNC_2\n")); } } break; case SYNC_2 : if(buf[i] == TWI_SYNC_2) { parseState = MSG_TYPE; if(DEBUG_EN) { fprintf_P(&com0_str, PSTR("Entering state MSG_TYPE\n")); } } else { if(DEBUG_EN) { fprintf_P(&com0_str, PSTR("WARNING - checkTwiData(): corrupted packet (SYNC_2)\n")); } parseState = SYNC_1; } break; case MSG_TYPE : packet->msg_type = buf[i]; parseState = MSG_LEN; if(DEBUG_EN) { fprintf_P(&com0_str, PSTR("Entering state MSG_LEN\n")); } break; case MSG_LEN : packet->msg_len = buf[i]; payloadByteCount = 0; //Reset counter parseState = PAYLOAD; if(DEBUG_EN) { fprintf_P(&com0_str, PSTR("Entering state PAYLOAD\n")); } break; case PAYLOAD : packet->msg_body[payloadByteCount] = buf[i]; payloadByteCount++; if(payloadByteCount >= packet->msg_len) { parseState = FRAME_TERM; if(DEBUG_EN) { fprintf_P(&com0_str, PSTR("Entering state FRAME_TERM\n")); } } else { parseState = PAYLOAD; } break; case FRAME_TERM : if(buf[i] == TWI_FRM_TERM) { //Successful packet packet->msg_valid_flag = 1; parseState = SYNC_1; } else { //Clear out packet (for now... change this later?) parseState = SYNC_1; if(DEBUG_EN) { fprintf_P(&com0_str, PSTR("WARNING - checkTwiData(): corrupted packet (FRAME_TERM)\n")); fprintf(&com0_str, "\tLength: %d\n\tValue (not term): 0x%02X\n", packet->msg_len, buf[i]); } clearPacketData(packet); } break; default : if(DEBUG_EN) { fprintf_P(&com0_str, PSTR("ERROR - checkTwiData() - default case in switch -- this should never happen...\n")); } break; } } index = index + numRxBytes; } else { // Ends up here if the last operation was a transmission if(DEBUG_EN) { fprintf_P(&com0_str, PSTR("Finished I2C transmission\n")); } } // Check if the TWI Transceiver has already been started. // If not then restart it to prepare it for new receptions. if(!TWI_Transceiver_Busy()) { TWI_Start_Transceiver(); } } else { // Ends up here if the last operation completed unsuccessfully TWI_Act_On_Failure_In_Last_Transmission( TWI_Get_State_Info() ); } } }

 

I don't have hardware at this moment, but will try the global solution tonight.

 

Thanks for the feedback.  I won't be able to reply again for another 12 hours or so.

 

Science is not consensus. Science is numbers.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Your code fails to build.

Perhaps when you get back to this you could post a stripped back version to demonstrate the error that we can build ourselves. It would also be highly convenient if it ran on a PC.

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

It fails to build due it being pulled from a much larger program.  I will try to obtain a simpler version of the code that replicates the problem.

 

Question: there is a ring buffer that is used both in the TWI_vect ISR and the function TWI_Get_Data_From_Transceiver.  It is not volatile (if I try to make it volatile I get a slew of warnings regarding the volatile qualifier getting discarded, etc. (and it still doesn't work).

 

Code:

 

uint8_t TWI_Get_Data_From_Transceiver( unsigned char *msg, unsigned char msgSize )
{
  uint8_t i;
  uint8_t numRead = 0;
  uint8_t truncated = 0;

  while ( TWI_Transceiver_Busy() ) {}             // Wait until TWI is ready for next transmission.

  if( TWI_statusReg.lastTransOK )               // Last transmission completed successfully.              
  {                   
	numRead = ringBufferGetCount(&rxBufTwi);

	if(numRead > msgSize) {
		if(DEBUG_EN) {
			fprintf(&com0_str, "WARNING - TWI_Get_Data_From_Transceiver(): array too small! (numRead: %d, bufSize: %d)\n", numRead, msgSize);
		}
		numRead = msgSize;
		truncated = 1;
	}

    for (i = 0; i < numRead; i++) {                 // Copy data from Transceiver buffer.
		msg[i] = ringBufferRemove(&rxBufTwi);
    }

	if(truncated == 0) {
		TWI_statusReg.RxDataInBuf = FALSE;          // Slave Receive data has been read from buffer.
	}
  }
  return(numRead);                                   
}

 

ISR (slightly modified from AVR Note, and ported to avrgcc):

ISR(TWI_vect)
{
  static unsigned char TWI_bufPtr;
//  twiEvent = 1;
  
  switch (TWSR)
  {
    case TWI_STX_ADR_ACK:            // Own SLA+R has been received; ACK has been returned
//    case TWI_STX_ADR_ACK_M_ARB_LOST: // Arbitration lost in SLA+R/W as Master; own SLA+R has been received; ACK has been returned
      TWI_bufPtr   = 0;                                 // Set buffer pointer to first data location
    case TWI_STX_DATA_ACK:           // Data byte in TWDR has been transmitted; ACK has been received
      TWDR = TWI_buf[TWI_bufPtr++];
      TWCR = (1<<TWEN)|                                 // TWI Interface enabled
             (1<<TWIE)|(1<<TWINT)|                      // Enable TWI Interupt and clear the flag to send byte
             (1<<TWEA)|(0<<TWSTA)|(0<<TWSTO)|           // 
             (0<<TWWC);                                 //
      TWI_busy = 1;
      break;
    case TWI_STX_DATA_NACK:          // Data byte in TWDR has been transmitted; NACK has been received. 
                                     // I.e. this could be the end of the transmission.
      if (TWI_bufPtr == TWI_msgSize) // Have we transceived all expected data?
      {
        TWI_statusReg.lastTransOK = TRUE;               // Set status bits to completed successfully. 
      } 
      else                          // Master has sent a NACK before all data where sent.
      {
        TWI_state = TWSR;                               // Store TWI State as errormessage.      
      }        
                                                        
      TWCR = (1<<TWEN)|                                 // Enable TWI-interface and release TWI pins
             (1<<TWIE)|(1<<TWINT)|                      // Keep interrupt enabled and clear the flag
             (1<<TWEA)|(0<<TWSTA)|(0<<TWSTO)|           // Answer on next address match
             (0<<TWWC);                                 //
      
      TWI_busy = 0;   // Transmit is finished, we are not busy anymore
      break;     
    case TWI_SRX_GEN_ACK:            // General call address has been received; ACK has been returned
//    case TWI_SRX_GEN_ACK_M_ARB_LOST: // Arbitration lost in SLA+R/W as Master; General call address has been received; ACK has been returned
      TWI_statusReg.genAddressCall = TRUE;
    case TWI_SRX_ADR_ACK:            // Own SLA+W has been received ACK has been returned
//    case TWI_SRX_ADR_ACK_M_ARB_LOST: // Arbitration lost in SLA+R/W as Master; own SLA+W has been received; ACK has been returned    
                                                        // Dont need to clear TWI_S_statusRegister.generalAddressCall due to that it is the default state.
      TWI_statusReg.RxDataInBuf = TRUE;      
      TWI_bufPtr   = 0;                                 // Set buffer pointer to first data location
      
                                                        // Reset the TWI Interupt to wait for a new event.
      TWCR = (1<<TWEN)|                                 // TWI Interface enabled
             (1<<TWIE)|(1<<TWINT)|                      // Enable TWI Interupt and clear the flag to send byte
             (1<<TWEA)|(0<<TWSTA)|(0<<TWSTO)|           // Expect ACK on this transmission
             (0<<TWWC);  
      TWI_busy = 1;
      
      break;
    case TWI_SRX_ADR_DATA_ACK:       // Previously addressed with own SLA+W; data has been received; ACK has been returned
    case TWI_SRX_GEN_DATA_ACK:       // Previously addressed with general call; data has been received; ACK has been returned
	  ringBufferInsert(&rxBufTwi, TWDR);
   	  twiEvent = 1;

      TWI_statusReg.lastTransOK = TRUE;                 // Set flag transmission successfull.       
                                                        // Reset the TWI Interupt to wait for a new event.
      TWCR = (1<<TWEN)|                                 // TWI Interface enabled
             (1<<TWIE)|(1<<TWINT)|                      // Enable TWI Interupt and clear the flag to send byte
             (1<<TWEA)|(0<<TWSTA)|(0<<TWSTO)|           // Send ACK after next reception
             (0<<TWWC);                                 // 
      TWI_busy = 1;
      break;
    case TWI_SRX_STOP_RESTART:       // A STOP condition or repeated START condition has been received while still addressed as Slave    
                                                        // Enter not addressed mode and listen to address match
      TWCR = (1<<TWEN)|                                 // Enable TWI-interface and release TWI pins
             (1<<TWIE)|(1<<TWINT)|                      // Enable interrupt and clear the flag
             (1<<TWEA)|(0<<TWSTA)|(0<<TWSTO)|           // Wait for new address match
             (0<<TWWC);                                 //
      
      TWI_busy = 0;  // We are waiting for a new address match, so we are not busy
      
      break;           
    case TWI_SRX_ADR_DATA_NACK:      // Previously addressed with own SLA+W; data has been received; NOT ACK has been returned
    case TWI_SRX_GEN_DATA_NACK:      // Previously addressed with general call; data has been received; NOT ACK has been returned
    case TWI_STX_DATA_ACK_LAST_BYTE: // Last data byte in TWDR has been transmitted (TWEA = ?0?); ACK has been received
//    case TWI_NO_STATE              // No relevant state information available; TWINT = ?0?
    case TWI_BUS_ERROR:         // Bus error due to an illegal START or STOP condition
      TWI_state = TWSR;                 //Store TWI State as errormessage, operation also clears noErrors bit
      TWCR =   (1<<TWSTO)|(1<<TWINT);   //Recover from TWI_BUS_ERROR, this will release the SDA and SCL pins thus enabling other devices to use the bus
      break;
    default:     
      TWI_state = TWSR;                                 // Store TWI State as errormessage, operation also clears the Success bit.      
      TWCR = (1<<TWEN)|                                 // Enable TWI-interface and release TWI pins
             (1<<TWIE)|(1<<TWINT)|                      // Keep interrupt enabled and clear the flag
             (1<<TWEA)|(0<<TWSTA)|(0<<TWSTO)|           // Acknowledge on any new requests.
             (0<<TWWC);                                 //
      
      TWI_busy = 0; // Unknown status, so we wait for a new address match that might be something we can handle
  }
}

 

Science is not consensus. Science is numbers.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Well, you need to make it volatile and resolve the warnings. That's kind of a point of those warnings. Compiler explicitly tells you that it will assume that variable is no longer volatile and it will optimize all uses of it.

 

Although with buffers it usually is not a problem, compilers are not that smart to optimize them.

NOTE: I no longer actively read this forum. Please ask your question on www.eevblog.com/forum if you want my answer.

Last Edited: Thu. Aug 13, 2015 - 05:22 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
      TWDR = TWI_buf[TWI_bufPtr++];

There's no check there for overflow?

 

BTW you don't appear to have shown ringBufferInsert(), ringBufferGetCount() and ringBufferRemove() so its tricky to know what needs to be volatile anyway. The rule about volatile is that it's only required on shared variables. I imagine in the ring buffer there are both read and write pointers. One being used by the "insert" that is called from the ISR() and one being used by the "remove" that is in the main code. The idea of volatile is to say "hey main code, keep an eye on this, for all you it might change at any moment if an interrupt occurs" but if the read/write pointers are separate it may not be required. If there's a count for the buffer than I could well believe that may be shared - being incremented by the "insert" and decremented by the "remove". If so it would need to be volatile so the main() code "sees" any change (it probably needs atomic protection too).

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

@Cliff: that code I haven't gotten to yet -- just working on receive yet.  That's pretty much straight from the Atmel code.

 

I have grafted on Dean's ring buffer code to the receive portion of the bus -- so your guess is correct.  There are both read and write pointers.  The count variable is protected by atomic blocks, but it is not volatile.

Science is not consensus. Science is numbers.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Found the problem.  Cliff's post got me thinking...

For longer messages, the parser started parsing before the entire message came in (by design).  However, the problem was when the TWI interrupt fired while I was emptying the ring buffer.

 

Fixed with:

 

//Get data
ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
  numRxBytes = TWI_Get_Data_From_Transceiver(&buf
, (MAX_TWI_PAYLOAD-index)); }

 

Not an ideal solution, but one that is working for now.

 

Thanks all for help/suggestions.

 

Edit: can't figure out why formatting is off in code block

Science is not consensus. Science is numbers.

Last Edited: Fri. Aug 14, 2015 - 03:03 AM