[Xenomai] FreeRTOS skin #2

[Xenomai] FreeRTOS skin #2

[Matthias Schneider]

Hi all,

after having rework extensively the FreeRTOS skin patch I feel ready 
for the second round of review comments. I hope no one will mind me 
starting a new thread for discussing it. I have tried to address all 
of the mentioned issues so far, plus many more.

Among them:

* reworked alignment macros
* #define portLONG long
* remapped error codes
* reworked scheduler start and stop completely as suggested
* reworked Scheduler.queued_task, .state and .num_tasks
* added new (internal) scheduler state taskSCHEDULER_SUSPENDING
* many other improvements and simplifications
* only take tasklist_lock for short time

Like suggested, I have gone for the simpler implementation of the task 
scheduler functionality and I added warnings and checks in case unsupported
use cases are triggered (e.g. holding the scheduler lock on mp systems, etc.)
This still needs to be reflected in the docu.

I have not yet addressed the issues of 

* vPortEnterCritical()/vPortExitCritical()
  since they are used extensively in the demo code (which is completely 
  hardware independent), but this can still be easily adapted.

* starting thread priority inheritance (need to start a new 
  discussion thread)

* there is still a workaround for the cobalt priorities in the patch...

Regards,
Matthias

p.s. Thank you a lot for your responsiveness addressing issues reported 
by me, including all the refactoring that is making my life easier!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Initial-Freertos-commit.-2.patch.gz
Type: application/x-gzip
Size: 34572 bytes
Desc: not available
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20140522/0136d1b5/attachment.bin>

Re: [Xenomai] FreeRTOS skin #2

[Matthias Schneider]

----- Original Message -----

> From: Matthias Schneider <ma30002000@yahoo.de>
> To: "xenomai@xenomai.org" <xenomai@xenomai.org>
> Cc: 
> Sent: Thursday, May 22, 2014 9:46 PM
> Subject: [Xenomai] FreeRTOS skin #2
> 
> Hi all,
> 
> after having rework extensively the FreeRTOS skin patch I feel ready 
> for the second round of review comments. I hope no one will mind me 
> starting a new thread for discussing it. I have tried to address all 
> of the mentioned issues so far, plus many more.
> 
> Among them:
> 
> * reworked alignment macros
> * #define portLONG long
> * remapped error codes
> * reworked scheduler start and stop completely as suggested
> * reworked Scheduler.queued_task, .state and .num_tasks
> * added new (internal) scheduler state taskSCHEDULER_SUSPENDING
> * many other improvements and simplifications
> * only take tasklist_lock for short time
> 
> Like suggested, I have gone for the simpler implementation of the task 
> scheduler functionality and I added warnings and checks in case unsupported
> use cases are triggered (e.g. holding the scheduler lock on mp systems, etc.)
> This still needs to be reflected in the docu.
> 
> I have not yet addressed the issues of 
> 
> * vPortEnterCritical()/vPortExitCritical()
>   since they are used extensively in the demo code (which is completely 
>   hardware independent), but this can still be easily adapted.
> 
> * starting thread priority inheritance (need to start a new 
>   discussion thread)
> 
> * there is still a workaround for the cobalt priorities in the patch...
> 
> Regards,
> Matthias
> 
> p.s. Thank you a lot for your responsiveness addressing issues reported 
> by me, including all the refactoring that is making my life easier!


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.

Regards,
Matthias
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Initial-Freertos-commit-no.-3.patch.gz
Type: application/x-gzip
Size: 34384 bytes
Desc: not available
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20140602/7a477986/attachment.bin>

Re: [Xenomai] FreeRTOS skin #2

[Philippe Gerum]

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.

Re: [Xenomai] FreeRTOS skin #2

[Matthias Schneider]

----- Original Message ----- 
> From: Philippe Gerum <rpm@xenomai.org> 
> To: Matthias Schneider <ma30002000@yahoo.de>; "xenomai@xenomai.org" <xenomai@xenomai.org> 
> Cc: 
> Sent: Friday, June 20, 2014 9:55 AM 
> Subject: Re: [Xenomai] FreeRTOS skin #2 
> 
> 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. 

Done. 

> 
> * 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. 
Done. 
  
> * __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. 
Done. 
Note that some freertos specific code remains to make duplicated task names 
unique.
  
