Firmware Versioning#185
Conversation
todo: automatically initialize version as CAN packet that can be reported to telemetry
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive firmware versioning system, enabling the embedding of semantic version information directly into the firmware binaries. This standardization facilitates better tracking of releases, simplifies debugging processes, and ensures consistent version identification across various deployments. The changes establish a foundational mechanism for managing firmware versions programmatically within the build system. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a firmware versioning mechanism by embedding a version structure into a dedicated flash section and updating the build system to populate it. This is a solid foundation for version tracking. My review includes suggestions to improve the robustness of the build script's version parsing and to refactor the C code to make the version information accessible to other firmware modules, which will be crucial for features like reporting the version over a CAN bus.
| typedef struct { | ||
| char magic[4]; | ||
| uint8_t major; | ||
| uint8_t minor; | ||
| uint16_t patch; | ||
| char build_date[12]; | ||
| } AppVersion_t; |
There was a problem hiding this comment.
The AppVersion_t struct is defined locally in this C file. To make the version information accessible to other parts of the firmware (e.g., for reporting over CAN or USB), this structure definition should be moved to the public header file fw_version.h. This makes the versioning data structure available across the project, which is essential for using this information elsewhere.
| @@ -0,0 +1,4 @@ | |||
| #ifndef FW_VERSION_H | |||
| #define FW_VERSION_H | |||
|
|
|||
There was a problem hiding this comment.
This header file is currently empty. It should contain the definition for AppVersion_t and an extern declaration for the global g_app_version variable. This will allow other modules to include this header and access the firmware version information.
| #include <stdint.h> | |
| typedef struct { | |
| char magic[4]; | |
| uint8_t major; | |
| uint8_t minor; | |
| uint16_t patch; | |
| char build_date[12]; | |
| } AppVersion_t; | |
| extern const AppVersion_t g_app_version; |
| final_defines.append("APP_VERS_MAJOR=" + version.split(".")[0]) | ||
| final_defines.append("APP_VERS_MINOR=" + version.split(".")[1]) | ||
| final_defines.append("APP_VERS_PATCH=" + version.split(".")[2]) |
There was a problem hiding this comment.
The current implementation for parsing the version string is not robust. It assumes the version string will always be in the format "major.minor.patch". If a user provides a version string with fewer than two dots (e.g., "1.0"), this code will raise an IndexError and cause the build to fail. It's better to validate the version string format to provide a clear error message and prevent build failures.
version_parts = version.split(".")
if len(version_parts) != 3:
fail("Version string must be in the format 'major.minor.patch', but got '{}'".format(version))
final_defines.append("APP_VERS_MAJOR=" + version_parts[0])
final_defines.append("APP_VERS_MINOR=" + version_parts[1])
final_defines.append("APP_VERS_PATCH=" + version_parts[2])
| @@ -0,0 +1,17 @@ | |||
| #include <stdint.h> | |||
There was a problem hiding this comment.
It is a common best practice for a source file to include its own header file. This ensures that the header is self-contained and that its declarations are consistent with the definitions in the source file. This also simplifies dependency management, as including fw_version.h will transitively include <stdint.h> (based on my other suggestion).
| #include <stdint.h> | |
| #include "fw_version.h" |
1 participant
TODO: