[PATCH 8/8] score: Replace watchdog handler implementation

Sebastian Huber sebastian.huber at embedded-brains.de
Thu Mar 3 21:09:47 UTC 2016


Thanks a lot for your review.

----- Gedare Bloom <gedare at rtems.org> schrieb:
> I scanned through most of it. I just have a few minor issues/questions:
>
> There is an _Assert(0) followed by a TODO. This _Assert() is likely
> dead/untested code block. It may be worth fabricating a fatal test for
> it, if possible?

This one:

> +  if ( pthread_kill ( ptimer->thread_id, ptimer->inf.sigev_signo ) ) {
> +    _Assert( FALSE );
> +    /*
> +     * TODO: What if an error happens at run-time? This should never
> +     *       occur because the timer should be canceled if the thread
> +     *       is deleted. This method is being invoked from the Clock
> +     *       Tick ISR so even if we decide to take action on an error,
> +     *       we don't have many options. We shouldn't shut the system
> down.
> +     */

Its a code move from one file to another, so nothing new.

>
> "be placed on Red-Black Trees for set management." copy-pasted comment
> should be Chains?

Thanks, for spotting this.

>
> "watchdog is scheduled and a black node". ditto, black should be red
> for the second one.

Oh, yes.

>
> _Watchdog_Ticks_from_seconds(): why is ticks = seconds<<30 the right
> thing to do? Same for _Watchdog_Ticks_from_timespec(). I am missing
> some assumption here, I guess. It might improve readability to provide
> a helper function for this.

Ok, sorry for the magic numbers.  2**30 == 1073741824 enough to cope with 1e9 nanoseconds.  So, we have 2**34 seconds available, leading to a year 2514 problem.



More information about the devel mailing list