> * vPortEnter/ExitCritical() 
> 
> - wouldn't these calls better emulated by threadobj_lock_sched() 
> assuming they stand for a global serialization mechanism? 
Unfortunately, this seems cause some test cases in the demo to fail. 
Since these tests are rather time consuming to investigate, I would like 
to leave this item open until all other issues have been addressed. We 
had also talked about other options concerning this subject as well 
(removing the function or leaving it empty), so I still need some 
time to think about it. 
> * Task registry support 
> 
> The code looks outdated, using fsobstacks is recommended. The registry 
> helpers in lib/alchemy/alarm.c illustrate a basic usage. 
Done. 

> 
> * 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. 
Ok, reading the affinity information once and caching it makes sense to 
avoid the switches to secondary mode. However, vTaskSuspendAll() / vTaskResumeAll() only works as expected when all tasks are pinned to a 
single cpu, storing this information on a by-task basis will not be sufficient. 
Now if there is one place to retrieve this information, which would it be? 
I can think of freertos_init() / freertos_scheduler_init() or 
vTaskStartScheduler(). However,  freertos_init() would probably not be 
able to allow setting process cpu affinity programatically at the 
beginning of main() if I am not mistaken. 
> Next time, I'll start reviewing other parts of the emulator. 
Great, thanks in advance for all the input.. 
> HTH, 
> 
> -- 
> Philippe. 
>

Re: [Xenomai] FreeRTOS skin #2

[Matthias Schneider]

----- Original Message -----

> From: Matthias Schneider <ma30002000@yahoo.de>
> To: Philippe Gerum <rpm@xenomai.org>; "xenomai@xenomai.org" <xenomai@xenomai.org>
> Cc: 
> Sent: Monday, June 23, 2014 12:31 PM
> Subject: Re: [Xenomai] FreeRTOS skin #2
> 
> ----- Original Message ----- 
>>  From: Philippe Gerum <rpm@xenomai.org> 
>>  To: Matthias Schneider <ma30002000@yahoo.de>; 
> "xenomai@xenomai.org" <xenomai@xenomai.org> 
>>  Cc: 
>>  Sent: Friday, June 20, 2014 9:55 AM 
>>  Subject: Re: [Xenomai] FreeRTOS skin #2 
>> 
>>  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. 
> 
> Done. 
> 
>> 
>>  * 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. 
> Done. 
>   
>>  * __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. 
> Done. 
> Note that some freertos specific code remains to make duplicated task names 
> unique.
>   
>>  * vPortEnter/ExitCritical() 
>> 
>>  - wouldn't these calls better emulated by threadobj_lock_sched() 
>>  assuming they stand for a global serialization mechanism? 
> Unfortunately, this seems cause some test cases in the demo to fail. 
> Since these tests are rather time consuming to investigate, I would like 
> to leave this item open until all other issues have been addressed. We 
> had also talked about other options concerning this subject as well 
> (removing the function or leaving it empty), so I still need some 
> time to think about it. 
>>  * Task registry support 
>> 
>>  The code looks outdated, using fsobstacks is recommended. The registry 
>>  helpers in lib/alchemy/alarm.c illustrate a basic usage. 
> Done. 
> 
>> 
>>  * 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. 
> Ok, reading the affinity information once and caching it makes sense to 
> avoid the switches to secondary mode. However, vTaskSuspendAll() / 
> vTaskResumeAll() only works as expected when all tasks are pinned to a 
> single cpu, storing this information on a by-task basis will not be sufficient. 
> Now if there is one place to retrieve this information, which would it be? 
> I can think of freertos_init() / freertos_scheduler_init() or 
> vTaskStartScheduler(). However,  freertos_init() would probably not be 
> able to allow setting process cpu affinity programatically at the 
> beginning of main() if I am not mistaken. 
>>  Next time, I'll start reviewing other parts of the emulator. 
> Great, thanks in advance for all the input.. 
> 
>>  HTH, 
>> 
>>  -- 
>>  Philippe. 
>> 


Please find enclosed a revised patch.

Matthias
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Initial-Freertos-commit-no.-4.patch.gz
Type: application/x-gzip
Size: 34354 bytes
Desc: not available
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20140623/2a044048/attachment.bin>

Re: [Xenomai] FreeRTOS skin #2

