Dolphin rc f722#1103
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds DolphinRC to the manufacturer table and introduces a new board configuration header for DOLPHINRC_F722 with MCU/board IDs, enabled peripherals, pin/timer mappings, and meter/blackbox defaults. ChangesDolphinRC F722 Board Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Please read the Manufacturer Design Guidelines as clearly stated in the template. You appear to have not done so. Also, please see my reply on your issue opened earlier about joining the Discord server for schematic review. |
Removed unused motor pin definitions for MOTOR5 to MOTOR8.
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
🤖 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 `@configs/DolphinRC_F722/config.h`:
- Line 27: The MANUFACTURER_ID macro currently uses the literal DolphinRC;
update the definition of MANUFACTURER_ID to use the registered manufacturer code
"DOLP" instead (replace the identifier DolphinRC with the string/code DOLP in
the `#define` MANUFACTURER_ID line) so metadata matches the registered
manufacturer ID.
- Around line 80-84: TIMER_PIN_MAP entries reference undefined macros
MOTOR5_PIN, MOTOR6_PIN, MOTOR7_PIN, and MOTOR8_PIN (and optionally
LED_STRIP_PIN); fix by either defining those pin macros in this config (e.g.,
add MOTOR5_PIN..MOTOR8_PIN definitions matching the board wiring) or
remove/replace the TIMER_PIN_MAP lines with the correct existing pin macros;
update the TIMER_PIN_MAP(...) calls (lines with TIMER_PIN_MAP(4..7,
MOTOR5_PIN..MOTOR8_PIN)) to use the newly defined MOTORx_PIN symbols or the
appropriate existing pin identifiers so compilation succeeds.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b1a7202-f5c9-42b6-a945-1aef82bdc152
📒 Files selected for processing (2)
Manufacturers.mdconfigs/DolphinRC_F722/config.h
Updated manufacturer ID and pin mappings for motors and LEDs.
| #define ADC1_DMA_OPT 0 | ||
| #define TIMUP8_DMA_OPT 0 | ||
| #define TIMUP3_DMA_OPT 0 |
There was a problem hiding this comment.
| #define ADC1_DMA_OPT 0 | |
| #define TIMUP8_DMA_OPT 0 | |
| #define TIMUP3_DMA_OPT 0 | |
| #define ADC_INSTANCE ADC3 | |
| #define ADC3_DMA_OPT 1 |
AFAIK TIMUPx_DMA_OPT isn't used for F7. Bitbanged DShot is default anyway.
ADC3 required due to DMA conflicts with TIM8 on ADC1, PC0 and PC1 support all three ADCs.
| #define DEFAULT_CURRENT_METER_SOURCE CURRENT_METER_ADC | ||
| #define DEFAULT_VOLTAGE_METER_SOURCE VOLTAGE_METER_ADC | ||
| #define DEFAULT_VOLTAGE_METER_SCALE_DEFAULT 110 | ||
| #define DEFAULT_CURRENT_METER_SCALE 250 |
There was a problem hiding this comment.
Is this FC going to be part of a stack?
…into DolphinRC_F722
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
♻️ Duplicate comments (1)
configs/DolphinRC_F722/config.h (1)
25-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDirectory name must be uppercase to match board name.
The board name macro is correctly uppercase (
DOLPHINRC_F722), but the directory nameconfigs/DolphinRC_F722/remains mixed-case. Per past review feedback, the directory should be renamed toconfigs/DOLPHINRC_F722/to maintain consistency with Betaflight naming conventions.🤖 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 `@configs/DolphinRC_F722/config.h` at line 25, The directory name currently uses mixed case; rename the directory named "DolphinRC_F722" to "DOLPHINRC_F722" so it exactly matches the BOARD_NAME macro (DOLPHINRC_F722) defined in config.h; update any references to that directory in build configs or includes to use the new uppercase directory name.
🤖 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 `@Manufacturers.md`:
- Line 47: The DOLP manufacturer entry is out of alphabetical order; remove the
existing line "|DOLP|DolphinRC|https://www.dolphinrc.com/|" from after DYST and
insert that exact entry between the DIAT and DRCL entries so the list remains
alphabetized (respecting the special-top-4 exception).
---
Duplicate comments:
In `@configs/DolphinRC_F722/config.h`:
- Line 25: The directory name currently uses mixed case; rename the directory
named "DolphinRC_F722" to "DOLPHINRC_F722" so it exactly matches the BOARD_NAME
macro (DOLPHINRC_F722) defined in config.h; update any references to that
directory in build configs or includes to use the new uppercase directory name.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1143fa97-f9d4-4b6d-98e3-f98408f51a8e
📒 Files selected for processing (2)
Manufacturers.mdconfigs/DolphinRC_F722/config.h
| |DRNR|Shenzhen Zhimu Technology Co., Ltd|https://www.droneer.com| | ||
| |DRRC|DroidRC|https://github.com/xiaoxiabub| | ||
| |DYST|DongYang Smart Technology Co., Ltd (dys)|http://www.dys.hk/| | ||
| |DOLP|DolphinRC|https://www.dolphinrc.com/| |
There was a problem hiding this comment.
Please use the ISO basic Latin alphabet order to sort the list (on the first column). Not just on the first letter, but on the entire 4 character manufacturer ID.
Co-authored-by: Osiris Inferi <github@flut.nl.eu.org>
Removed duplicate entry for DolphinRC in the Manufacturers list.
osirisinferi
left a comment
There was a problem hiding this comment.
Some last (I think/hope) requests for changes from my part.
Co-authored-by: Osiris Inferi <github@flut.nl.eu.org>
Pull-Request requirements
Mandatory Review for All New Flight Controllers
Hardware Compliance Requirements
These measures help maintain high standards and ensure compatibility within the Betaflight ecosystem.
If you have any questions or need guidance, feel free to reach out to the Betaflight development team.
Housekeeping
master.Checklist (✓/✕, or y/n)
Summary by CodeRabbit
New Features
Documentation