-
-
Notifications
You must be signed in to change notification settings - Fork 179
Add runtime warning if the MCU firmware is out of date, based on a hash of the firmware sources #768
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
| ): | ||
| pconfig = self._printer.lookup_object("configfile") | ||
| pconfig.runtime_warning( | ||
| f"MCU {self._name!r} firmware is out of date, it will still work but an update is recommended" |
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 should be more assertive. Something like: "it may not function as intended, please update as soon as possible."
| last_modified = max(file.stat().st_mtime for file in sources) | ||
| if hash_cache.is_file() and hash_cache.stat().st_mtime >= last_modified: | ||
| return hash_cache.read_text() | ||
| hash = hashlib.md5() |
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.
Use something modern. I know this isn't for security, but using a modern algorithm is also likely to be faster. Also apparently sometimes md5 isn't available because there are FIPS compliant versions of Python. Since we pretty much always recommend 64 bit platforms, Blake2b would be a good choice.
| def generate_code(self, options): | ||
| cleanbuild, self.toolstr = tool_versions(options.tools) | ||
| self.version = build_version(options.extra, cleanbuild) | ||
| self.sources = get_firmware_hash() |
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 this should be called sources_hash. Same goes in all locations where sources is really the hash of the source dir.
| and sources != self._printer.get_start_args().get("sources_hash") | ||
| ): | ||
| pconfig = self._printer.lookup_object("configfile") | ||
| pconfig.runtime_warning( |
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.
We do these runtime warnings a few times. The cost of always looking up pconfig is negligible. Hoist it out of the condtionals.
| self._get_status_info["app"] = app | ||
| self._get_status_info["mcu_version"] = version | ||
| self._get_status_info["mcu_build_versions"] = build_versions | ||
| self._get_status_info["mcu_sources"] = sources |
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.
mcu_sources_hash
| ) | ||
| app = msgparser.get_app_info() | ||
| version, build_versions = msgparser.get_version_info() | ||
| sources = msgparser.get_sources_hash() |
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.
sources_hash
| .config | ||
| .config.old | ||
| klippy/.version | ||
| klippy/.sources |
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 it actually makes more sense to place .sources at the root instead, since it's pulled in from both Klippy and buildcommands.
Fixes to the MCU firmware currently do not trigger the "firmware out of date" error. That error only compares the MCU API dictionaries.
This generates a hash of the firmware sources, includes that in the MCU firmware build, and then compares against the current firmware at runtime.
I also added
[danger_options] warn_on_mismatched_firmware_sources: Falseas an option to disable this check.Checklist