[Matthias Schneider]

----- Original Message -----

> From: Matthias Schneider <ma30002000@yahoo.de>
> To: Philippe Gerum <rpm@xenomai.org>; "xenomai@xenomai.org" <xenomai@xenomai.org>
> Cc: 
> Sent: Monday, June 23, 2014 10:27 PM
> Subject: Re: [Xenomai] FreeRTOS skin #2
> 
> ----- Original Message -----
> 
>>  From: Matthias Schneider <ma30002000@yahoo.de>
>>  To: Philippe Gerum <rpm@xenomai.org>; "xenomai@xenomai.org" 
> <xenomai@xenomai.org>
>>  Cc: 
>>  Sent: Monday, June 23, 2014 12:31 PM
>>  Subject: Re: [Xenomai] FreeRTOS skin #2
>> 
>>  ----- Original Message ----- 
>>>   From: Philippe Gerum <rpm@xenomai.org> 
>>>   To: Matthias Schneider <ma30002000@yahoo.de>; 
>>  "xenomai@xenomai.org" <xenomai@xenomai.org> 
>>>   Cc: 
>>>   Sent: Friday, June 20, 2014 9:55 AM 
>>>   Subject: Re: [Xenomai] FreeRTOS skin #2 
>>> 
>>>   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. 
>> 
>>  Done. 
>> 
>>> 
>>>   * 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. 
>>  Done. 
>>    
>>>   * __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. 
>>  Done. 
>>  Note that some freertos specific code remains to make duplicated task names 
> 
>>  unique.
>>    
>>>   * vPortEnter/ExitCritical() 
>>> 
>>>   - wouldn't these calls better emulated by threadobj_lock_sched() 
>>>   assuming they stand for a global serialization mechanism? 
>>  Unfortunately, this seems cause some test cases in the demo to fail. 
>>  Since these tests are rather time consuming to investigate, I would like 
>>  to leave this item open until all other issues have been addressed. We 
>>  had also talked about other options concerning this subject as well 
>>  (removing the function or leaving it empty), so I still need some 
>>  time to think about it. 
>>>   * Task registry support 
>>> 
>>>   The code looks outdated, using fsobstacks is recommended. The registry 
> 
>>>   helpers in lib/alchemy/alarm.c illustrate a basic usage. 
>>  Done. 
>> 
>>> 
>>>   * 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. 
>>  Ok, reading the affinity information once and caching it makes sense to 
>>  avoid the switches to secondary mode. However, vTaskSuspendAll() / 
>>  vTaskResumeAll() only works as expected when all tasks are pinned to a 
>>  single cpu, storing this information on a by-task basis will not be 
> sufficient. 
>>  Now if there is one place to retrieve this information, which would it be? 
>>  I can think of freertos_init() / freertos_scheduler_init() or 
>>  vTaskStartScheduler(). However,  freertos_init() would probably not be 
>>  able to allow setting process cpu affinity programatically at the 
>>  beginning of main() if I am not mistaken. 
>>>   Next time, I'll start reviewing other parts of the emulator. 
>>  Great, thanks in advance for all the input.. 
>> 
>>>   HTH, 
>>> 
>>>   -- 
>>>   Philippe. 
>>> 
> 
> 
> Please find enclosed a revised patch.
> 
> Matthias


Please find enclosed another calling the check_processors() only when starting
the scheduler and remembering its output. It is also adapted to the latest changes.
Any comments will be appreciated.

Matthias
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Initial-Freertos-commit-no.-5.patch.gz
Type: application/gzip
Size: 34306 bytes
Desc: not available
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20140701/e7483dac/attachment.bin>

Re: [Xenomai] FreeRTOS skin #2

[Philippe Gerum]

On 07/01/2014 06:48 PM, Matthias Schneider wrote:
> Please find enclosed another calling the check_processors() only when starting
> the scheduler and remembering its output. It is also adapted to the latest changes.
> Any comments will be appreciated.
>

- clusters allowing key dups are available using the *_dup() call form
- vTaskEndScheduler() can be simplified, and should not panic if it 
cannot lock a vanishing thread pulled from the task list.

commit 9239281e4badb56a2449723e0204c597ed3743b1
Author: Philippe Gerum <rpm@xenomai.org>
Date:   Wed Jul 9 09:26:55 2014 +0200

     freertos: use cluster with duplicate key support

