Re: [Xenomai] FreeRTOS skin #2

[Philippe Gerum <rpm@xenomai.org>]

On 06/02/2014 08:46 PM, Matthias Schneider wrote:

> Please find enclosed a new version of the FreeRTOS skin. It adapts to the
> latest changes in forge/next and also removes the previously included
> workarounds - thanks to the changes done based on the first review round
> they were no longer necessary.
>

Thanks. Here we go:

* vTaskStartScheduler()

Dropping the task_list lock then yielding is superfluous, the caller 
will wait on the condvar for the scheduler state to switch to SUSPENDING 
anyway, implicitly releasing this lock before going to sleep.

* vTaskDelay()

Cobalt wraps sched_yield(), so you likely want to call 
__RT(sched_yield()) to pick the proper implementation depending on the 
underlying real-time core.

* __xTaskGenericCreate()

- It looks like the mutex attribute initialized locally is a left over 
from a previous implementation.

- Drawing unique identifier labels should be done using a name 
generator, e.g.

static DEFINE_NAME_GENERATOR(task_namegen, struct freertos_task, name);
...
	generate_name(task->name, name, &task_namegen);

The open-coded method also used by other emulators is racy.

* vPortEnter/ExitCritical()

- wouldn't these calls better emulated by threadobj_lock_sched() 
assuming they stand for a global serialization mechanism?

* Task registry support

The code looks outdated, using fsobstacks is recommended. The registry 
helpers in lib/alchemy/alarm.c illustrate a basic usage.

* vTaskSuspendAll()

Running check_processors() there may switch the caller to linux mode
at each invocation, due to calling sysconf/sched_getaffinity indirectly. 
I would rather assume that the CPU affinity is not allowed to change 
once started, and determine whether a task is bound to more than a 
single CPU in the trampoline code. Then vTaskSuspendAll() may do a 
simple flag-based test for checking the consistency, still allowing 
different freertos tasks to be pinned on distinct CPUs though. At any 
rate, this would be an acceptable restriction, since freertos does not 
seem to be SMP-capable in its reference implementation anyway.

Next time, I'll start reviewing other parts of the emulator.

HTH,

-- 
Philippe.