"Warning, this statement is never executed", need help finding the reason for it.

[JoniS]

Hi, Since i could not find "general coding" subforum i will post here, even when i dont use TrueStudio. but visualGDB and arm gcc toolchain.

so the proplem is here, i'm trying to parse Steering wheel button Can codes from my cars canbus, to detect if it was long pressed or just tapped, to get some more functionality from the couple buttons i have. I have tested this prototype many months IRL with my daily car and all the other aspects are working great.

void __attribute__((optimize("O0"))) AdvancedMappedMessageParser(CanMessage *message, uint32_t time) {
		
	uint32_t timeStamp = time;
	uint64_t currentPayload;
	static uint32_t lastValidMsg = 0;
	static CanParserState state = Idle;
	static CanMessage lastMsg;
	static volatile uint32_t processingStarted = 0;
	
	/* Recover from possible deadlock */
	if (message == NULL && state != Idle && time - processingStarted > 1000) {
		state = Idle;
		lastValidMsg = 0;
		processingStarted = 0;
		return;
	} 
	if (message == NULL)
		return;
		
	
	currentPayload = message->getPayload();
	
	switch (state) {
		case Idle:
		if (IsPayloadMapped(currentPayload)) {
			/* save timestamp from the receive of the message */
			processingStarted = message->getTimeStamp();	
			/* Check if long press is mapped for the payload, so we know if further processing is needed */
			if (IsLongPressMapped(currentPayload))
			{			
				state = ProcessKeyPress;
				lastMsg = *message;	
			}
			else
			{
				/* No long press mapped for payload, execute now and move to wait for release or timeout */
				SendMediaEvent((getMappedButton(message, false))->action);	
				state = Release;
			}					
		}	
		break;
		
		case ProcessKeyPress:
		/* Check if we got same message, it indicates we might have long press */
		if (currentPayload == lastMsg.getPayload()) {
			if ((timeStamp - processingStarted) > HardwareConfig.ButtonSettings.LongPressDelay) {
				SendMediaEvent((getMappedButton(message, true))->action);
				/* Action cant repeat & long press, so we just go to release state now*/
				state = Release;
			}				
		}
		/* check if button was released, if it was send event and go to release state */
		else if(IsReleaseMessage(message)) {
				SendMediaEvent((getMappedButton(&lastMsg, false))->action);	<< this line of code			
				state = Release;
		}
		/* Check the payload against other button mappings */
		else if(IsPayloadMapped(currentPayload)) {		
			SendMediaEvent((getMappedButton(&lastMsg, false))->action);								
			lastMsg = *message;
			processingStarted = message->getTimeStamp();
			if (!IsLongPressMapped(currentPayload)) {
				SendMediaEvent((getMappedButton(&lastMsg, false))->action);	
				state = Release;
			}
		}	
		
		break;
		case Release:
		if (IsReleaseMessage(message) && timeStamp - processingStarted > HardwareConfig.ButtonSettings.ReleaseDelay) {
			state = Idle;
		}
			
		if (lastMsg.getPayload() == currentPayload) {
			if (getMappedButton(message, false)->AltAction == ButtonActionType::Repeat) {
				if (timeStamp - processingStarted > HardwareConfig.ButtonSettings.RepeatActionDelay) {
					SendMediaEvent((getMappedButton(message, false))->action);	
					processingStarted = timeStamp;
				}
					
			}		
		}
		else if(IsPayloadMapped(currentPayload)) {									
			lastMsg = *message;
			processingStarted = message->getTimeStamp();
			if (!IsLongPressMapped(currentPayload)) {
				SendMediaEvent((getMappedButton(&lastMsg, false))->action);	
				state = Release;
			}
		}	
				
		break;
						
		default:
		break;
	}	
	
	lastMsg = *message;
	lastValidMsg = timeStamp;
	return;
}

I have looked at the code for hours and at this point i'm too frustrated to find out what's causing that if else statement to be something that cant be reached, maybe someone else could spot the proplem with fresh eyes.

i have had countless iterations of the parser, but just cant get long presses and normal presses to work as expected, only long presses cool it works, only short presses and it works again. but for reason or another when i try get both at the same time, i either make some silly mistake or the compiler thinks it knows better and optimise's something out.

[Korporal]

You haven't told us which line is generating the compiler warning.

[JoniS]

/* check if button was released, if it was send event and go to release state */
		else if(IsReleaseMessage(message)) {
				SendMediaEvent((getMappedButton(&lastMsg, false))->action);	<< this line of code			
				state = Release;
		}

It's the first line inside that if else, so I believe for some reason compiler thinks the if else can never be true, or reached.​

[JoniS]

Currently I do have hardcoded "button map" which for sure contains the release code, and the function(IsReleaseMessage) is working as expected if I took it separate to test. ​

[Korporal]

	case ProcessKeyPress:
		/* Check if we got same message, it indicates we might have long press */
		if (currentPayload == lastMsg.getPayload()) 
		{
			if ((timeStamp - processingStarted) > HardwareConfig.ButtonSettings.LongPressDelay) 
			{
				mp = getMappedButton(message, true);
				SendMediaEvent(mp->action);
				/* Action cant repeat & long press, so we just go to release state now*/
				state = Release;
			}
		}

break out the nested function call like this, then lets see if it whines about line 7 or 8 (using above numbering).

[Tesla DeLorean]

How does getPayload() change in function for lastMsg vs currentPayload?

Same function, same data, compiler might assume the answer is the same.

[JoniS]

Mainloop feeds the parser from circular buffer which gets it's data from can rx interrupt.

​

believe it should warn about other if's and if else's statements aswell, if it did not figure that the data is coming from outside the mainloop.

[Korporal]

Sorry I copied the wrong part, but basically extract the nested function call getMappedButton and make it a separate function call as I did above, do that in all places for consistency and lets see how the compiler then reacts.

[Korporal]

Are any of those apparent function calls actually macros?

[Tesla DeLorean]

No, look to be function calls via pointers in the structure.

Not clear to me how geyPayload() determines the object it is being associated with, and I'm assuming this doesn't have C++ code.

So my point would be that the code before the switch, and in the case cause if to look like

x = y;

..

if (x == y)

{

} else if (not happening)

{

}

[JoniS]

typedef struct MappedButton_t {
	/* Lower bytes of Can message payload */
	uint32_t PayloadLowerNibble = 0;
	/* Upper bytes of Can message payload */
	uint32_t PayloadUpperNibble = 0;
	/* Can message Identifier */
	uint16_t Identifier = 0;
	/* User defined media action*/
	HID::MediaKeys action = HID::MediaKeys::None;
	/* Does this button support Long Press */
	ButtonActionType AltAction = ButtonActionType::Normal; 
		
} MappedButton_t;

uint64_t CanMessage::getPayload() return the databytes of the can frame(CanMessage is c++ class object), getMappedButton() function checks the assosiated Payload versus array of MappedButton_t's.

MappedButton_t* getMappedButton(CanMessage *msg, bool longpress) {
	
	/* Use to index the key mapping, if user did long press, but only "short" press is mapped */
	int8_t NoLongPressMapped = -1;
	
	for (int i = 0; i < 8; i++) {
		if (ButtonMap[i].PayloadUpperNibble == msg->getPayloadUpperNipple() &&
				ButtonMap[i].PayloadLowerNibble == msg->getPayloadLowNipple()) {
			/* Check if long press action enabled incase user did longpress */
			if (longpress && ButtonMap[i].AltAction == ButtonActionType::LongPress)
				return &ButtonMap[i];
			else 
				NoLongPressMapped = i;
		}
	}
		/* Check if we were looking for long press action, but no long press action is mapped for the Steering button */
		if (NoLongPressMapped != -1) 
			return &ButtonMap[NoLongPressMapped];	
	
		/* return null if no mapped button is found */
		/* Caller is responsible for null checking */
		return NULL;			
}

