Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add core register schema with protocol version #57

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

glopesdev
Copy link
Collaborator

Following discussions in #22, this PR introduces a core register schema with a protocolVersion field specifying the minimum semantic version mandating a given set of common registers.

The new JSON schema is named core.json and is to be used by both the common register metadata file and each of the device.yml files.

The common register metadata file is also renamed to core.yml to avoid the unusual terminology of "common". In Harp parlance, the "core" is always what we talk about when discussing requirements for hardware devices. There is no direct reference to this metadata file other than from the generator and analysis packages, which will be themselves updated in the near future to embed the new core.yml. Since these packages distribute their own snapshots, changing and renaming this file is not expected to be breaking.

Fixes #22

@glopesdev glopesdev added proposal Request for a new feature feature New planned feature labels Oct 30, 2024
@glopesdev glopesdev requested a review from a team October 30, 2024 22:15
"properties": {
"protocolVersion": {
"description": "Specifies the semantic version of the device protocol.",
"type": "string"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consider adding a pattern to validate the semver

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good idea, the following pattern works for me (note double backslashes to account for escape characters):

"pattern": "^(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\.(0|[1-9]\\d*)",

Should we demand full semantic version for this field (i.e. 3 numbers without prerelease) or leave it at only 2 numbers like the firmware version and hardware targets?

If we do use this, should we upgrade the firmwareVersion and hardwareTargets to use a pattern as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. I would leave the patch in preparation for #44 since it was unanimously agreed that having patch would be useful.

Copy link
Collaborator Author

@glopesdev glopesdev Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. In that case we need to review a few more details related to firmware upload file format. Currently the .hex metadata is specified using an exclusively major/minor format, e.g. see HarpVersion and FirmwareMetadata.

If we change this spec, we need to change these classes, possibly by introducing default fields (i.e. zero patch version by default everywhere), but also we need to decide very precisely how these releases are to be named and distributed going forward.

This is even more relevant because firmware and hardware version validation currently makes use of these version numbers in the Device constructor.

Shall we open a new issue / proposal with this?

Copy link
Collaborator Author

@glopesdev glopesdev Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to have a way to infer these versions from other places than the filename (e.g. a sort of file header). I am not sure if the atxmega or pico support this

I don't think this is supported by the Intel HEX format. I think these files are generated directly from the Atmel studio compiler (@filcarv can you confirm?). We could try to design our own firmware container format, which might help unify firmware updates, but it would take a while and the chances of getting it wrong are quite high at this stage I think.

I think that for now opening an issue in Bonsai.Harp to allow for a third version is what makes more sense. What do you think?

I am not too concerned about Bonsai.Harp at this point, since that is easy to change, but more about making sure that we completely cover all ripple effects of proposed changes. I have added some extra notes to the version register spec issue #44.

My main realization while thinking about this is that we really shouldn't have "patch" versions of a pure spec. I mean, there is no implementation to fix really, so if you change the spec this will always be at least a "minor" version, right?

Copy link
Collaborator Author

@glopesdev glopesdev Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To follow up on this, there is an argument why versions at the level of the device.json should be kept at 2 numbers, i.e. ignoring all patches. I will try to summarize individually below:

  • protocolVersion: there was an argument brought up that "patch" changes to the spec could be changes to documentation. However, this field is talking about device "requirements", and if these changes are never functional, there is no need to actually change the "requirement" of the device because a spec typo was fixed, since all devices implementing the old spec would still be entirely compliant with no change at all.
  • firmwareVersion: the abstract device interface needs to match a specific device firmware register layout. Similar to the above, patch changes to the firmware will not by definition ever change the register layout, so nothing at all will change in the abstract device interface. Old interfaces will work exactly the same in the new patched firmware, and vice-versa, so again it seems like any corrections in the firmware need not be included in the requirements for the device.yml. In terms of keeping devices up to date, the best way would be to embed firmware in the packages themselves, as discussed several times previously.
  • hardwareTargets: similar to the firmware discussion above, nothing about pure fixes in hardware implementation should have any implications on targeting of abstract device interfaces.

It seems therefore that we would potentially lose more than we would gain by restricting abstract interfaces to patch versions of the protocol, firmware and hardware.

Note that I am not suggesting here we don't include patch versions in the metadata retrieved from the device (as suggested in #44). That will still be extremely useful to log and expose. My discussion here is purely centered around the utility of having those patch versions exposed as "requirements" of an abstract device interface.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Playing devil's advocate here. Given the centralized source nature of the device.yml, it seems possible that consumers may want to use it to generate automatic documentation by targeting a specific version of the protocol. I agree with you in that it does feel like a weird scenario where people would target version X.Y.1 instead of X.Y.2, since .2 would be strictly better (patched). That being said, I wonder if there would ever be a case where this is a legitimate use.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I am following your argument correctly, but in that case couldn't they just target a specific hash or tag of the device repo and simply generate docs from there?

I guess your point is how they would then target the correct version of the core.yml? It does seem to be a bit of a stretch. To be honest for docs I would find it more convenient to simply target a version of the core Bonsai.Harp package.

What I am mostly trying to understand here is what is the cost/benefit analysis of changing interpretation of device schema versioning and I am now leaning towards that there may not be so much benefit that would justify the costs, but maybe we can discuss this in a forthcoming meeting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess your point is how they would then target the correct version of the core.yml? It does seem to be a bit of a stretch. To be honest for docs I would find it more convenient to simply target a version of the core Bonsai.Harp package.

The issue here is that it would couple a language-agnostic standard (device.yml) to a bonsai package.

but in that case couldn't they just target a specific hash or tag of the device repo and simply generate docs from there?

Yep this is indeed the use case I was thinking. But instead of using a hash, in my mind, providing a device.yml would provide all the information instead. This being said...

What I am mostly trying to understand here is what is the cost/benefit analysis of changing interpretation of device schema versioning

I think this is the crux. As i mentioned before, I don't see a scenario where someone would like to generate assets against an out-of-date version of the core.yml, since patches are by definition always backwards compatible and bug-fixing in nature. Assuming the consumer always wants the "most" updated within MINOR version seems sensible.

schema/device.json Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldnt we add the platform (or any other name) here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the question I left open in the discussion. I don't mind adding this to the PR and making a bigger change with all the updates to the JSON schema, but then we should decide in detail what exactly to name these new fields. I added here only the things that I felt we had enough detail to proceed, happy to get more feedback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My fear is that this will keep delaying the inevitable decoupling of the harp protocol version from the implementation. I think we need to be explicit about it and this would be one way to go about it. But I agree that it is not one or the other. Lets merge this as is, and add the other field afterward. If the field has a default, probably "null" even, we can make the change backward compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New planned feature proposal Request for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for versioning core register schema
2 participants