xQueueSendFromISR: if fail question...

ulmus71 wrote on Wednesday, June 13, 2012:

I have question:
if ‘xQueueSendFromISR’ fails, it means it couldnt send variable into buffer/queue, but….
this function check first if it has room and return ‘fail’ if there is no room, or first it takes argument variable and then checks if there is room to put it into queue?

My ask is because as an argument i would like to put a register, but reading that register causes remove that value from hardware FIFO queue. And i don’t want to loose that value if xQueueSendFromISR fails, because i want that value in the next interrupt call to try again xQueueSendFromISR.

davedoors wrote on Wednesday, June 13, 2012:

1 Go to the definition of xQueueSendFromISR(), you will see it is a macro that calls xQueueGenericSendFromISR().

2 Go to the definition of xQueueGenericSendFromISR() and look at the simple source code.

…and you have answered your own question very quickly. A check

if( pxQueue->uxMessagesWaiting < pxQueue->uxLength )

is made before anything is done.

richard_damon wrote on Wednesday, June 13, 2012:

I would be careful about passing a hardware register as a “buffer” to a function in this manner.  The issue is that the function is designed to move arbitrary data from a memory buffer, and will use a memcpy call to transfer the data. Sometimes the way this is implemented it does a byte by byte copy of the buffer (since memcpy is designed for arbitrary blocks of memory, it might have some optimizations to do bigger ones if possible, but isn’t required). Registers of this type, due to the destructive read nature, often require that reads from them be in whole word units, thus you may get wrong data transfered.

It is better to add an explicit check for space in the queue, read the register into a local if space is available, then send it into the queue.

Another option would be to implement a 1 word buffer in your ISR, where if the Send Fails, you store the data in the buffer, but before getting new data you check if you have data in your buffer and use it instead.

ulmus71 wrote on Wednesday, June 13, 2012:

Thanx all of you, i am using registers :slight_smile: and check UART from 625 bauds to 625000 bauds and all is working correct, interrupt is tiny, :slight_smile: only pros, no cons:)

void __ISR(_UART_1_VECTOR, IPL1) vU1_ISR_Wraper(void);
void vU1_ISR_Handler(void)
{
	unsigned char	znak;
	static signed portBASE_TYPE xHigherPriorityTaskWoken; 
	xHigherPriorityTaskWoken = pdFALSE;
//	RECEIVER
	if((IFS0&BIT_27)==BIT_27)
	{
		if(xQueueSendFromISR( xUSBRxQueue, (void*)&U1RXREG, &xHigherPriorityTaskWoken ))
			IFS0CLR=BIT_27;
		else
		{/* queue full, what to do??? */
			znak=U1RXREG;//for any case, empty RCV buff
			IFS0CLR=BIT_27;//only loose character, but have no infinity loop
		}	
	}
//	TRANSMITTER
	if((IFS0&BIT_28)==BIT_28)
	{
		while(!(U1STA&BIT_9))
			if(xQueueReceiveFromISR(xUSBTxQueue, (void*)&U1TXREG, &xHigherPriorityTaskWoken)==pdFAIL)
				break;
		IFS0CLR=BIT_28;
	}
//	ERROR
	IFS0CLR=BIT_26;
	//switch context if necessary
	portEND_SWITCHING_ISR( xHigherPriorityTaskWoken );
}

richard_damon wrote on Wednesday, June 13, 2012:

You don’t specify what processor you are using, but from the names it looks like some form of a PIC32. Here are some of the issues with what you are doing.

&U1RXREG is the address of the whole register. Casting this to void*, and then accessing only 1 byte of the word (I am assuming the Queue are only storing 1 byte at a time) is architecture dependent. It normally works on “little endian” machines (where the lowest order byte of a word has the lowest address), which is what PICs are, but does not work on “big endian” machines, you have thus quietly imbedded an architectural dependance into you coding style.

Second, on some machines, the hardware specification manual specifically says that I/O registers should only be accessed with full word access instructions (Not knowing your exact processor, we can’t check this case) and your method of access here will NOT meet that restriction if it is there. Again, it may well work for your case, but you are developing a programming style that is hiding a architectural dependency.

As a more general issue, your code also now has duplicate sections doing the same things (getting the character from te buffer and clearing the interrupt). If you reversed the code and had the logic be: Data available interrupt happened, so get the data and clear the interrupt, then try to enque the data, and if that succeeds, we are done, else we need to think of what to do for error recovery; then you get cleaner code.

