-
Notifications
You must be signed in to change notification settings - Fork 6
Add semantic version register #68
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should probably decide on what shape the document to keep track of the SDK takes. I think the what makes most sense is to add it to the who_am_i database as yet another field. All devices currently registered are either ATXMEGA or pico so it shouldn't be too hard to manually add this information without breaking things.
I was thinking to possibly add it into the JSON schema directly by resolving #43. |
From SRM DeviceName:
address: 12
type: U8
length: 25
interfaceType: string
access: Write
description: Stores the user-specified device name. This requires:
-From the point of view of the schema that defines the device.yml, we also discussed that would be important to keep a list of maintained "interfaceType" in a form of a controlled vocabulary (i.e. OneOf). It is then the responsibility of downstream tooling to maintain a mapping between "interfaceType" and converters.
|
This PR is likely to undergo further modifications given discussions in https://github.com/orgs/harp-tech/discussions/129 |
After consolidating feedback from multiple SRMs (see above) the current proposal will be updated to the following:
This gives us an exactly 32-byte register with all the relevant versioning metadata for validation. To summarize:
We will move to a quick 5-min approve or refuse point in the next Harp SRM, so any last comments or objections, please bring them now. Footnotes
|
@glopesdev A few notes, but otherwise I think this is making sense.
|
I guess we could do that, but then shouldn't we also equally argue to have the firmware version first, since that is probably what we want to validate? In the end perhaps the order is not so important since we can always index whatever we want. Perhaps another possibility to discuss is the meaning of the core version? In the end if by "core" we mean the SDK, this is just a dependency of the firmware. If we know the firmware version we must already know the core version from that so how informative would it be really? Instead, perhaps the core "interface" version is what might be most useful, so it could tell us quickly about the capabilities of the board in terms of core operations and core data logging (e.g. can it do heartbeat, tag, etc). If we do agree that core interface registers are not really "optional", and that they really must all be present as soon as the device implements a specific core interface, then I agree this likely should be moved first.
I did try to suggest that but @PathogenDavid pointed out that there are advantages to making it very unambiguous what exactly is being hashed. If we did decide to hash something else then this would be a breaking change in validation anyway. If we have a name for what the device.yml is that is equally unambiguous we could change to that I guess, but we have always been calling it "device.yml".
I agree, and I would say further that we make it clear that the TAG is expected to never match this hash. This is not a hash of the firmware, or the git revision, but rather of the device.yml contents. You can have in TAG whatever you want but it is not related at all to this number and can be simply left zero.
This was merged into the schema in #57. However, it hasn't been rolled out yet to the generators since we haven't resolved all these details, and I would like to make sure we have everything untangled and clear before doing it. We can always say that, before the rollout, all devices simply implement the first formalised core schema (there were no formalised versions before that, and we still haven't released any other changes). This does bring up a good point about what constitutes a "release" of the protocol repository. I think there should probably be a unified release as well, as having all these different bits and pieces versioned differently (binary protocol, device core schema, sync protocol, etc, etc) is really not helping anybody. I think we should really simplify and unify here: core_version == protocol release; device_version == device release (firmware+interface). And then get rid of everything else since we won't be able to really keep track of it other than what we are already doing by keeping it in the repository. |
I think that the
To clarify, by |
That's right, this is exactly Thinking more about your point on the name of Given this, the proposal would be updated to the following:
This gives us an exactly 32-byte register with all the relevant versioning metadata for validation. To summarize:
We will move to a quick 5-min approve or refuse point in the next Harp SRM, so any last comments or objections, please bring them now. Footnotes
|
Still related to the
|
I would not consider a merge commit to main to be a release as it would not give us time to revise and test ourselves internally the protocol specification as a whole, and we know from experience this is an important process in assessing the quality of the specification.
This is only true if you are looking exclusively at core register specification changes. However, I am now proposing we unify releases of all contents of this repository into a single version, which will refer to all the different moving parts, including schema changes, synchronization protocol changes, etc. As we have seen recently, some of these changes can result in subtle behavior differences, e.g. fixes being deployed to resolve clock alignment issues. These did not imply necessarily any change to the core register interface but did result in very important changes to device synchronization behavior with implications all the way to downstream data analysis. Additionally, if we start using interface hashes as part of the contents of
Yes, I was going to make a separate PR to fix that. We can decide either to pack the old changes into "releases" (versioned according to core atxmega releases for legacy compatibility) and make a change log like that, or alternatively we can leave the changelog close to the files, but with dates instead of version numbers.
Yes, this will be a decision to be made by the maintainer of each device. In order to be compliant with this spec, first of all the device needs to implement From the moment the device implements |
1466b0a
to
784ddb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth describing the deprecation of the old version registers in favor of the new one. Something like:
R_CORE_VERSION_H
(U8) – Major Core Version
Address: 004
Contains the major version of the Harp Protocol specification.
Warning
This register is deprecated in favor of R_VERSION
. The value of this register is expected to be equal to the major version of the R_VERSION
Protocol version.
We also refine our terminology by referring to the `device.yml` file as the interface schema file.
784ddb1
to
1f4afdf
Compare
Summary
This PR adds a new register to the core device schema called
R_VERSION
.Motivation
The current registers 01-07 are used to version hardware, assembly, harp core, and application firmware.
The assembly version has been unused in practice, and we are now moving in a direction where we need to keep track of the implemented protocol version explicitly, since the harp core and its implementation have diverged and now support multiple core implementations (pico and atxmega). The current version registers also do not support specifying a "patch" version, and require multiple read queries to retrieve all version components.
Rather than reshuffling and adding new registers with the missing information, we propose adding a single new register
R_VERSION
, which will pack all the required information.Detailed Design
A new register will be added with the following specs and layout:
Name: R_VERSION
Address: 19
Format: U8[32]
Access: Read-only
For the sake of backwards compatibility, we will keep all the existing registers mirroring the contents of the major and minor version components in the corresponding bytes of the new
R_VERSION
register.This gives us an exactly 32-byte register with all the relevant versioning metadata for validation. To summarize:
PROTOCOL_VERSION
: required to know synchronization guarantees and core register capabilities, e.g. devices might not synchronize correctly if not all sharing same protocol versionFIRMWARE_VERSION
: firmware and interface version will now be effectively identical and are expected to validate directly against each other. Following Use a single version number in device repositories #145 we will start enforcing joint releases of firmware and interface, with rebuilds of firmware and interface even if nothing changedHARDWARE_VERSION
: required to know the board revision number, e.g. Behavior 1.0, 1.1, 2.0SDK_IDENTIFIER
: which core SDK are we using (core.atxmega, core.pico, microharp, etc)INTERFACE_HASH
: the last line of defence in validation is a SHA1 hash digest of thedevice.yml
file which is implemented by the device firmware. This will give us a way to automatically and universally validate that the interface knows how to talk to the device, even in the case of hacks, forks and unpublished tweaks.12R_TAG
can still optionally encode the git revision hash, but that will remain in a separate register.We will move to a quick 5-min approve or refuse point in the next Harp SRM, so any last comments or objections, please bring them now.
Design Meetings
Closes #44
Footnotes
INTERFACE_HASH
will be as soon as possible incorporated into the firmware generators so at least.h
files can be generated that can be used to automatically update this hash when embedding in the firmware code of both atxmega and pico cores. ↩In the meantime, for backwards compatibility, when
R_VERSION
does not exist, or whenINTERFACE_HASH
is zero, this specific validation can be skipped. ↩