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 flag to platform storage to capture UserConfig platform. #339

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

axlan
Copy link
Collaborator

@axlan axlan commented Oct 12, 2024

No description provided.

@axlan axlan self-assigned this Oct 12, 2024

/**
* @ref DataType::USER_CONFIG flag that does not specify what platform the
* configuration corresponds to.
Copy link
Collaborator

@adamshapiro0 adamshapiro0 Oct 15, 2024

Choose a reason for hiding this comment

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

This sentence reads a little oddly. "a flag that does not specify something" takes a second to parse. Maybe "indicating that the platform type is not known" or something like that.

We might want to clarify all of this a little more, maybe a combination of the documentation for these constants and some more detail in the flags field documentation itself. I think flags would be a better place to say how that field is defined for DataType::USER_CONFIG and the other data types. Is flags a bitmask? Or maybe it's a generic integer and a subset its bits correspond to a platform type flag enum, including 0 if platform type is unknown? Or is it a full uint8_t enum with a lot of extra space for the future, but only for platform type? That documentation can @ref these constants.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The use case of this flag is two-fold:

  1. It indicates the behavior for backward compatibility.
  2. It allows the host FE client user to not need to set flags at all, and it will work by default (the size check is sufficient to catch platform type errors).

The platform should always be known, you implicitly need to know it to form an acceptable UserConfig data blob, so I didn't want to word it that way.

The purpose of this is really only for identifying binaries loaded from disk that aren't associated with a device. Ideally, a customer should never have to set or check this.

Copy link
Collaborator

@adamshapiro0 adamshapiro0 left a comment

Choose a reason for hiding this comment

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

Structurally looks good, but I think we should add more explanation to the flags documentation itself. Right now it's a little unclear how it's defined. "DataType-specific flags" doesn't really tell you what to do with it if you are looking at the doxygen HTML, so you end up having to stumble across the flag constants yourself, and then it's still not 100% clear.

@axlan axlan merged commit b8ca122 into master Oct 15, 2024
14 checks passed
@axlan axlan deleted the storage-flags branch October 15, 2024 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants