-
Notifications
You must be signed in to change notification settings - Fork 5
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
Adding records for tracking version of IOC and PandA firmware #150
base: main
Are you sure you want to change the base?
Conversation
TODO: Add / fix tests |
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.
This would be a great feature!
Co-authored-by: Eva Lott <[email protected]>
…into add-version-pvs
…cks-ioc into add-version-pvs
… block and add PVI info
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #150 +/- ##
==========================================
- Coverage 91.75% 91.61% -0.14%
==========================================
Files 8 8
Lines 1394 1419 +25
Branches 164 168 +4
==========================================
+ Hits 1279 1300 +21
- Misses 82 84 +2
- Partials 33 35 +2 ☔ View full report in Codecov by Sentry. |
if len(idn.split(":")) / 2 > len(list(firmware_versions.keys())): | ||
logging.error(f"Version string {idn} recieved from PandA could not be parsed!") | ||
else: | ||
for firmware_name in firmware_versions: | ||
pattern = re.compile( | ||
rf'{re.escape(firmware_name)}:\s*([^:]+?)(?=\s*\b(?:{"|".join(map(re.escape, firmware_versions))}):|$)' # noqa: E501 | ||
) | ||
if match := pattern.search(idn): | ||
firmware_versions[firmware_name] = match.group(1).strip() | ||
logging.info( | ||
f"{firmware_name} Version: {firmware_versions[firmware_name]}" | ||
) | ||
else: | ||
logging.warning(f"Failed to get {firmware_name} version information!") | ||
|
||
return { | ||
EpicsName(firmware_name.upper().replace(" ", "_")): version | ||
for firmware_name, version in firmware_versions.items() | ||
} |
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.
- Unnecessary line too long ignore.
- There's the race condition below which this change fixes.
- IDN provides an extra key value pair
"Some Other Version: 1"
in which case we have no way to tell what is in the previous version and what is in the key, but it didn't provide some keyrootfs
, so the check still passes.
- IDN provides an extra key value pair
if len(idn.split(":")) / 2 > len(list(firmware_versions.keys())): | |
logging.error(f"Version string {idn} recieved from PandA could not be parsed!") | |
else: | |
for firmware_name in firmware_versions: | |
pattern = re.compile( | |
rf'{re.escape(firmware_name)}:\s*([^:]+?)(?=\s*\b(?:{"|".join(map(re.escape, firmware_versions))}):|$)' # noqa: E501 | |
) | |
if match := pattern.search(idn): | |
firmware_versions[firmware_name] = match.group(1).strip() | |
logging.info( | |
f"{firmware_name} Version: {firmware_versions[firmware_name]}" | |
) | |
else: | |
logging.warning(f"Failed to get {firmware_name} version information!") | |
return { | |
EpicsName(firmware_name.upper().replace(" ", "_")): version | |
for firmware_name, version in firmware_versions.items() | |
} | |
if (sum(name in idn for name in firmware_versions) < idn.count(":")): | |
error_text = f"Version string {idn} received unexpected version numbers!" | |
logging.error(error_text) | |
return { | |
EpicsName(firmware_name.upper().replace(" ", "_")): error_text | |
for firmware_name in firmware_versions | |
} | |
for firmware_name in firmware_versions: | |
pattern = re.compile( | |
rf'{re.escape(firmware_name)}:\s*([^:]+?)(?=\s*\b(?:" | |
rf"{"|".join(map(re.escape, firmware_versions))}):|$)' | |
) | |
if match := pattern.search(idn): | |
firmware_versions[firmware_name] = match.group(1).strip() | |
logging.info( | |
f"{firmware_name} Version: {firmware_versions[firmware_name]}" | |
) | |
else: | |
logging.warning(f"Failed to get {firmware_name} version information!") | |
return { | |
EpicsName(firmware_name.upper().replace(" ", "_")): version | |
for firmware_name, version in firmware_versions.items() | |
} |
@@ -1824,6 +1865,40 @@ def create_block_records( | |||
|
|||
return record_dict | |||
|
|||
def create_version_records(self, fw_vers_dict: dict[EpicsName, str]): |
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.
Big preference for unshortened variable names which don't reference type in method definitions.
def create_version_records(self, fw_vers_dict: dict[EpicsName, str]): | |
def create_version_records(self, firmware_versions: dict[EpicsName, str]): |
async def get_panda_versions( | ||
client: AsyncioClient, | ||
) -> dict[EpicsName, str]: | ||
"""Function that gets version information from the PandA using the IDN command |
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.
Thought it might be good to link:
https://pandablocks-server.readthedocs.io/en/latest/commands.html#system-commands
here too to explain why we need the parsing logic.
Would be useful for checking the versions of everything on the floor without needing to get into the IOC console or logging into the IOC server.