Open bruno-f-cruz opened 8 months ago
Feedback from SRM meeting:
Pros:
Cons / Questions:
Pros:
Cons / Questions:
An alternative proposal would be to reserve space in the general Harp message header to have "per-register" status / metadata. In this proposal, the status bits would then be set only for the heartbeat message, but without the need to extend the size of the register
Special status bits only for specific registers
An alternative proposal would be to reserve space in the general Harp message header to have "per-register" status / metadata. In this proposal, the status bits would then be set only for the heartbeat message, but without the need to extend the size of the register
I don't think this is warranted since the synchronization protocol is not continuous (i.e synchronization is only ensured once every second). I think a periodic event is more inline with this.
I don't think this is warranted since the synchronization protocol is not continuous (i.e synchronization is only ensured once every second). I think a periodic event is more inline with this.
My suggestion was not to add this for every message, only for heartbeat events, hence register-specific status bits, I will explain better in the SRM, but I agree probably not worth it anyway.
Feedback from SRM meeting:
Ok from all the past discussions I believe these are the critical points to move forward:
What register will this bit flag be added to?
OpControl
or CLK_CONFIG
)? (bit name SynchStatus
)?? I suggest Bit2 of OP_REG or Bit 5 of CLK_CONFIGWhat is the expected behavior?
Seconds
core register`) will be disabled and replaced by a new 1hz periodic event. This event will contain a flag that will state whether the device is currently being externally (True) synchronized or generating its own clock reference (False).Related to #32 as this register could also report the error state of the device
We want to leverage the heartbeat event to relay information about the current status of the device. For now, this could be circumscribed to the synchronization state of the device, but having the space to introduce extra flags later on is critical. This could be used to relay other "status" messages, such as error states, communication layer quality (e.g. for wireless devices), etc...
We ended up with three possible distinct implementations with respective pros and cons that I will briefly list.
OP_REG
registerThe most immediate solution would be to use the current OP_REG
and co-opt the Bit2 of the register to implement this behavior. This change would be fully backward compatible when it comes to the interface, but people would need to change the event they use to synchronize boards. Additionally, the event would also report the current state of the device (Bit0-1) which could also be used to solve #32 if we use the State=2 value to indicate a generic error state.
This option has two main drawbacks. First, it uses the last available bit in the register, so future flags to indicate the state of the device would be impossible to add to the current configuration. Second, it "muddies" the waters by having a register that is used to Write (e.g. device state, alive status, led status) and Read-only (the new event).
The main advantage is keeping one likely relevant piece of information regarding the state of the device, in this case, the OPMODE
, in the same register (and thus adjacent memory) that is responsible to report the status of the device. It is also backward compatible as mentioned before.
OP_REG
registerAn extension of the previous implementation. The register would suffer a breaking change by going from a 8bit to a 16bit register, allowing more flags to be encoded in the future. It would still suffer from the other issue. Importantly, this would be a breaking change, and its implementation should wait for a larger batch of breaking changes thus delaying its deployment. (Side note, would it be possible to turn the register into a uint8[2] instead of uint16? This could potentially allow for backward compatibility)
Name
: 'Heartbeat' or 'Status'Size
: Likely a U16 so we have ample space for future flagsAccess
: Read-onlyalive
flag.Seconds
core register) will be disabled and replaced by a new 1Hz periodic event. This event will contain a flag that will report whether the device is currently being externally (True) synchronized or generating its own clock reference (False).This suggestion would be backward compatible as it adds a new register. It also splits the write vs read access issue from the previous two solutions. The main drawback is the separation of the OPMODE from this register and the consumption of another <32 core register.
Some more thoughts from the discussion:
Active
to Standby
mode due to the nature of the device state machine, so reporting any internal changes to this existing register as an event might be warranted.[!NOTE] The above notwithstanding, this is currently the only spontaneous change in the value of the register. All other bits seem intended as configuration states and will never be changed by the device. If a new status or heartbeat register is introduced, we can specifically introduce a new bit to represent whether the device is in active or standby mode (see below).
[!NOTE] On further consideration, I feel we shouldn't entirely duplicate the state of
OP_MODE
in the new register, and would argue the only meaningful bit to report is whether the device is active or on standby.
It was argued that leftover bit 2 might be used as a backwards compatible toggle between the old heartbeat behavior of reporting R_TIMESTAMP_SECOND
and the putative new register. However, this might actually be more error prone in the mid-term if devices are updated without changing their behavior, since it raises the risk of mixed formats in setups with multiple devices; also see below.
The leftover bit 2 is better positioned to extend OP_MODE
to 8 possible values, which may be useful in future devices, e.g. wireless devices could maybe benefit from a special mode different than speed mode.
[!WARNING] The
R_OPERATION_CTRL
spec is currently wrong since it lists this register as 16-bit whereas historically it has so far been an 8-bit register. This appears to be a typo when porting the original DOCX / PDF document to markdown, since the original document was correct. After discussion ends we should make sure to review all registers in this document.
Notes from 29022024
We will go with the new additional register. I propose the following specs:
Name: Heartbeat Size: U16 Access: Read, Event BitFlags:
I will draft a PR with a complete proposal
It would be very helpful to have an additional flag indicating whether a device is currently synchronized (i.e. receiving a synchronization message on every second) is a source. This is super relevant for online QC but also when using the same clock device across multiple setups, since there is no easy way to communicate with the same clock synchronizer from two different computers for instance.
This change should probably be made to the core and result in a new release. If we add it as an additional array position, it should even be backward compatible.