Review request...

Reviewing code is not a task you can accomplish in 10 or 20 minutes. And the code being a scheduler certainly does not make it any easier. Since I had nothing better to do this weekend, I had a look. And it has taken hours. I don't think you will get any other responses.

It is good for you to have written the code. Even if it won't gain a vast popularity, at least you have certainly learned something useful. And those ambitious young programmers who intend to write the best RTOS ever have a chance to look how someone else has done it.

Then the remarks..

Function rtos_disable_scheduler() does not prevent an interrupt from causing scheduling.

A task can handle one interrupt source only (at a time). It is not hard for me to imagine a situation where it would be preferred to handle several interrupt sources by one task. In my opinion it is waste of resources to have a separate task for everything. More generally the OS lacks the functionality to wait for a message (from another task) or an event (from one or more ISR). Actually I have found this to be a problem with other OS:es as well (including RTX166). Even when sending a message from an ISR is supported, there is no guarantee that it is not lost, and therefore cannot be used. The workaround is to have a dedicated task for reveiving events from ISR:s and translate them to messages for the tasks which handle those events. This costs one additional task, an extra task switch and some processing. The reason I miss a 'wait for anything addressed to me' functionality is that I like to write my tasks like this (simplified):

void task( void)
{
   init();

   while( 1)
   {
      wait_for_something_to_happen();

      process_whatever_happened();
   }
}

You have the following comment in a switch statement:

      switch (lp_timer->type)
         // WAR STORY
         // it is always good practice to place the most common cases at the beginning
         // of the switch block, due to performnce reasons (pipeline prefetch related issues...)
      {
         case eOneShotNonRelease:

What makes you think that putting the most common case first in the switch statement would increase performance? Actually in this case it is selected last of all other alternatives.

Your example code:

 l_result = rtos_flag_timer_inform_in(l_flag_timer, SECONDS(1), &s_missed_ticks) ;
 if (l_result == l_flag_timer) // ok, wait for 1 second
 {
   while (l_missed_ticks == 0)
   {
      rtos_timer_suspend(l_flag_timer) ;
      l_missed_ticks = s_missed_ticks ;
      rtos_timer_resume(l_flag_timer) ;
   }

Bad example. The timer is bound to miss a significant amount of ticks while suspended. Using function interrupt_safe_assign_ulong() should work better.

Why do you need 3 hardware timers? You could do with one. Using 100us timer resolution leads to a significant CPU load if you have many running timers. If context switch time alone is 100us, you cannot quarantee any timing to that resolution, especially when all interrupts may be disabled for a relatively long time.

You automatically allocate two timers for a task. Why not use just one? Task state already tells what it is waiting for.

You should at least offer the option of saving MAC related registers as part of the task context if the OS is supposed to run on a XC16x. On the other hand you could easily support C16x by saving and restoring the MULIP flag and DPP0.

Your task_info structure members sp and r0 change size with the used memory model. If Keil decided to change the order in which the segment and the offset of a 32 bit pointer are stored, your code would stop working. I admit this is an unlikely scenario. However, the user stacks must always reside in the NDATA memory space, which is now not guaranteed. At least you should explain how to force them there.

The source modules have a lot of lines longer than 100 characters - one even 194. Not everyone has a widescreen display. I don't, and it makes reading the source code tedious.

The ROM footprint is too big, and the same goes for the RAM footprint. At least you should provide an easy way of leaving out those memory consuming diagnostic messages and trace. Even then it is not small.

Sauli

Thanks a lot. I will try to take your comments into account in the next release.

Greetings,
Tamir

Sauli,
Thanks again for the review. You wrote:

Bad example. The timer is bound to miss a significant amount of ticks while suspended. Using function interrupt_safe_assign_ulong() should work better.

The call to 'rtos_timer_suspend' does not affect the hardware at all as the timer will continue to tick normally but not servied in the timer ISR. On the other hand, using 'interrupt_safe_assign_ulong' as you suggested would make matters even worse as it physically disabled not only the interrupt line of the timer, but all interrupts. The proposed usage is actually the cheapest one, unless you see a better one, of course.

Kind regards,
Tamir Michael

The call to 'rtos_timer_suspend' does not affect the hardware at all as the timer will continue to tick normally but not servied in the timer ISR.

In that case, the function has been given the wrong name. If it doesn't actually suspend the timer, but rather only IRQ generation, then it should be called "rtos_timer_tick_disable" or something.

Both functionalities are worth having, but for different purposes. One is to actually "stop time", so it maintain the same sequence of timer events relative to each other, but not their cycle time.

The other means that time goes on, so timer ticks will be synchronous to their previous rhythm afer re-enabling the ticks. But sequence of events may be completely deranged.

   while (l_missed_ticks == 0)
   {
      rtos_timer_suspend(l_flag_timer) ;
      l_missed_ticks = s_missed_ticks ;
      rtos_timer_resume(l_flag_timer) ;
   }

When this loop is running, the timer is suspended about half of the time. Then, there is a 50% chance that another task starts running while the timer is suspensed. If the task does any amount of work, it is going to run hundreds of us. The timer will miss all those 100us ticks. And it gets worse if you have several active tasks.

I also notice that once an eFlag type timer increments the missed deadline counter, the timer is freed. Then there is nothing stopping another task from allocating the timer for some other purpose. Polling the deadline counter still works, but it does not tell how many ticks ago the timer expired.

I suggested interrupt_safe_assign_ulong because then you don't have to suspend the timer, just:

  interrupt_safe_assign_ulong( &l_missed_ticks, &s_missed_ticks);

You already disable interrupts in many places for a lot longer times, so this should not make any difference. I personally would make these kind on interrupt safe assignments with an ATOMIC sequence - a lot faster and safer. By safer I mean that disabling interrupts globally with the IEN bit of PSW may have undesired side effects due to pipeline effects. Clearing the IEN bit does not take effect immediately, but an interrupt may still be acknowledged after that. If the acknowledged interrupt is low priority and time consuming, and you have a very time critical interrupt, that time critical interrupt is not serviced until the low priority interrupt is serviced and your 'interrupt safe' code is executed.

Sauli

Sauli,
Thanks for your insight. Well, you are right, but the core of the "problem" is that this sort of timer is not really suitable for a preemptive environment. The other kind of timers supported (utilized by 'rtos_callback_timer_inform_every' and 'rtos_callback_timer_inform_in') are much more accurate, as they can invoke a user provided callback onces expired. If the callback is lightweight, it is both a more accurate and useful solution. In my little spare time I am working of release 2.2 (based on your review and some bug fixes) which will be the last one for the C166. I am going to make a port for the ARM9, and hopefully I will be able to reuse a major parts of my previous work.

Dear Friends,

I have headache for UDP protocol,
I want to open a port for connection, but ".Bind" command gives "Runtime Error 87"
I did not found any info about this issue and i dont' think it can happen, if anybody does please give me hand.

Thank you,

of all threads in this forum, do you have to spam mine? I can help you but will not - get lost along with your UDP headache !

Sauli,
One more thing. repeatedly stopping/starting a hardware timer will create an accumulated error, so I chose not to do so. Of couse, this does not reduce from the validity of the arguments you made.

Kind regards,
Tamir Michael