[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