This discussion has been locked.
You can no longer post new replies to this discussion. If you have a question you can start a new discussion

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

Reply
  • 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.

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