On Tue, Mar 26, 2019 at 07:42:57PM -0700, Richard Cochran wrote:
> On Tue, Mar 26, 2019 at 10:06:08PM +0000, Patel, Vedang wrote:
> > After pondering a little bit more, I think that switch/case is not suitable
> > in this scenario. The current we switch/case is implemented is as follows:
> >
> > switch (*state) {
> > case SERVO_UNLOCKED:
> > servo->curr_offset_values = servo->num_offset_values;
> > break;
> > case SERVO_LOCKED:
> > if (check_offset_threshold(servo, offset)) {
> > *state = SERVO_LOCKED_STABLE;
> > }
> > /* fall through. */
> > case SERVO_JUMP:
> > servo->first_update = 0;
> > break;
> > case SERVO_LOCKED_STABLE:
> > break;
> > }
> >
> > But, here SERVO_LOCKED_STABLE case will never occur.
>
> Even if it is a noop, still enumerating the cases is clearer to me.
> You could add a comment explaining why that case is never reached.
Actually, this switch/case shouldn't make any assumptions about how
the sub-module might have set 'state'. It might go LOCKED-JUMP-LOCKED
for example.
So you need (servo->curr_offset_values = servo->num_offset_values) in
case JUMP as well.
Also, please avoid the fall through. Just duplicate the clearing of
servo->first_update. The compiler will figure the optimization out.
Thanks,
Richard
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
