Inconsistency in servo state enumeration
Concerns wr-cores and ppsi repositories. Issue reported by David Emrich:
For starters, in ppsi/include/hw-specific/wrh.h we can see:
/* Servo state */
typedef enum {
WRH_UNINITIALIZED = 0,
WRH_SYNC_TAI,
WRH_SYNC_NSEC,
WRH_SYNC_PHASE,
WRH_TRACK_PHASE,
WRH_WAIT_OFFSET_STABLE,
}wrh_servo_state_t;
While, in ppsi/proto-ext-common/wrh-servo.c, the states are described in the following order:
static const char *wrh_servo_state_name[] = {
[WRH_UNINITIALIZED] = "Uninitialized",
[WRH_SYNC_NSEC] = "SYNC_NSEC",
[WRH_SYNC_TAI] = "SYNC_SEC",
[WRH_SYNC_PHASE] = "SYNC_PHASE",
[WRH_TRACK_PHASE] = "TRACK_PHASE",
[WRH_WAIT_OFFSET_STABLE] = "WAIT_OFFSET_STABLE",
};
You can see that the ordering between the NSEC and TAI terms is reversed between the two. So, when executed, the array contents actually look like...
static const char *wrh_servo_state_name[] = {
[0] = "Uninitialized",
[2] = "SYNC_NSEC",
[1] = "SYNC_SEC",
[3] = "SYNC_PHASE",
[4] = "TRACK_PHASE",
[5] = "WAIT_OFFSET_STABLE",
};
On the face of it I don't think this is crucial as long as all users of that array ONLY reference the first argument as the numeric state variable and never reference the index/position within the array itself. However for consistency and clarity, I would suggest that the ordering inside ppsi/proto-ext-common/wrh-servo.c be changed to match the enumerated type in the wrh.h header file.
Moving on, once again, inside wr-cores/modules/wrc_core/wrc_syscon_wb.wb, we see
reg {
name = "WRPC Diag: servo status";
prefix = "WDIAG_SSTAT";
field {
name = "WR valid";
prefix = "wr_mode";
description = "0: not valid\1:valid";
type = BIT;
access_bus = READ_WRITE;
access_dev = READ_ONLY;
};
field {
name = "Servo State";
prefix = "servostate";
description = "0: Uninitialized\
1: SYNC_NSEC\
2: SYNC_TAI\
3: SYNC_PHASE\
4: TRACK_PHASE\
5: WAIT_OFFSET_STABLE";
type = SLV;
size = 4;
align = 8;
access_bus = READ_WRITE;
access_dev = READ_ONLY;
};
And inside wr-cores/modules/wrc_core/wrc_diags_wb.wb we see:
reg {
name = "WRPC Diag: servo status";
prefix = "WDIAG_SSTAT";
field {
name = "WR valid";
prefix = "wr_mode";
description = "0: not valid\
1:valid";
type = BIT;
access_bus = READ_ONLY;
access_dev = WRITE_ONLY;
};
field {
name = "Servo State";
prefix = "servostate";
description = "0: Uninitialized\
1: SYNC_NSEC\
2: SYNC_TAI\
3: SYNC_PHASE\
4: TRACK_PHASE\
5: WAIT_OFFSET_STABLE";
type = SLV;
size = 4;
align = 8;
access_bus = READ_ONLY;
access_dev = WRITE_ONLY;
};
};
Barring minor formatting, these two extracts from the .wb files are the same thing but they are BOTH different from the order enumerated in the original wrh.h header.
Does this mean that the state numbers that are running inside the WR hardware are different from the numbers assigned in the MIB databases? (And wherever else these .wb files are used?) Is that important or does "everyone" reference the name only and the numbers are ignored?
What about if there is a similar servo state variable within any FPGA code?
I bring this to your awareness in case it might be the heart of some as-yet un-diagnosed issue that only appears when any given White Rabbit system is "out of lock", either during initialisation or if something fails during operation.