On Wed, Jun 22, 2022 at 12:47:55PM +0530, SyncMonk Technologies wrote:
> - adding organizational TLV support for interface rate
> - delay asymmetry calculation for master and slave
So it could be split into two or three patches?
> --- a/config.c
> +++ b/config.c
> @@ -240,6 +240,7 @@ struct config_item config_tab[] = {
> GLOB_ITEM_ENU("clock_type", CLOCK_TYPE_ORDINARY, clock_type_enu),
> GLOB_ITEM_ENU("dataset_comparison", DS_CMP_IEEE1588, dataset_comp_enu),
> PORT_ITEM_INT("delayAsymmetry", 0, INT_MIN, INT_MAX),
> + GLOB_ITEM_INT("interface_rate_tlv", 0, 0, 1),
Why is it not a port-specific option? If you reject a suggestion, it's
a good practice to explain your reasoning.
> +uint64_t interface_bitperiod(struct interface *iface)
> +{
> + /* 10^18 atto-sec per bit*/
> + uint64_t if_atto_sec_bit_period = 0x0DE0B6B3A7640000;
This constant is ugly. I'd suggest to write it as 1000000000000000000.
> +
> + if (!iface->if_info.valid) {
> + return 0;
> + }
> +
> + if_atto_sec_bit_period /= iface->if_info.speed;
> + if_atto_sec_bit_period /= 1000000; /* 1 Mb = 10^6 */
> + return if_atto_sec_bit_period;
Wouldn't the function be easier to read if it was simplified to this?
uint64_t interface_bitperiod(struct interface *iface)
{
if (!iface->if_info.valid)
return 0;
/* Megabits per second converted to attoseconds per bit */
return 1000000000000 / iface->if_info.speed;
}
> --- a/port_signaling.c
> +++ b/port_signaling.c
> @@ -103,10 +103,38 @@ static int process_interval_request(struct port *p,
> return 0;
> }
>
> +static int process_interface_rate(struct port *p,
> + struct msg_interface_rate_tlv *r)
Please align the parameters in the declaration.
> +{
> + Integer64 delayAsymmetry;
> + double nsDelay;
> +
> + if (clock_interface_rate_tlv (p->clock) &&
> + interface_ifinfo_valid(p->iface)) {
And this too.
> + /* Delay Asymmetry Calculation */
> + delayAsymmetry = r->interfaceBitPeriod -
> + interface_bitperiod(p->iface);
> + delayAsymmetry = delayAsymmetry/2;
Inconsistent coding style (number of spaces around "=" and "/").
> + nsDelay = (double)delayAsymmetry / 1000000000;
Casting can be avoided by dividing by 1.0e9.
> + delayAsymmetry =
> + (r->numberOfBitsAfterTimestamp -
> + r->numberOfBitsBeforeTimestamp) * nsDelay;
I find it confusing when a variable is updated in multiple steps like
here, i.e. when it doesn't always contain a value according to its
name. I'd suggest to merge it into a single assignment, or use more
variables.
> + if (delayAsymmetry != p->portAsymmetry) {
> + /* Updating the nanosecond part */
This comment is confusing to me.
> --- a/ptp4l.8
> +++ b/ptp4l.8
> @@ -705,6 +705,12 @@ the interval, the sample will be printed instead of the
> statistics. The
> messages are printed at the LOG_INFO level.
> The default is 0 (1 second).
> .TP
> +.B interface_rate_tlv
> +When master and slave instances are operating at different interface rate,
> +delay asymmetry caused due to different interface rate needs to be
> compensated.
> +The master and slave exhanges their interface rate based on interface rate
> TLV
> +as per G.8275.2 Annex D.
> +The default is 0 (does not support interface rate tlv).
tlv -> TLV
> @@ -256,6 +281,16 @@ static int unicast_service_reply(struct port *p, struct
> ptp_message *dst,
> if (err) {
> goto out;
> }
> +
> + if (clock_interface_rate_tlv (p->clock) && duration > 0 &&
> + clock_telecom_profile(p->clock) &&
> + interface_ifinfo_valid(p->iface)) {
Please align the lines.
--
Miroslav Lichvar
_______________________________________________
Linuxptp-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