You also should give some more thoughts to your error recovery here. If you can be sure that this really shouldn’t happen, than maybe quietly dropping characters is ok, but if the serial port is sending data with any form of structure, it may be important for the receiving task to know that a data loss has occurred. Generally, at a minimum I set an overrun flag that can be checked, and sometimes put the serial port in a mode where I start to discard ALL characters received until the queue is emptied, and maybe even until a “record break” if the protocol makes that easy to find, in an attempt to minimize the number of records that are disturbed by the hang up. I sometimes will even then enqueue and “error character” into the queue so the receiving task knows the data has been messed up.

ulmus71 wrote on Wednesday, June 13, 2012:

thx richard_damon for your answer, but…

quietly imbedded an architectural dependance into you coding style

… writing interrupt, doing whatever you want with peripherial registers are putting architectural dependence into my/your code…

you think that:

int some_var;
some_var=U1RXREG;
if(xQueueSendFromISR( xUSBRxQueue, &some_var, &xHigherPriorityTaskWoken ))
.........

has less architectural depenendence, then:

if(xQueueSendFromISR( xUSBRxQueue, (void*)&U1RXREG, &xHigherPriorityTaskWoken ))

?
I think not, and do you know other uPC that has U1RXREG a bigendian style? i think not, too.

Yes, i know my code has duplicated places, cause i leaved that to write further coding in that place. I just paste into post first run of interrupt handle function, now it looks very very different from that above. I think the most exactly as you wrote above :smiley:

richard_damon wrote on Thursday, June 14, 2012:

Yes, your code wouldn’t have a chance to run on a different machine as is, but if you needed to port it to a machine that was big endian, and edited the obvious differences, you would likely be puzzled why you where always getting 0’s (of maybe FF’s) from the serial port. Thus not only is your code itself architectually dependent, but so is your coding STYLE.

Note that the code that you SHOULD write would be:

char some_var;
some_var = U1RXREG;
if(xQueueSendFromISR( xUSBRxQueue, &some_var, &HigherPriorityTaskWokne)) 

unless of course your Queue is defined to store items of size sizeof(int)

I hope you don’t use an int when receiving a character from the queue too! The type of the variable should ALWAY match the type whose sizeof was used to define the queue (or at the very least the sizeof the type needs to match), which is one reason the &U1RXREG is wrong, as I am fairly sure the type of that will be an unsigned int, while I suspect your queue was built for a sizeof = 1.

ulmus71 wrote on Thursday, June 14, 2012:

Yes Richard, you’re right, and i have declared var as unsigned char, just writing here i wrote ‘int’ (dont know why) :frowning:
I would like to do timy code and wondered if as an argument for send and receive from queue i can use special register… but quickly decided to use char variable, keeping in mind the same things you are writing about:)
Thanx anyway for a piece of advice, i preciate those veru much, thanx again!

rtel wrote on Thursday, June 14, 2012:

Just a couple of things to add here:

+ From memory, U1RXREG is in fact a 32 bit register, nine of which can hold data although in most cases the 9th bit will not be used.

+ Unless you have a low data rate, it is not always a good idea to write each received character to a queue because it is inefficient (although extremely convenient).  Using a DMA, circular buffer, etc. and a single semaphore to unblock a task is often preferred.  I know most of the demos use the queuing method, but most also state it is only done to deliberately load the system for test purposes.  Take a look at the transfer modes used by FreeRTOS+IO for more efficient examples.

http://www.freertos.org/FreeRTOS-Plus/FreeRTOS_Plus_IO/FreeRTOS_IO_Transfer_Modes.shtml

Regards.

ulmus71 wrote on Thursday, June 14, 2012:

I did perhabs something different from all: i am using semaphore when tx queue is full and wait till queue is empty, then i laod next data to the queue. Now i can work without any ‘hangs’ with slow bauds and at full rate with high bauds. Without this semaphore all was working great but if i had to send tens of kBs at full rate and receiver can handle slow receiving data then my transmitter was hanging around:) now with semaphore i have time gap to serve other tasks when queue is emptied.

ulmus71 wrote on Thursday, June 14, 2012:

in the end, i did DMA transfer with semaphore :slight_smile: no queues, no waits, no nothing :wink: best of all :slight_smile: