glopesdev commented 7 months ago

Following the developments in harp-tech/reflex-generator#57 and harp-tech/protocol#21, we started wondering there is currently no way of versioning the common YML file itself.

One option to workaround this is to add an additional schema for the core, e.g. core.json which would require an extra property on these YML files which is the core version. The number added there would have to match the core version used to implement the device firmware for each device.

There might be a few more details to think about related to how we merge this common YML file with the individual device schemas, but this versioning will be important so we don't accidentally start including new registers in a joint device map which are not actually implemented by the device firmware if it is still using an older harp core.

bruno-f-cruz commented 7 months ago

Should also be discussed together with https://github.com/harp-tech/protocol/issues/13

glopesdev commented 5 months ago

Are core registers tied to a specific core implementation version? no, they are tied to specification version

coreVersionHigh / Low should return the version of the core firmware implementation (atmel core or pico core)
device.yml needs to declare which version of the core specification it implements

Outstanding questions:

are we ok with using "draft-xx" for versioning / url of meta-schema file
what name should this field have?
where do we need to change meta-schema? is it enough to add a new field to device.json? do we need to also add a core.json to use as meta-schema for common.yml?
are we ok with the name common.yml?
should the core version high / low registers return the version number of the common.yml file that it implements?
- if yes, do we also need to return the version of the core firmware anyway? maybe not required since there is a tight link between firmware version and harp core firmware version (if core changes, firmware version needs to bump up)

One cautionary note is that we've had in the past versions where critical core firmware behavior bugs get introduced at specific core versions.

glopesdev commented 3 months ago

One important consideration is there can be releases of the same device firmware version for different core implementations, for compatibility reasons. This suggests we probably want an extra register to store the specification version.

A proposal which was suggested is to create a new single register containing all version information (hardware, firmware, core, specification) encoded as a payload specification. This would allow us to extend the range of versioning (e.g. to include a new patch version number, the new specification versions, etc) as well as providing a single register to retrieve the entire version information in a single call.

Example definition

  VersionInfo:
    address: 15
    type: U8
    length: 12
    access: Read
    description: Stores the version info for the device hardware, firmware, core, and specification used.
    payloadSpec:
      HardwareVersion:
        offset: 0
        description: Specifies the hardware version of the device.
        interfaceType: HarpVersion
      FirmwareVersion:
        offset: 3
        description: Specifies the firmware version of the device.
        interfaceType: HarpVersion
      CoreVersion:
        offset: 6
        description: Specifies the core version of the device.
        interfaceType: HarpVersion
      SpecVersion:
        offset: 9
        description: Specifies the spec version of the device.
        interfaceType: HarpVersion

Outstanding questions

Name of the register
Number of version components (2 or 3)?
Order of version components (hardware, firmware, core, spec or something else?)
Interface type used to represent version in Bonsai.Harp, there is currently a HarpVersion type already, but it only includes two components (high / low)

bruno-f-cruz commented 3 months ago

Related to #44

glopesdev commented 3 months ago

Actually, I know realise perhaps the true implications of the comment I had made 4 months ago:

device.yml needs to declare which version of the core specification it implements

This actually opens up another solution to implement this versioning without the need for an extra register: if the device.yml for the device declares which version of the specification is implemented, then it would be possible to deduce immediately from the device.yml which core registers are available, without the need to change anything about the current registers.

Because the change on the device.yml described here is required anyway, and is not exclusive with #44, I am now leaning towards resolving just this issue first and only afterwards worrying about the new version registers.

bruno-f-cruz commented 3 months ago

I think we need both actually. Otherwise how would you ensure that the hardware you are connecting to implements a specific version of the protocol? There is no guarantees.

If you use the device specific nodes, I can see a way where you could cross reference the implementation version to the protocol version, but this would require saving multiple yml files per assembly and searching through them (since in theory multiple implantation versions might target the same protocol version). Additionally, the core harp package still needs to know which protocol versionthe target device is speaking and here running this cross reference is much harder since we don't keep the device yml in this assembly. So not sure if I follow how a new register is not necessary.

Edit: after re reading your comment, if all your are saying is that this change should take temporal precedence over the register, I agree. It should also likely be folded together with #43! Which raises the question if we should also version the hardware core implementation that the device implements. I don't think this is necessary for now but might as well consider it...

glopesdev commented 3 months ago

Edit: after re reading your comment, if all your are saying is that this change should take temporal precedence over the register, I agree.

Yes, this is what I was recommending to get this out quicker.

Otherwise how would you ensure that the hardware you are connecting to implements a specific version of the protocol? There is no guarantees.

There is one guarantee that you can make via the firmware version. We know that each specific device firmware version is linked to a unique device.yml file. If this YML file contains in addition the specification version, as proposed in this issue, then indirectly we do know the version of the core registers.

Especially once we start saving the device YML files together with data logging, I suspect we could use just this new field in the YML for a really long time. In fact for data analysis, apart from maybe some QC, I don't think we will need to care much about the spec version register at all.

P.S. In fact this is also why I was delaying updating the reflex generators with new core registers until this issue was resolved. Basically by doing that we can assume that if a YML file does not specify a spec version, it targets the first reflex generator core, because everything else after that will be explicitly specified.

bruno-f-cruz commented 3 months ago

Ok that makes sense!

Regarding the second point, maybe for data analysis, but for online qc makes things much harder. Additionally it will require people to always use a yaml file, essentially making it impossible to validate the protocol version when using the generic interface.

glopesdev commented 3 months ago

That is true, but I thought the generic device interface was to be used without validation, to allow for rapid prototyping and debugging. Right now there is no validation by design, and in that case we would have to implement the validation ourselves anyway.

bruno-f-cruz commented 3 months ago

I think we might be thinking about a different kind of validstion tho. The device specific validation makes sense since a specific device implements a specific set of registers and functions which are both locked to the version of the device yaml it is used to generate the interface. The generic interface doesn't care about either of these BUT it does care what version of the protocol the device implements since all the core registers are implemented via this package. The core registers are, in turn, locked to a specific protocol version. Otherwise how else would you be able to know what bonsai.harp package version to install ? Unless we add a core.yml file to the bonsai.harp package that affords manual validation.

glopesdev commented 3 months ago

If we don't introduce breaking changes in the protocol, I don't think we need to worry about validating generic Harp devices.

The reason is that all changes are incremental in terms of supported functionality. Even old generic interfaces will be able to accept devices with new registers, and even log everything (yet another advantage of using numbers instead of names for logging).

If you use an old interface you just won't be able to parse the new registers or send newer commands, but to me this seems completely fine and expected, especially since using newer registers is something that is required really only at the workflow level. In other words, if the workflow doesn't need a new functionality anyway, it would work fine with an older core package, even if working with newer devices.

Adding the common.yml metadata to the harp core package is already the plan and it is required for analysis. However, this would not by itself solve the issue of major version compatibility I think. Introducing breaking changes is part of an ongoing bigger and deeper discussion that I feel won't be easily solved by validating single device registers.

As soon as we introduce breaking changes in the protocol and try to mix old and new devices in the same workflow there would be many more things breaking because you can really only have one version of the protocol client assembly installed right now.

I can think of several ways to allow co-existence in the future, but they are all painful, and all of them would force us to spend more maintenance resources than what we can currently allocate.

So on this I agree with @filcarv that we should absolutely try to avoid breaking the protocol for as long as we possibly can until we stabilise both our requirements and the ecosystem.

bruno-f-cruz commented 3 months ago

Ok I agree that they are separate issues and if there are no breaking changes the interface should work. B it I still don't see a user would be able to know what protocol version the device implements without the additional register. Say you use an interface that targets app1.2core2.3 and your interface is for app1.3core2.4. It so happens they the device will work because no breaking changes were made BUT the device yaml that gave rise to the interface knows that app1.3core2.4 happens to be associated with protocol version 0.3. Critically the yaml doesn't know anything about app1.2core2.3. This device yaml might not even exist. How would the user recover this information? This is what I am worried about by not having it explicitly tracked in the device firmware.

The user would need to go to some source firmware file and try to figure what what version of the protocol core2.3 implements....

bruno-f-cruz commented 2 months ago

21 depends on this

bruno-f-cruz commented 2 months ago

@glopesdev
The current suggestion would thus include:

Create a new core/common.json that mirrors the "device.json" but specifies the structure for the core, along with a field to track the version;
Refactor common.yml to follow the last point
To the device.json, add a new field to track the Harp protocol version (in order to attempt to cross-reference to the previous point) and a new field to track "core" (e.g.: HarpTech/RP2040, HarpTech/Atxmega, Aind/RP2040, etc...)
- Use / (ideally from GitHub) as the default name of the implementation. This allows for scoping name spaces across forks for instance...

harp-tech / protocol

Add support for versioning core register schema #22

Example definition

Outstanding questions

21 depends on this