Bug in the Win32 Port

wtodd wrote on Tuesday, July 09, 2019:

Can you please give me a line number (from the file I posted).

Line 666

If I understand you correctly here - that you are describing something
it is not valid to do. Normally scheduled windows threads cannot access
FreeRTOS scheduler operations, and Windows threads that are running
FreeRTOS tasks cannot make Windows system calls - the two are just not
compatible logically.

We call vPortGenerateSimulatedInterrupt from Windows threads, which I guess is not the intended use case, but like I say later on in my post, this works well with our fixes.

Perhaps it would be less confusing about what is allowed or not if there were some standard way of interacting between Windows and FreeRTOS threads. We’ve been using the following function for that purpose, with good results:

typedef struct
{
	int32_t (*func)(void* arg);
	void* arg;
	int32_t result;

	StaticSemaphore_t wait_buf;
	SemaphoreHandle_t wait;
} sys_exec_state_t;

VOID CALLBACK sys_exec_func_cb(PTP_CALLBACK_INSTANCE instance, PVOID context)
{
	sys_exec_state_t* state = (sys_exec_state_t*)context;
	BaseType_t yield = pdFALSE;

	// call the function
	state->result = state->func(state->arg);

	// tell the task that we're done
	taskENTER_CRITICAL();
	xSemaphoreGiveFromISR(state->wait, &yield);

	if (yield == pdTRUE)
	{
		portYIELD();
	}
	taskEXIT_CRITICAL();
}

int32_t sys_exec_func(int32_t (*func)(void* arg), void* arg)
{
	int32_t result = 0;
	sys_exec_state_t state;

	state.func = func;
	state.arg = arg;
	state.result = 0;
	state.wait = xSemaphoreCreateBinaryStatic(&state.wait_buf);

	BOOL submitted = FALSE;

	taskENTER_CRITICAL();
	submitted = TrySubmitThreadpoolCallback(sys_exec_func_cb, &state, NULL);
	taskEXIT_CRITICAL();

	if (submitted == TRUE)
	{
		xSemaphoreTake(state.wait, portMAX_DELAY);
		result = state.result;
	}
	else
	{
		result = -EWOULDBLOCK;
	}

	vSemaphoreDelete(&state.wait);
	return result;
}

This allows you to synchronously execute a function outside of the FreeRTOS context from a FreeRTOS task. For example we can safely do something like this:

typedef struct
{
    const void* ptr;
    size_t size_of_elements;
    size_t number_of_elements;
	FILE* file;
} fwrite_args_t;

int32_t do_fwrite(void* arg)
{
    fwrite_args_t* args = (fwrite_args_t*)arg;
    return fwrite(args->ptr, args->size_of_elements, args->number_of_elements, args->file);
}
 
int32_t safe_fwrite(const void *ptr, size_t size_of_elements, size_t number_of_elements, FILE *file)
{
     fwrite_args_t args;
     args.ptr = ptr;
	 args.size_of_elements = size_of_elements;
	 args.number_of_elements = number_of_elements;
	 args.file = file;
	 
	 return sys_exec_func(do_fwrite, &args);
}

void freertos_task(void* arg)
{
    FILE* f;
    uint8_t buffer[10];
    
    // ...
    safe_fwrite(buffer, 10, 1, f);
    // ...
}

rtel wrote on Tuesday, July 09, 2019:

From your description there are a lot of good enhancements here, I just
need a little time to digest and test.

davidefer wrote on Wednesday, July 10, 2019:

From my side, the tests I’ve done with the Richard’s version were ok. I post here my modified version with the termination management and some other mods. I’ll follow the discussion and in case needed I can also do some tests.
Thank you very much for the support!

port.c (25.6 KB)

rtel wrote on Thursday, July 11, 2019:

I made a couple of edits to this post just to keep you up to date with progress. Only commented on the first two points so far. Search for [edit1] in the post.

https://sourceforge.net/p/freertos/discussion/382005/thread/d8a09bc895/#74d1/9a09

wtodd wrote on Thursday, July 11, 2019:

