Add UID, Tag, Heartbeat and Version registers to core interface specification

[bruno-f-cruz]

This PR updates the core.yml with the missing registers and bit fields.

• Following Add UID register to protocol #39, Add Tag register to protocol #38, Add hearbeat register #45 and 68, we add the interface specification for UID, Tag, Heartbeat and Version registers
• Following Deprecate R_TIMESTAMP_OFFSET from Device specification #113 , we deprecate thie register and remove its mention from the interface
• Following Add ability to trigger firmware update mode from Harp protocol #166 we add the specified bit to the R_RESET register

[bruno-f-cruz]

@glopesdev
I was playing around with the "Version" register, and I am not super happy how it turned out. I think we would greatly benefit from having #40 and #118 solved, but you may have a better idea.

[glopesdev]

@bruno-f-cruz I will have a more in-depth look, but a first quick question is about the "deprecated" attribute.

Is that part of the schema yet? I can't seem to find it anywhere.

[bruno-f-cruz]

@bruno-f-cruz I will have a more in-depth look, but a first quick question is about the "deprecated" attribute.

Is that part of the schema yet? I can't seem to find it anywhere.

I targeted #154, assuming that it would get accepted. I can split in two, but some of the registers need to be deprecated in this PR anyway.

[glopesdev]

I was playing around with the "Version" register, and I am not super happy how it turned out.

@bruno-f-cruz I had a look and for this specific register it might be easier to leverage a custom converter and go for a simpler schema:

  Version:
    address: 19
    type: U8
    length: 32
    access: Event
    payloadSpec:
      ProtocolVersion:
        offset: 0
        interfaceType: HarpVersion
      FirmwareVersion:
        offset: 3
        interfaceType: HarpVersion
      HardwareVersion:
        offset: 6
        interfaceType: HarpVersion
      SdkId:
        offset: 9
        interfaceType: string
      InterfaceHash:
        offset: 12
        interfaceType: string
    converter: RawPayload

This generates the following payload type:

        public VersionPayload(
            HarpVersion protocolVersion,
            HarpVersion firmwareVersion,
            HarpVersion hardwareVersion,
            string sdkId,
            string interfaceHash)
        {
            ProtocolVersion = protocolVersion;
            FirmwareVersion = firmwareVersion;
            HardwareVersion = hardwareVersion;
            SdkId = sdkId;
            InterfaceHash = interfaceHash;
        }

and the partial conversion stubs:

        private static partial VersionPayload ParsePayload(ArraySegment<byte> payload);

        private static partial ArraySegment<byte> FormatPayload(VersionPayload value);

The changes we would need to make this effective is to extend HarpVersion with an extra nullable patch member, and implement the converter in the core.

Later for harp-python we can have a custom converter as well. Since this is a well-defined core register there may be no need to overcomplicate the spec further since we can have control over the core packages.

[bruno-f-cruz]

Sounds good to me. One small detail is whether the patch should be nullable or default to 0.
I don't necessarily have a strong opinion either way, but to be consistent with the SemVer reference in the protocol, that in turn says:

A normal version number MUST take the form X.Y.Z where X, Y, and Z are non-negative integers

It sounds like we should consider making it 0? Or do you have a different interpretation?

[glopesdev]

It sounds like we should consider making it 0? Or do you have a different interpretation?

The HarpVersion type is used both for versioning and for matching, so you can see all components are actually nullable, where null simply means "match against anything".

This allows us to do version matching, where something with a version 1.0.x will match against both 1.0.0 and 1.0.1. To avoid breaking changes, the new member would have to be nullable.

We can discuss pros and cons of this, but from what I experimented with in bonsai-rx/harp#103, adding Patch as nullable shouldn't break or limit anything relative to the other properties for Major and Minor. If you think of anything in addition to the unit tests I already added, feel free to review that PR with the edge case and I will add it to the battery of regression tests.

[bruno-f-cruz]

Did not realize they were nullable. In that case it makes sense, sure!

[bruno-f-cruz]

@glopesdev I was looking at this deprecated spec again and wondering if we can make this generally more useful. Looking at pydantic's interface, they allow deprecated to be a null | string type. If null (default) the register would be deemed not deprecated; else, the register is deprecated with the string message. I think this would be quite a useful feature to potentially help surface these deprecated warnings to the regenerated code.

What do you think?