Re: [Linuxptp-devel] [PATCH 3/4] clock: Clear leap flags after leap second.

2021-10-05 Thread Richard Cochran
On Tue, Oct 05, 2021 at 12:27:14PM +0200, Miroslav Lichvar wrote:
> On Mon, Oct 04, 2021 at 06:56:50AM -0700, Richard Cochran wrote:
> > How about this?
> > 
> > 1. Refactor the setting of clock.tds into a common helper function, as
> >you suggest.
> > 
> > 2. Provide a global function to clear the LS flags, called from
> >port_tx_announce explicitly.
> > 
> >This could be clock_update_utc_offset(), but the name might suggest
> >that the clock would be stepped.  Maybe clock_clear_leap_flags()
> >instead?
> 
> Or maybe clock_update_leap_status()? I have no strong preference for
> any of the three.

okay.

It would make even more sense to me to call it from the timer
expiration block, like so:

case FD_MANNO_TIMER:
pr_debug("%s: master tx announce timeout", p->log_name);
port_set_manno_tmo(p);
+   clock_update_leap_status(p->clock);
return port_tx_announce(p, NULL) ? EV_FAULT_DETECTED : EV_NONE;


> > 3. And BTW, the naming of clock_update_grandmaster() isn't great.
> >Long ago I couldn't think of a better name at the time.  It sounds
> >like it would update something about a remote GM.
> 
> I think it's consistent with clock_update_slave(). If one is renamed,
> should the other follow?

Good point.  Let's leave them alone then.

Thanks,
Richard


___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel


Re: [Linuxptp-devel] [PATCH 3/4] clock: Clear leap flags after leap second.

2021-10-05 Thread Miroslav Lichvar
On Mon, Oct 04, 2021 at 06:56:50AM -0700, Richard Cochran wrote:
> How about this?
> 
> 1. Refactor the setting of clock.tds into a common helper function, as
>you suggest.
> 
> 2. Provide a global function to clear the LS flags, called from
>port_tx_announce explicitly.
> 
>This could be clock_update_utc_offset(), but the name might suggest
>that the clock would be stepped.  Maybe clock_clear_leap_flags()
>instead?

Or maybe clock_update_leap_status()? I have no strong preference for
any of the three.

> 3. And BTW, the naming of clock_update_grandmaster() isn't great.
>Long ago I couldn't think of a better name at the time.  It sounds
>like it would update something about a remote GM.

I think it's consistent with clock_update_slave(). If one is renamed,
should the other follow?

>Rename clock_update_grandmaster() to clock_reset_local_gm().

-- 
Miroslav Lichvar



___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel