Review request...

Hello all and thank you for reading,
I am about to ask you to do something rather unusual, it is more like a favor actually. I am the owner of an open source kernel available for free download at sourceforge.net/.../
The thing is that I never got any feedback from anybody as to the quality of my work; I have no way to judge my efforts. I was hoping that you would be so kind to help me. Do you see any serious problems with it? What are the major deficiencies? Do you even like it?
I know this is something you would probably expect from a boss of colleague. I'm asking it as a favor.

Thanks in advance,
Tamir Michael

Parents
  • 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

Reply Children
No data
More questions in this forum