[Korporal]

Hmmm

[S.Ma]

Sometime the error is earlier than detected. You can anyway check the generated assembly and try to put breakpoint to make sure there is code in the clicked code path.

Still I feel unconfortable with this:

if (message == NULL && state != Idle && time - processingStarted > 1000) {

overwrapping won't hurt.

[JoniS]

 ((message == NULL) && (state != Idle) && ((time - processingStarted) > 1000))

Now it should quarantee the order of execution that is intended.

[Tesla DeLorean]

Perhaps you could use standard bisection methods to pin-point the test the compiler is hung up on?

[JoniS]

Okay to make it more interesting in my opinion.

else if(IsReleaseMessage(message)) {
		>>		SendMediaEvent((getMappedButton(&lastMsg, false))->action);	<<			
				state = Release;
		}

Compiler does optimise out only one line of code which is the one its crying for

SendMediaEvent((getMappedButton(&lastMsg, false))->action);				

Only that gets optimised away, the if else statement is there and working 100% correctly with test dataset, even the "state = Release" gets executed when debugging every line one after another.

When i calculate it, there are 5 identical function calls like that in the parser different states, only one gets optimised away with no obvious reason.

[JoniS]

One bug killed

/* Use to index the key mapping, if user did long press, but only "short" press is mapped */
	uint8_t NoLongPressMapped = 100;
	
	for (uint8_t i = 0; i < 8; i++) {
		if ((ButtonMap[i].PayloadUpperNibble == msg->getPayloadUpperNipple()) && (ButtonMap[i].PayloadLowerNibble == msg->getPayloadLowNipple())) {
			/* Check if long press action enabled incase user did longpress */
			if (longpress && ButtonMap[i].AltAction == ButtonActionType::LongPress)
				return &ButtonMap[i];
			else if(ButtonMap[i].AltAction != ButtonActionType::LongPress)
				NoLongPressMapped = i;
		}
	}
		/* Check if we were looking for long press action, but no long press action is mapped for the Steering button */
		if (NoLongPressMapped != 100) 
			return &ButtonMap[NoLongPressMapped];	
	
		/* return null if no mapped button is found */
		/* Caller is responsible for null checking */
		return NULL;

i did assign new value to "NoLongPressMapped" even when the actual mapping had alternative action of LongPress, since it was never checked -.-

if (longpress && ButtonMap[i].AltAction == ButtonActionType::LongPress)
				return &ButtonMap[i];
			else 
				NoLongPressMapped = i;
 
...
...
if (NoLongPressMapped != -1) 
			return &ButtonMap[NoLongPressMapped];

yes, hitting my head to desk right now. Sadly this did not fix the more urgent issue. But now i might actually to be able to detect between long and short presses =)

(Obviously this first implementation, always returned the Mapped key which was later on the array, not one based if we seek long press mapping or not)

[Korporal]

Are you able to prove that the function is executed? can you put a breakpoint in it and prove that the function called from that line (54) if you can then that pretty much wraps it up for the compiler.

[JoniS]

If I take optimisations off then it will be executed, thought for some reason debugger shows me only null data when stepping in there, and it does not actually send any reasonable data to the function call.(SendMediaEvent)

​

If I let the compiler ​optimize then I can see correct data with debugger which comes from my car canbus, but can't set break point on that line and it's never executed.

​

Sadly the arm assembler code is bit different than 8-bit avr, and i can't say that I completely understand ​it, but it seems some of the if/if else statements does get optimised out aswell, which would result in the behaviour im currently seeing with the parser.

​

Now with the getMappedButton function fixed, it does detect normal presses and long presses, but it likes to send every command multiple times forward, and even if it did detect long press, it will always execute command for short press before that​. I believe it's should work correctly now if the compiler was not so aggressively optimising stuff, since when it removes some of the if/else statements the parser goes to wrong states at wrong times.