Add ACCT/ACCTON_GFH7 board configuration - #1145
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:
WalkthroughThe pull request registers Accton Technology Corporation and adds the ACCTON_GFH7 STM32H753 Betaflight target configuration, including hardware mappings, enabled peripherals, timers, defaults, and output inversion settings. ChangesNew board target support
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Config does not match schematic. The same comments for the F4 config apply here. |
2.add OSD function and hearter
There was a problem hiding this comment.
🧹 Nitpick comments (2)
configs/ACCTON_GFH7/config.h (2)
96-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix typo in comment.
♻️ Proposed refactor
`#define` PINIO1_PIN PC12 // 12V_EN Switch -#define PINIO2_PIN PE3 // hearter switch pin +#define PINIO2_PIN PE3 // heater switch pin🤖 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/ACCTON_GFH7/config.h` around lines 96 - 97, Correct the spelling in the comment for PINIO2_PIN, changing “hearter” to “heater” while leaving the macro definition and PINIO1_PIN unchanged.
126-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDefine a user-friendly name for
PINIO2_BOX.Since
PINIO1_BOXhas a custom name defined viaBOX_USER1_NAME, consider defining a corresponding name forPINIO2_BOX(e.g.,BOX_USER2_NAME) so it displays clearly in the Configurator instead of defaulting to "USER2".♻️ Proposed refactor
`#define` BOX_USER1_NAME "VTX SWITCH" +#define BOX_USER2_NAME "HEATER SWITCH" `#define` PINIO1_BOX 40 `#define` PINIO2_BOX 41🤖 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/ACCTON_GFH7/config.h` around lines 126 - 128, Define a corresponding user-friendly name for PINIO2_BOX by adding the appropriate BOX_USER2_NAME configuration alongside BOX_USER1_NAME, so the second user mode displays a descriptive label instead of the default “USER2”.
🤖 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 `@configs/ACCTON_GFH7/config.h`:
- Around line 96-97: Correct the spelling in the comment for PINIO2_PIN,
changing “hearter” to “heater” while leaving the macro definition and PINIO1_PIN
unchanged.
- Around line 126-128: Define a corresponding user-friendly name for PINIO2_BOX
by adding the appropriate BOX_USER2_NAME configuration alongside BOX_USER1_NAME,
so the second user mode displays a descriptive label instead of the default
“USER2”.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6e63494d-286d-40f1-a970-f5aff7208487
📒 Files selected for processing (1)
configs/ACCTON_GFH7/config.h
|
Modified the H7 config settings based on the F4 config settings. |
1.add servo 2.pinio setting 3.timer_pin_mapping 4.ADC, BARO and MAG set 5.SYSTEM_HSE_MHZ modified
|
@ot0tot Modified the H7 config settings based on H7 new schematic.Please review it .Thanks. |
1. removed second IMU(LSM6DSK320X) defined 2. removed OSD defined 3. removed LED1_PIN defined 4. removed piniox_config defined 5. modified LED_STRIO_PIN configuration 6. modified ADC1_DMA_OPT configuration 7. modified MAG_I2C configuration 8. modified DSHOT_BITBANG configuration 9. add DEFAULT_VOLTAGE_METER_SCALE 10. add DEFAULT_GYRO_TO_USE
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
configs/ACCTON_GFH7/config.h (1)
29-36: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnable the LSM6DSK320X driver for gyro 2.
GYRO_2_SPI_INSTANCE SPI2leavesGYRO_2enabled with no matching sensor macro, because onlyUSE_ACC_SPI_ICM42688P/USE_GYRO_SPI_ICM42688Pare defined here. AddUSE_ACCGYRO_LSM6DSK320Xso the second IMU is built and can be configured separately from gyro 1.Proposed fix
`#define` USE_ACC_SPI_ICM42688P +#define USE_ACCGYRO_LSM6DSK320X `#define` USE_GYRO🤖 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/ACCTON_GFH7/config.h` around lines 29 - 36, Add the USE_ACCGYRO_LSM6DSK320X feature macro alongside the existing sensor definitions in config.h so GYRO_2 on SPI2 has a matching LSM6DSK320X driver while preserving the existing ICM42688P configuration for gyro 1.
🤖 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/ACCTON_GFH7/config.h`:
- Around line 98-100: Correct the hardware name spelling in the BOX_USER2_NAME
label and the adjacent PINIO2_PIN comment, changing “HEARTER”/“hearter” to
“HEATER”/“heater” while leaving the pin definitions unchanged.
---
Outside diff comments:
In `@configs/ACCTON_GFH7/config.h`:
- Around line 29-36: Add the USE_ACCGYRO_LSM6DSK320X feature macro alongside the
existing sensor definitions in config.h so GYRO_2 on SPI2 has a matching
LSM6DSK320X driver while preserving the existing ICM42688P configuration for
gyro 1.
🪄 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 Plus
Run ID: 4df359a1-c905-4afb-bd8f-5365fb2dda11
📒 Files selected for processing (1)
configs/ACCTON_GFH7/config.h
|
@ot0tot Modified the H7 config settings based on your suggestions.Please review it .Thanks. |
This comment was marked as outdated.
This comment was marked as outdated.
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
🤖 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/ACCT/ACCTON_GFH7/config.h`:
- Around line 29-36: Add the supported LSM6DSK320X accelerometer/gyro driver
configuration macro alongside the existing gyro driver defines in the
ACCTON_GFH7 configuration, while preserving the current ICM42688P settings and
second-gyro pin/SPI declarations.
🪄 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 Plus
Run ID: 9e666729-5ea6-4429-ad70-6bc76d6785c0
📒 Files selected for processing (1)
configs/ACCT/ACCTON_GFH7/config.h
|
@osirisinferi Modified the H7 config settings based on your suggestions.Please review it .Thanks. |
RunnyCow
changed the title
9ceed43 to
de52426
Compare
…into Branch_H7_master # Conflicts: # Manufacturers.md
|
@coderabbitai, regarding this #1145 (review) , is it resolved? any other open items to resolve? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR metadata ==='
gh pr view 1145 --repo betaflight/config --json number,state,isDraft,mergeable,reviewDecision,statusCheckRollup,files
echo
echo '=== Referenced review ==='
gh api repos/betaflight/config/pulls/1145/reviews/4795114658 \
--jq '{id,user:.user.login,state,body,submitted_at,commit_id}'
echo
echo '=== Review threads ==='
gh api graphql -F owner='betaflight' -F name='config' -F number=1145 -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
originalLine
comments(first: 20) {
nodes { author { login } body createdAt url }
}
}
}
}
}
}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] |
{isResolved,path,line,originalLine,comments:[.comments.nodes[] | {author:.author.login,body,url}]}'
echo
echo '=== Changed configuration ==='
ast-grep outline configs/ACCT/ACCTON_GFH7/config.h --items all
nl -ba configs/ACCT/ACCTON_GFH7/config.h | sed -n '1,220p'
echo
rg -n -C2 '^\|\s*ACCT\s*\|' Manufacturers.mdLength of output: 22553 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Sensor and feature definitions ==='
sed -n '20,115p' configs/ACCT/ACCTON_GFH7/config.h
echo
echo '=== Timer, ADC, video, and defaults ==='
sed -n '115,180p' configs/ACCT/ACCTON_GFH7/config.h
echo
echo '=== Relevant current declarations ==='
rg -n -C2 \
'USE_ACCGYRO_LSM6DSK320X|USE_(ACC|GYRO)_SPI_ICM42688P|GYRO_2_|DEFAULT_GYRO_TO_USE|LED[0-9]?_PIN|MAX7456_SPI_INSTANCE|ADC1_DMA_OPT' \
configs/ACCT/ACCTON_GFH7/config.h
echo
echo '=== Failed target-check log ==='
gh run view 32233713853 --repo betaflight/config --log-failedLength of output: 6472
Open items:
The directory migration requirement is resolved. 🧠 Learnings usedYou are interacting with an AI system. |
RunnyCow
left a comment
RunnyCow
left a comment
There was a problem hiding this comment.
Thanks for the suggestion. I have updated the code accordingly.
|
@RunnyCow , your last commit only addressed the spelling error and skipped the prior concerns in #1145 (comment) if i'm not mistaken. |
2、Removed MAX7456_SPI_INSTANCE SPI1 , because the hardware is absent
|
@nerdCopter I have resolved all the issues; please review it again.Thank you. |
| #define SYSTEM_HSE_MHZ 16 | ||
| #define GYRO_1_SPI_INSTANCE SPI4 | ||
| #define SDIO_DEVICE SDIODEV_2 | ||
| #define SDIO_USE_4BIT 1 No newline at end of file |
|
|
||
| #pragma once | ||
|
|
||
| #define FC_TARGET_MCU STM32H753 |
There was a problem hiding this comment.
CI: Makefile:258: *** No TARGET_MCU specified. Is the target.mk valid for STM32H753?. Stop.
|
This PR needs betaflight/betaflight#15654 |
6 participants
Description
Add support for the new flight controller: ACCT/ACCTON_GFH7
Hardware Specifications
Test Plan
Checklist (✓/✕, or y/n)
Summary by CodeRabbit