[edit1] This is basically the same case as a tick interrupt resulting in a context switch.

The yield interrupt is synchronous, while the tick interrupt is asynchronous. I agree that the timing is not critical for asynchronous interrupts, but it definitely is critical for synchrounous interrupts (like the yield interrupt). When the user calls vPortGenerateSimulatedInterrupt from a FreeRTOS task, it should behave in a synchronous manner, regardless of which interrupt it is.

For example, if I register my own simulated interrupt handler and then call vPortGenerateSimulatedInterrupt from a task, I would expect that task to yield to the interrupt handler before returning from vPortGenerateSimulatedInterrupt, and, if the interrupt handler requests a context switch, I would also expect the scheduler to switch to another task before the first returns from vPortGenerateSimulatedInterrupt. The fact that it doesn’t currently do that with the yield interrupt is the reason it crashes. My point is that it is possible to fix this for every interrupt, not just the yield interrupt.

wtodd wrote on Thursday, July 11, 2019:

In fact, the real issue is not really with vPortGenerateSimulatedInterrupt at all, but rather with vPortExitCritical. When interrupts are enabled after the final call to vPortExitCritical, it should process all pending interrupts before returning from vPortExitCritical. That way, if you do something like

 taskENTER_CRITICAL();
 vPortGenerateSimulatedInterrupt(portINTERRUPT_UART);
 // ...
 taskEXIT_CRITICAL();

The interrupts handlers will be processed before returning from taskEXIT_CRITICAL. Then, vPortGenerateSimulatedInterrupt can be as simple as this:

void vPortGenerateSimulatedInterrupt(uint32_t ulInterruptNumber)
{
	portENTER_CRITICAL();
	uxPendingInterrupts |= ( 1U << ulInterruptNumber );
	SetEvent( pvInterruptEvent );
	portEXIT_CRITICAL();
}

rtel wrote on Thursday, July 11, 2019:

I like that simplification.

jcforlc wrote on Wednesday, October 16, 2019:

Hi, I have used your win32 port to enable Unity unit testing. It works well except it did not allow me to restart the scheduler, neccessary for running multiple tests requiring a freertos environment.

The solution was simple, just add the reset terminate flag below after the thread cleanup code:

// #PORTCHANGE terminate all the created Windows threads
ulErrorCode = TerminateThread(pvSimulatedPeripheralTimerHandle, 0);
// now all the created tasks must be terminated
for (int i=0; i< createdThreadsNum; i++)
{
    ulErrorCode = TerminateThread(createdThreadsList[i].pvThread, 0);
}

** xTerminateRequest = pdFALSE; //reset terminate flag in case we restart scheduler
**

rtel wrote on Wednesday, October 16, 2019:

Grateful if you could attach your entire port.c file for the Win32 code
so I can see where you added this (task deletion or ending the
scheduler?) and how you are using xTerminateRequest. The latest
FreeRTOS versions have some changes to the Win32 port.

jcforlc wrote on Thursday, October 17, 2019:

my modification/addition is at line 358
file is based on DavidEfer last post of his modified port.c see: https://sourceforge.net/p/freertos/discussion/382005/thread/d8a09bc895/?page=1&limit=25#52b4

I assume this is the latest port.c that returns correctly after VTaskEndScheduler???
David’s port allows the Unity test framework to start/stop the FreeRTOS simulator and
display test results.

The purpose of modification is simply to allow calling vTaskStartScheduler ‘again’ after vTaskEndScheduler has been called. If xTerminateRequest is not reset then the scheduler wont start…

The reason I want to do this is because I use Unity unit test software (throwtheswitch.org) to unit test multiple tasks/objects one after the other in the same Unity test context.

The UnityTest.cpp file best describes how this is done.

NOTE1: I am not certain if vTaskEndscheduler is releasing all resources… I suspect its not
but hopefully will not be an issue for test purposes.

NOTE2: I have not tested this with the percepio trace recorder in the simulator demo.

regards,

port.c (25.7 KB) UnityTest.cpp (2.6 KB)