diff --git a/lib/freertos/task.c b/lib/freertos/task.c
index f7d99a5..d201711 100644
--- a/lib/freertos/task.c
+++ b/lib/freertos/task.c
@@ -407,7 +407,6 @@ static signed portBASE_TYPE 
__xTaskGenericCreate(struct freertos_task *task,
  	struct corethread_attributes cta;
  	struct threadobj_init_data idata;
  	int ret, cprio;
-	size_t i = 0, id_pos;

  	cprio = convert_freertos_task_priority(uxPriority);

@@ -427,24 +426,7 @@ static signed portBASE_TYPE 
__xTaskGenericCreate(struct freertos_task *task,

  	generate_name(task->name, (const char *)name, &task_namegen);

-	/* since cluster entries must have unique names, we try to append
-	   names that seem to exist already with an id. */
-	for (i = 0; i < TASK_DUPLICATED_NAME_MAX_ID; ++i) {
-		ret = cluster_addobj(&freertos_task_table, task->name, &task->cobj);
-		if (ret != -EEXIST)
-			break;
-		if (i == 0) {
-			id_pos = strlen(task->name);
-			id_pos = (id_pos > TASK_DUPLICATED_NAME_MAX_ID_MAX_POS)
-				? TASK_DUPLICATED_NAME_MAX_ID_MAX_POS
-				: id_pos;	
-		}
-		sprintf(task->name + id_pos, "%1d", i);
-	}
-	if (ret == -EEXIST) {
-		warning("duplicate task name: %s (used > %d times)",
-			task->name, TASK_DUPLICATED_NAME_MAX_ID);
-	}
+	ret = cluster_addobj_dup(&freertos_task_table, task->name, &task->cobj);
  	if (__bt(ret)) {
  		threadobj_uninit(&task->thobj);
  		return pdFALSE;

commit c867515edfd63ce7abc0b76095e0613ceb8f7f22
Author: Philippe Gerum <rpm@xenomai.org>
Date:   Wed Jul 9 09:21:03 2014 +0200

     freertos: fix vTaskEndScheduler()

diff --git a/lib/freertos/task.c b/lib/freertos/task.c
index 0af799a..f7d99a5 100644
--- a/lib/freertos/task.c
+++ b/lib/freertos/task.c
@@ -445,8 +445,7 @@ static signed portBASE_TYPE 
__xTaskGenericCreate(struct freertos_task *task,
  		warning("duplicate task name: %s (used > %d times)",
  			task->name, TASK_DUPLICATED_NAME_MAX_ID);
  	}
-	if (ret) {
-		__bt(ret);
+	if (__bt(ret)) {
  		threadobj_uninit(&task->thobj);
  		return pdFALSE;
  	}
@@ -781,35 +780,33 @@ void vTaskStartScheduler(void) {
  	CANCEL_RESTORE(svc);
  }

-void vTaskEndScheduler(void) {
+void vTaskEndScheduler(void)
+{
  	struct service svc;
-	struct freertos_task *task, *tmp, *current;
+	struct freertos_task *task, *current;

  	CANCEL_DEFER(svc);
-	threadobj_lock_sched();

+	current = freertos_task_current();
  	__RT(pthread_mutex_lock(&freertos_scheduler.tasklist_lock));
  	freertos_scheduler.state = taskSCHEDULER_SUSPENDING;
-	__RT(pthread_mutex_unlock(&freertos_scheduler.tasklist_lock));

-	current = freertos_task_current();
-	if (!pvlist_empty(&freertos_task_list)) {
-		pvlist_for_each_entry_safe(task, tmp, &freertos_task_list, next) {
-			if (task == current)
-				continue;
-
-			if (get_freertos_task(task))
-				threadobj_cancel(&task->thobj);
-			else
-				panic("could not lock task %s", task->name);
-        	}
+	while (!pvlist_empty(&freertos_task_list)) {
+		task = pvlist_pop_entry(&freertos_task_list,
+					struct freertos_task, next);
+		if (task == current)
+			continue;
+		__RT(pthread_mutex_unlock(&freertos_scheduler.tasklist_lock));
+		if (get_freertos_task(task))
+			threadobj_cancel(&task->thobj);
+		__RT(pthread_mutex_lock(&freertos_scheduler.tasklist_lock));
  	}

-	threadobj_unlock_sched();
-	CANCEL_RESTORE(svc);
-
+	__RT(pthread_mutex_unlock(&freertos_scheduler.tasklist_lock));
  	threadobj_lock(&current->thobj);
  	threadobj_cancel(&current->thobj);
+
+	CANCEL_RESTORE(svc);
  }

  portBASE_TYPE xTaskGetSchedulerState(void)

-- 
Philippe.

Re: [Xenomai] FreeRTOS skin #2

[Philippe Gerum]

On 07/09/2014 09:42 AM, Philippe Gerum wrote:
> On 07/01/2014 06:48 PM, Matthias Schneider wrote:
>> Please find enclosed another calling the check_processors() only when 
>> starting
>> the scheduler and remembering its output. It is also adapted to the 
>> latest changes.
>> Any comments will be appreciated.
>>
> 
> - clusters allowing key dups are available using the *_dup() call form

I just noticed that you would need uniquement labels for pushing your tasks to the registry anyway, so the following patch on top of the previous one would likely do:

diff --git a/lib/freertos/task.c b/lib/freertos/task.c
index d201711..d005163 100644
--- a/lib/freertos/task.c
+++ b/lib/freertos/task.c
@@ -426,10 +426,17 @@ static signed portBASE_TYPE __xTaskGenericCreate(struct freertos_task *task,
 
 	generate_name(task->name, (const char *)name, &task_namegen);
 
-	ret = cluster_addobj_dup(&freertos_task_table, task->name, &task->cobj);
-	if (__bt(ret)) {
-		threadobj_uninit(&task->thobj);
-		return pdFALSE;
+	for (;;) {
+		ret = cluster_addobj(&freertos_task_table, task->name, &task->cobj);
+		if (ret == -EEXIST) {
+			generate_name(task->name, NULL, &task_namegen);
+			continue;
+		}
+		if (__bt(ret)) {
+			threadobj_uninit(&task->thobj);
+			return pdFALSE;
+		}
+		break;
 	}
 
 	registry_init_file_obstack(&task->fsobj, &registry_ops);

-- 
Philippe.

Re: [Xenomai] FreeRTOS skin #2

[Matthias Schneider]

----- Original Message -----

> From: Philippe Gerum <rpm@xenomai.org>
> To: Matthias Schneider <ma30002000@yahoo.de>; "xenomai@xenomai.org" <xenomai@xenomai.org>
> Cc: 
> Sent: Wednesday, July 9, 2014 10:24 AM
> Subject: Re: [Xenomai] FreeRTOS skin #2
> 
> On 07/09/2014 09:42 AM, Philippe Gerum wrote:
>>  On 07/01/2014 06:48 PM, Matthias Schneider wrote:
>>>  Please find enclosed another calling the check_processors() only when 
>>>  starting
>>>  the scheduler and remembering its output. It is also adapted to the 
>>>  latest changes.
>>>  Any comments will be appreciated.
>>> 
>> 
>>  - clusters allowing key dups are available using the *_dup() call form
> 
> I just noticed that you would need uniquement labels for pushing your tasks to 
> the registry anyway, so the following patch on top of the previous one would 
> likely do:
> 
> diff --git a/lib/freertos/task.c b/lib/freertos/task.c
> index d201711..d005163 100644
> --- a/lib/freertos/task.c
> +++ b/lib/freertos/task.c
> @@ -426,10 +426,17 @@ static signed portBASE_TYPE __xTaskGenericCreate(struct 
> freertos_task *task,
> 
>     generate_name(task->name, (const char *)name, &task_namegen);
> 
> -    ret = cluster_addobj_dup(&freertos_task_table, task->name, 
> &task->cobj);
> -    if (__bt(ret)) {
> -        threadobj_uninit(&task->thobj);
> -        return pdFALSE;
> +    for (;;) {
> +        ret = cluster_addobj(&freertos_task_table, task->name, 
> &task->cobj);
> +        if (ret == -EEXIST) {
> +            generate_name(task->name, NULL, &task_namegen);
> +            continue;
> +        }
> +        if (__bt(ret)) {
> +            threadobj_uninit(&task->thobj);
> +            return pdFALSE;
> +        }
> +        break;
>     }
> 
>     registry_init_file_obstack(&task->fsobj, &registry_ops);
> 
> 
> -- 
> Philippe.
> 

This changes the effective names of the duplicated tasks. However,
I do not see a big deal about it, so I have added this change to my patch.

Matthias

Re: [Xenomai] FreeRTOS skin #2

[Matthias Schneider]

----- Original Message -----
> From: Philippe Gerum <rpm@xenomai.org>
> To: Matthias Schneider <ma30002000@yahoo.de>; "xenomai@xenomai.org" <xenomai@xenomai.org>
> Cc: 
> Sent: Wednesday, July 9, 2014 9:42 AM
> Subject: Re: [Xenomai] FreeRTOS skin #2
> 
> On 07/01/2014 06:48 PM, Matthias Schneider wrote:
>>  Please find enclosed another calling the check_processors() only when 
> starting
>>  the scheduler and remembering its output. It is also adapted to the latest 
> changes.
>>  Any comments will be appreciated.
>> 
> 
> - clusters allowing key dups are available using the *_dup() call form
> - vTaskEndScheduler() can be simplified, and should not panic if it 
> cannot lock a vanishing thread pulled from the task list.
> 
> -- 

> Philippe.
>

The vTaskEndScheduler() patch unfortunately now leads to a frozen
vTaskStartScheduler() function when shutting down in some tests.

This is probably due to freertos_scheduler.tasklist_end_cond not being
triggered when the list gets empty in vTaskEndScheduler() ? also, 
freertos_scheduler.num_tasks seems not to be decremented when removing
elemets from the list via pvlist_pop_entry().

Even when calling pthread_cond_signal() at the end of vTaskEndScheduler(),
I still get the freeze. Any idea?

Re: [Xenomai] FreeRTOS skin #2

[Philippe Gerum]

On 07/14/2014 09:18 PM, Matthias Schneider wrote:
> 
> 
> 
> 
> ----- Original Message -----
>> From: Philippe Gerum <rpm@xenomai.org>
>> To: Matthias Schneider <ma30002000@yahoo.de>; "xenomai@xenomai.org" <xenomai@xenomai.org>
>> Cc: 
>> Sent: Wednesday, July 9, 2014 9:42 AM
>> Subject: Re: [Xenomai] FreeRTOS skin #2
>>
>> On 07/01/2014 06:48 PM, Matthias Schneider wrote:
>>>  Please find enclosed another calling the check_processors() only when 
>> starting
>>>  the scheduler and remembering its output. It is also adapted to the latest 
>> changes.
>>>  Any comments will be appreciated.
>>>
>>
>> - clusters allowing key dups are available using the *_dup() call form
>> - vTaskEndScheduler() can be simplified, and should not panic if it 
>> cannot lock a vanishing thread pulled from the task list.
>>  
>> -- 
> 
>> Philippe.
>>
> 
> The vTaskEndScheduler() patch unfortunately now leads to a frozen
> vTaskStartScheduler() function when shutting down in some tests.
> 
> This is probably due to freertos_scheduler.tasklist_end_cond not being
> triggered when the list gets empty in vTaskEndScheduler() ? also, 

That condvar is shared between the task finalizer and
vTaskStartScheduler() instead. If no task terminates after
vTaskEndScheduler() is called, then we will be missing the trigger
indeed. The logic needs some fixing. I also just noticed that the
start_cond variable seems redundant with the purpose of
threadobj_wait_start().

> freertos_scheduler.num_tasks seems not to be decremented when removing
> elemets from the list via pvlist_pop_entry().
> 

> Even when calling pthread_cond_signal() at the end of vTaskEndScheduler(),
> I still get the freeze. Any idea?
> 
> 

-- 
Philippe.

Re: [Xenomai] FreeRTOS skin #2

[Philippe Gerum]

On 07/01/2014 06:48 PM, Matthias Schneider wrote:

> Please find enclosed another calling the check_processors() only when starting
> the scheduler and remembering its output. It is also adapted to the latest changes.
> Any comments will be appreciated.
>

I don't think checking the registry contents in the testsuite is worth 
it: the output format from the registry may change over time, leaving 
tests parsing this output unfixed and outdated (e.g. we would not 
receive any compilation error in case of mismatch). Besides, the tester 
may want to disable the registry support at build time.

-- 
Philippe.