Project Restructuring#4
Open
ehenry344 wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughRefactors build paths and scripts, reorganises .gitignore, increases LiftDrag ChangesProject build, model, world and docs updates
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
assets/models/betaloop_iris_with_standoffs/model.sdf (1)
645-696:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftFix Betaflight rotor→joint mapping:
<rotor id>is ignored for motor indexing (order matters), so the current jointName / turningDirection don’t match the documented BF QUADX motor order.
BetaflightPluginreads<rotor id>but applies commands asthis->rotors[i].cmd = pkt.motorSpeed[i]whereiis the<rotor>element parse order—so motorSpeed[0] drives the first<rotor>block’sjointName/turningDirection. Inassets/models/betaloop_iris_with_standoffs/model.sdfthe comment mapping (639-644) is therefore contradicted by the current rotor blocks (648-695).🐛 Proposed fix to align mapping with the documented BF motor order
<rotor id="0"> ... - <jointName>rotor_0_joint</jointName> - <turningDirection>ccw</turningDirection> + <jointName>rotor_3_joint</jointName> + <turningDirection>cw</turningDirection> <rotorVelocitySlowdownSim>1</rotorVelocitySlowdownSim> </rotor> <rotor id="1"> ... - <jointName>rotor_1_joint</jointName> - <turningDirection>ccw</turningDirection> + <jointName>rotor_0_joint</jointName> + <turningDirection>ccw</turningDirection> <rotorVelocitySlowdownSim>1</rotorVelocitySlowdownSim> </rotor> <rotor id="2"> ... - <jointName>rotor_2_joint</jointName> - <turningDirection>cw</turningDirection> + <jointName>rotor_1_joint</jointName> + <turningDirection>ccw</turningDirection> <rotorVelocitySlowdownSim>1</rotorVelocitySlowdownSim> </rotor> <rotor id="3"> ... - <jointName>rotor_3_joint</jointName> - <turningDirection>cw</turningDirection> + <jointName>rotor_2_joint</jointName> + <turningDirection>cw</turningDirection> <rotorVelocitySlowdownSim>1</rotorVelocitySlowdownSim> </rotor>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/models/betaloop_iris_with_standoffs/model.sdf` around lines 645 - 696, The BetaflightPlugin uses the parse order of <rotor> elements (not the <rotor id>) to map motorSpeed[i] to this->rotors[i].cmd, so reorder the <rotor> blocks in the SDF to match the documented Betaflight QUADX motor order and update each <jointName> and <turningDirection> values accordingly; keep the existing <rotor id> values if you want them for readability but ensure the element order of the <rotor> blocks matches the expected motorSpeed index mapping used by BetaflightPlugin (references: BetaflightPlugin, <rotor>, <jointName>, <turningDirection>, motorSpeed).
🧹 Nitpick comments (1)
scripts/build_plugin.sh (1)
18-20: 💤 Low value
SRC_DIRis unused — drop it.Static analysis (SC2034) flags
SRC_DIRas never referenced after the restructure; onlyPROJECT_DIRandBUILD_DIRare used.♻️ Proposed cleanup
PROJECT_DIR="$(dirname "$SCRIPT_DIR")" -SRC_DIR="$PROJECT_DIR/src" BUILD_DIR="$PROJECT_DIR/build"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build_plugin.sh` around lines 18 - 20, Remove the unused SRC_DIR variable assignment to satisfy SC2034: delete the line that sets SRC_DIR="$PROJECT_DIR/src" and leave only PROJECT_DIR and BUILD_DIR declarations (PROJECT_DIR="$(dirname "$SCRIPT_DIR")" and BUILD_DIR="$PROJECT_DIR/build"); also scan for any remaining references to SRC_DIR in the script and remove or replace them so no stale usage remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 7: The README currently uses the incorrect product name "MacOS"; update
the sentence in the README that reads "Ubuntu Linux and MacOS" to use the
correct capitalization and spelling "macOS" so it reads "Ubuntu Linux and macOS"
(locate the string in README.md).
---
Outside diff comments:
In `@assets/models/betaloop_iris_with_standoffs/model.sdf`:
- Around line 645-696: The BetaflightPlugin uses the parse order of <rotor>
elements (not the <rotor id>) to map motorSpeed[i] to this->rotors[i].cmd, so
reorder the <rotor> blocks in the SDF to match the documented Betaflight QUADX
motor order and update each <jointName> and <turningDirection> values
accordingly; keep the existing <rotor id> values if you want them for
readability but ensure the element order of the <rotor> blocks matches the
expected motorSpeed index mapping used by BetaflightPlugin (references:
BetaflightPlugin, <rotor>, <jointName>, <turningDirection>, motorSpeed).
---
Nitpick comments:
In `@scripts/build_plugin.sh`:
- Around line 18-20: Remove the unused SRC_DIR variable assignment to satisfy
SC2034: delete the line that sets SRC_DIR="$PROJECT_DIR/src" and leave only
PROJECT_DIR and BUILD_DIR declarations (PROJECT_DIR="$(dirname "$SCRIPT_DIR")"
and BUILD_DIR="$PROJECT_DIR/build"); also scan for any remaining references to
SRC_DIR in the script and remove or replace them so no stale usage remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30023ab4-5b0d-45b2-869b-e8abcd9a9fef
⛔ Files ignored due to path filters (12)
assets/models/battery_tattu_1300/mesh/tattu_1300.daeis excluded by!**/*.daeassets/models/betaloop_gimbal_2d/meshes/base_arm.daeis excluded by!**/*.daeassets/models/betaloop_gimbal_2d/meshes/base_main.daeis excluded by!**/*.daeassets/models/betaloop_gimbal_2d/meshes/tilt.daeis excluded by!**/*.daeassets/models/betaloop_iris_with_standoffs/meshes/iris.daeis excluded by!**/*.daeassets/models/betaloop_iris_with_standoffs/meshes/iris_prop_ccw.daeis excluded by!**/*.daeassets/models/betaloop_iris_with_standoffs/meshes/iris_prop_cw.daeis excluded by!**/*.daeassets/models/camera_runcam_micro_swift/meshes/runcam_micro_swift.daeis excluded by!**/*.daeassets/models/motor_emax_rs2306/mesh/motor_emax_rs2306.daeis excluded by!**/*.daeassets/models/quadcopter_model/meshes/iris.daeis excluded by!**/*.daeassets/models/quadcopter_model/meshes/iris_prop_ccw.daeis excluded by!**/*.daeassets/models/quadcopter_model/meshes/iris_prop_cw.daeis excluded by!**/*.dae
📒 Files selected for processing (38)
.gitignoreCMakeLists.txtGAZEBO_HARMONIC_MIGRATION.mdREADME.mdassets/models/README.mdassets/models/battery_tattu_1300/README.mdassets/models/betaloop_gimbal_2d/model.configassets/models/betaloop_gimbal_2d/model.sdfassets/models/betaloop_iris_with_standoffs/model.configassets/models/betaloop_iris_with_standoffs/model.sdfassets/models/betaloop_iris_with_standoffs_demo/model.configassets/models/betaloop_iris_with_standoffs_demo/model.sdfassets/models/camera_runcam_micro_swift/README.mdassets/models/camera_runcam_micro_swift/model.configassets/models/camera_runcam_micro_swift/model.sdfassets/models/motor_emax_rs2306/README.mdassets/models/quadcopter/model.configassets/models/quadcopter/model.sdfassets/models/quadcopter_attitude_control/model.configassets/models/quadcopter_attitude_control/model.sdfassets/models/quadcopter_model/model.configassets/models/quadcopter_model/model.sdfassets/models/quadcopter_model/model.tplassets/models/test_quad/model.configassets/models/test_quad/model.sdfassets/worlds/betaloop_iris_betaflight_demo.worldassets/worlds/betaloop_iris_betaflight_demo_gasstation.worldassets/worlds/betaloop_iris_betaflight_demo_harmonic.sdfassets/worlds/fixed.worldassets/worlds/quadcopter_test_harmonic.sdfassets/worlds/test_betaflight.sdfassets/worlds/test_harmonic.sdfplugins/README.mdscripts/build_plugin.shscripts/install_gazebo_harmonic.shsrc/BetaflightPlugin.ccsrc/BetaflightPlugin.hhstart_gazebo.sh
💤 Files with no reviewable changes (3)
- plugins/README.md
- GAZEBO_HARMONIC_MIGRATION.md
- start_gazebo.sh
| developing realistic and accurate aircraft models. | ||
| ## Supported Platforms | ||
|
|
||
| This project has native support and has been tested with Ubuntu Linux and MacOS. Windows is supported through WSL2. |
There was a problem hiding this comment.
Use the correct product name "macOS".
Apple's OS is written "macOS", not "MacOS".
✏️ Proposed fix
-This project has native support and has been tested with Ubuntu Linux and MacOS. Windows is supported through WSL2.
+This project has native support and has been tested with Ubuntu Linux and macOS. Windows is supported through WSL2.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| This project has native support and has been tested with Ubuntu Linux and MacOS. Windows is supported through WSL2. | |
| This project has native support and has been tested with Ubuntu Linux and macOS. Windows is supported through WSL2. |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: The operating system from Apple is written “macOS”.
Context: ...d has been tested with Ubuntu Linux and MacOS. Windows is supported through WSL2. ##...
(MAC_OS)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 7, The README currently uses the incorrect product name
"MacOS"; update the sentence in the README that reads "Ubuntu Linux and MacOS"
to use the correct capitalization and spelling "macOS" so it reads "Ubuntu Linux
and macOS" (locate the string in README.md).
There was a problem hiding this comment.
🧹 Nitpick comments (3)
assets/worlds/betaloop_iris_betaflight_demo_harmonic.sdf (1)
17-17: ⚡ Quick winConfirm the performance impact of the tighter physics timestep.
The
max_step_sizehas been reduced from 0.004 to 0.001 (4 ms to 1 ms), which increases the physics update rate by a factor of four. While this improves simulation accuracy and better aligns with the 1000 Hz IMU update rate, it will also increase computational load by approximately 4×.Ensure that the target hardware can maintain real-time performance with this configuration, particularly when combined with the modified LiftDrag parameters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/worlds/betaloop_iris_betaflight_demo_harmonic.sdf` at line 17, The physics timestep was tightened by changing the <max_step_size> value from 0.004 to 0.001 which will raise physics update rate ~4× and likely increase CPU load; verify real-time performance on the target hardware (run the full stack with the modified <max_step_size> and the updated LiftDrag parameters, measure CPU usage and real-time factor), and if the system cannot sustain real-time, either: raise <max_step_size> to a value that meets performance targets (e.g., back to 0.004 or an intermediate value), offload/optimize costly components, or document the hardware requirements and intended trade-offs in the world file comment. Ensure tests include IMU/physics-coupled scenarios so timing and stability are validated.assets/models/betaloop_iris_with_standoffs/model.sdf (2)
505-623: ⚡ Quick winVerify the impact of doubling the a0 lift parameter.
The
a0parameter has been increased from 0.025 to 0.05 across all eight LiftDrag plugin instances (two per rotor for four rotors). This change doubles the zero-angle-of-attack lift coefficient, which will significantly alter the aircraft's aerodynamic behaviour and likely reduce the required hover throttle by approximately half.Ensure this tuning change has been tested in simulation and produces the desired flight characteristics.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/models/betaloop_iris_with_standoffs/model.sdf` around lines 505 - 623, The a0 lift parameter was doubled from 0.025 to 0.05 across all LiftDrag plugin instances, which will significantly change aircraft lift; locate every gz::sim::systems::LiftDrag plugin block in model.sdf for link_name entries rotor_0, rotor_1, rotor_2, and rotor_3 (each rotor has two LiftDrag blocks) and either revert a0 back to 0.025 or run controlled simulation tests (hover throttle, trim, and stability) to validate the new value; if the new behavior is undesired, set a0=0.025 in those plugin blocks and add a short comment documenting the tested values and test results.
501-501: ⚡ Quick winUpdate the comment to reflect the new a0 value.
The comment still references
a0=0.025and~42% hover throttle, but the actuala0value has been changed to 0.05. Please update this comment to document the new tuning parameters and expected hover throttle percentage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/models/betaloop_iris_with_standoffs/model.sdf` at line 501, The inline comment that currently says "a0=0.025 tuned for ~42% hover throttle" is stale; change it to state the new tuning parameter a0=0.05 and update the expected hover throttle percentage accordingly (recompute or measure the hover throttle with a0=0.05 and replace "~42%" with the observed value). Locate the comment near the a0 parameter in the model.sdf and ensure the text documents the actual a0 value and the measured/expected hover throttle percentage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@assets/models/betaloop_iris_with_standoffs/model.sdf`:
- Around line 505-623: The a0 lift parameter was doubled from 0.025 to 0.05
across all LiftDrag plugin instances, which will significantly change aircraft
lift; locate every gz::sim::systems::LiftDrag plugin block in model.sdf for
link_name entries rotor_0, rotor_1, rotor_2, and rotor_3 (each rotor has two
LiftDrag blocks) and either revert a0 back to 0.025 or run controlled simulation
tests (hover throttle, trim, and stability) to validate the new value; if the
new behavior is undesired, set a0=0.025 in those plugin blocks and add a short
comment documenting the tested values and test results.
- Line 501: The inline comment that currently says "a0=0.025 tuned for ~42%
hover throttle" is stale; change it to state the new tuning parameter a0=0.05
and update the expected hover throttle percentage accordingly (recompute or
measure the hover throttle with a0=0.05 and replace "~42%" with the observed
value). Locate the comment near the a0 parameter in the model.sdf and ensure the
text documents the actual a0 value and the measured/expected hover throttle
percentage.
In `@assets/worlds/betaloop_iris_betaflight_demo_harmonic.sdf`:
- Line 17: The physics timestep was tightened by changing the <max_step_size>
value from 0.004 to 0.001 which will raise physics update rate ~4× and likely
increase CPU load; verify real-time performance on the target hardware (run the
full stack with the modified <max_step_size> and the updated LiftDrag
parameters, measure CPU usage and real-time factor), and if the system cannot
sustain real-time, either: raise <max_step_size> to a value that meets
performance targets (e.g., back to 0.004 or an intermediate value),
offload/optimize costly components, or document the hardware requirements and
intended trade-offs in the world file comment. Ensure tests include
IMU/physics-coupled scenarios so timing and stability are validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f8a8a9f-50c3-4194-b600-18d099661cb9
📒 Files selected for processing (2)
assets/models/betaloop_iris_with_standoffs/model.sdfassets/worlds/betaloop_iris_betaflight_demo_harmonic.sdf
Author
|
@blckmn Please review when you get a chance. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request restructures the project to improve workflow. Changes include default build output rerouting, README update and more.
Summary by CodeRabbit
Improvements
Documentation
Chores