-
Notifications
You must be signed in to change notification settings - Fork 14.9k
copilot: Review instructions for uorb msgs #26169
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
| # Review Guidelines for PX4 uORB Message Definitions | ||
|
|
||
| You are an expert embedded software engineer specializing in the PX4 Autopilot system. | ||
| Your task is to review `.msg` files to ensure they comply with the official [PX4 uORB Documentation Standard](https://docs.px4.io/main/en/uorb/uorb_documentation). |
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.
You might want to use a relative path
|
|
||
| ## 2. Field Documentation Standards | ||
|
|
||
| - **Inline Comments:** Every field and constant should have a comment on the same line, separated by exactly one space. |
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.
every field no exceptions?
|
|
||
| ## 5. Deprecation & Semantic Changes | ||
|
|
||
| - **Field Deprecation:** Do not delete fields. Rename them with a `DEPRECATED_` prefix or add a `# DEPRECATED` comment to maintain binary compatibility. |
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.
Do not delete fields
Since when? We need to be able to extend or remove uORB messages
Rename them with a
DEPRECATED_prefix
Renaming breaks compatibility
| ## 5. Deprecation & Semantic Changes | ||
|
|
||
| - **Field Deprecation:** Do not delete fields. Rename them with a `DEPRECATED_` prefix or add a `# DEPRECATED` comment to maintain binary compatibility. | ||
| - **Semantic Changes:** If a unit changes (e.g., degrees to radians), the field **must** be renamed to prevent silent failures in subscribers. |
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.
Really? I'm not so sure about this, shouldn't it be a versioned message if we really care about backwards compatibility?
This uses gemini to write some copilot review instructions based on https://docs.px4.io/main/en/uorb/uorb_documentation
The idea is that copilot should do basic review only any PR that includes msg.
I have not checked this carefully - was hoping to iterate.