On Mon, Oct 04, 2021 at 09:24:31AM +0200, Miroslav Lichvar wrote:
> On Sat, Oct 02, 2021 at 07:09:02AM -0700, Richard Cochran wrote:
> > The function, clock_time_properties, is a query that should not change
> > the program state.
>
> So, would you like to move it to a new function, e.g.
> clock_update_time_properties()?
Yes, that would be more clear.
> > With this change, one call chain will be:
> >
> > port_tx_announce
> > clock_time_properties
> > clock_update_utc_offset
> >clock_update_grandmaster
> >
> > That clobbers the grandmasterIdentity with the local dds.clockIdentity
> > in case of BC.
>
> The function specifically checks for grandmaster:
>
> > > + /* Don't do anything if not a grandmaster with a leap flag set. */
> > > + if (c->cur.stepsRemoved > 0 || leap == 0) {
> > > + return;
> > > + }
> > > +
>
> If it's a BC, stepsRemoved should be larger than zero, right? I
> thought that was easier than looking for a port in the slave state.
I see.
Still this bothers me. Global functions that query shouldn't have
side effects. In C++ one would mark clock_time_properties() as
const.
The logic that needs the LS flags cleared (Tx Announce) should do that
explicitly.
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?
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.
Rename clock_update_grandmaster() to clock_reset_local_gm().
Thoughts?
Thanks,
Richard
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
