Repository navigation
Add board support fo Droneerx6 - #33573
Conversation
|
note to reviewed...this is a redo of a previously reviewed PR #33313 that had requested changes.... |
|
local image file : libraries/AP_HAL_ChibiOS/hwdef/DroneerX6/DroneerX6_Pinout.png |
Hwurzburg
left a comment
There was a problem hiding this comment.
Preliminary
Images are not linked correctly, and this is actually what's failing CI right now (the check-merge-commits job runs a README-image validator, and it's erroring with README.md for new board DroneerX6 contains no local image references). Both DroneerX6_V1.png and DroneerX6_Pinout.png are committed in the hwdef dir, but the README links them as absolute GitHub blob URLs pointing at your fork:

...

These need to be relative links to the files you already committed, e.g.:


Fix this and CI should go green. I won't do a full review until it is, but I went ahead and looked at the pinout image anyway since it's sitting right there in the PR — findings below assume you fix the links.
Also, squash the "hwdef:fix DroneerX6 README.md" commit into the "hwdef: Add support for DroneerX6" commit — one commit per subsystem, not a fixup tacked on after.
README.md
Intro / Specifications
- Add physical dimensions/weight to the Features list — required by the template and currently missing entirely.
Pinout / naming consistency
- The GPIOs table uses a third naming scheme (
M1-M16) that matches neither the pinout image (which just prints bare numbers underMAIN/AUX) nor your own PWM Output section text (MAIN1-MAIN8,AUX1-AUX6,TIM12_CH1/TIM12_CH2). Pick one naming scheme and use it everywhere — I'd suggest matching the PWM Output section's names since that's the most descriptive. - TIM12_CH1/TIM12_CH2 (outputs 15/16) are described alongside "AUX1 to AUX6" as if they're on the same connector, but per the pinout image they're actually broken out on the separate
AD&IOconnector, not the 6-pinAUXconnector. Worth a sentence clarifying that, so someone doesn't go looking for output 15/16 on the AUX connector and come up short. - The pinout image's
SPIconnector (on the carrier board, pinsSPI6_SCK/SPI6_MISO/SPI6_MOSI/EXT1_CS/etc.) is not mentioned anywhere in the README — and per the hwdef (see below) it's actually disabled in firmware. If it's staying disabled, say so in the README (or in a comment near the pinout) so users don't wire something to a connector that does nothing.
RC Input
- CRSF/ELRS is named as one of the protocols needing a full UART, but the
SERIAL6_OPTIONSvalue for it (0) is never given — only FPort (15) and SRXL2 (4) are. Add it. - Add a line stating CRSF/ELRS needs a DMA-capable UART for reliable operation (UART4/SERIAL6 does have DMA here, so it's a correct recommendation — just say so).
- `SERIAL6_PROTOCOL` should be set to "23".
- FPort would require `SERIAL6_OPTIONS` be set to "15".
- CRSF/ELRS would require `SERIAL6_OPTIONS` be set to "0". CRSF/ELRS needs a UART with DMA for reliable operation; UART4 (SERIAL6) has DMA.
- SRXL2 would require `SERIAL6_OPTIONS` to be set to "4" (half-duplex) and connects only the TX pin.
UART Mapping
- Telem2 (UART5) and Telem3 (USART2) both have flow control pins per the hwdef and the pinout image — note that in the UART Mapping section (the template calls this out explicitly).
- Telem1/UART7 is defined in hwdef.dat with RTS/CTS pins (PF8/PE10), but the pinout image's
TELEM1connector only shows 4 pins (GND/RX/TX/5V) — no flow-control pads. Please verify: if UART7's RTS/CTS aren't actually wired to anything, drop those two lines from hwdef.dat. - SERIAL5 (Telem3/USART2) is documented as defaulting to MAVLink2, but the hwdef line meant to set that is commented out (see hwdef.dat finding below) — so as currently written the firmware will actually default it to NONE, not MAVLink2. Fix the hwdef and the README will be correct as-is.
- SERIAL8 (OTG2, labelled "OTG-SLCAN") is documented as defaulting to SLCAN, but there's no
define HAL_OTG2_PROTOCOL SerialProtocol_SLCANoverride in hwdef.dat. Since this board has CAN1/CAN2 defined, chibios_hwdef.py's generator will actually default OTG2 to MAVLink2 @ 115200, not SLCAN. Either add the override or fix the README.
Battery Monitoring
BATT_MONITOR 8is set in defaults.parm — this belongs in hwdef.dat instead, not defaults.parm.
hwdef.dat
DMA_PRIORITY TIM5* TIM4* SPI1* SPI2* SPI3* SDMMC* USART6* ADC* UART* USART*— the IMU SPI buses (SPI1/SPI2/SPI3) are listed after the motor-output timers (TIM5/TIM4). This is backwards: IMU SPI DMA should out-prioritize the PWM timers so motor output DMA doesn't contend with gyro sampling. Please reorder so SPI1*/SPI2*/SPI3* come first.
DMA_PRIORITY SPI1* SPI2* SPI3* TIM5* TIM4* SDMMC* USART6* ADC* UART* USART*
- Line 60:
#define DEFAULT_SERIAL5_PROTOCOL SerialProtocol_MAVLink2— this is written with a leading#, which makes it a comment in hwdef.dat syntax, not an activedefinedirective. As-is this line does nothing, and SERIAL5 (Telem3) will fall back to the USER/NONE default instead of MAVLink2 as the README claims.
define DEFAULT_SERIAL5_PROTOCOL SerialProtocol_MAVLink2
- The GPS & SAFETY connector in the pinout image shows a
TIM4_CH1pad, but noTIM4_CH1pin is defined anywhere in hwdef.dat. Please verify what this pad is for and add the pin definition (or drop it from the image if it's not actually connected/used). - SPI6 (external SPI connector) is fully commented out ("disabled to save DMA channels") yet the connector and its pins are printed on the pinout image as if live. If it's staying disabled, note that in the README per above; otherwise re-enable it.
defaults.parm
- Move
BATT_MONITOR 8to hwdef.dat (see README finding above).
RE-request my review when these changes are made.
|
TIM4_CH1 --> BUZZER |
Hwurzburg
left a comment
There was a problem hiding this comment.
Thanks for the updates — most of the previous round's items are fixed (relative image links, dimensions/weight, TIM12_CH1/CH2 AD&IO note, SPI6 disabled note, CRSF/ELRS options, Telem2/Telem3 RTS/CTS note, Telem1 RTS/CTS pins dropped, SERIAL5 MAVLink2 define, SERIAL8 protocol doc, BATT_MONITOR moved to hwdef.dat, DMA_PRIORITY reorder). Two items are still outstanding:
1. TIM4_CH1 pad still undefined, and labelling pads by timer/channel isn't user friendly. The GPS & SAFETY connector in the pinout image shows a TIM4_CH1 pad, but hwdef.dat still only defines TIM4_CH2/TIM4_CH3 — no TIM4_CH1 pin was added, and there's no note explaining it's unused. More generally, labelling TIMx_CHy in the pinout is not user friendly — please label pads with their functions instead (e.g. what the pin is actually for), and either add the missing pin definition or drop it from the image if it's not connected.
2. Fixup commit still not squashed. I asked for "hwdef:fix DroneerX6 README.md" to be squashed into "hwdef: Add support for DroneerX6" — one commit per subsystem. Instead there are now two separate README fixup commits on top of the original, so this is still outstanding (and the history has grown, not shrunk). Please squash all of these into a single commit per subsystem.
Re-request my review when these are addressed.
Hwurzburg
left a comment
There was a problem hiding this comment.
Thanks. The buzzer pad relabel and the commit squash are both done. While redoing the image, though, the AUX7/AUX8 pads on AD&IO were relabelled A7/A8, and the README still calls them TIM12_CH1/TIM12_CH2. A full pass over the current head turned up the following:
hwdef.dat
The motor timers share DMA streams with SPI buses, including an IMU. In the generated hwdef.h, TIM4_UP (DShot for AUX5-6) is on DMA2 stream 5 together with SPI3_RX, which is IMU icm45686_3, and SPI5_RX (FRAM). TIM5_UP (AUX1-4) is on DMA2 stream 1 with SPI5_TX and all the UART TX streams. While an SPI transfer holds the stream, the DShot frame for that group can be dropped, and the IMU sample timing gets pushed around by motor output. All 16 streams are in use, so DMA_NOSHARE alone fails to build (Missing required DMA for USART6_TX). I built this combination and it gives the IMU SPI buses and both motor timers their own streams:
DMA_NOSHARE SPI1* SPI2* SPI3* TIM*
and give up RX DMA on the two GPS ports and the debug port, none of which need it:
PB7 USART1_RX USART1 NODMA
PE0 UART8_RX UART8 NODMA
PD9 USART3_RX USART3 NODMA
Then update the README's DMA column to match (see the UART Mapping item below).
HAL_HEATER_MAG_OFFSET_RM3100 is defined with Vector3f(0,0,0), and the HAL_HEATER_MAG_OFFSET line that would use it is commented out, so the define does nothing. Either measure the real offset and enable it, or remove both lines.
Remove the leftover commented-out lines copied from the 6X: #PF8/#PE10 UART7 RTS/CTS, the SPI4 pin block, #define HAL_HEATER_MAG_OFFSET, #ROMFS io_firmware_dshot.bin ...highpolh.bin/#define HAL_WITH_IO_MCU_DSHOT 1, and #V6X. Also fix the comments that name parts this board doesn't have (MS5611 on I2C3, DRDY1_BMP388).
README.md
Pinout / naming
The image now labels outputs 15/16 as A7/A8 on the AD&IO connector, but the PWM Output section and GPIO table still call them TIM12_CH1/TIM12_CH2. Use the image labels.
Still open from my first review: the GPIO table names outputs M1-M16, while the case labels them MAIN 1-8 and AUX 1-6. Use MAIN1-MAIN8, AUX1-AUX6, A7, A8 so the README matches what the user sees. The table's header row is also malformed (| IO Pins | | FMU Pins | followed by a second header row); make it a single | Pin | GPIO | ... | header.
"SPI6 is disabled" means nothing to a user, because the image labels the connector SPI with no SPI6 anywhere. Say "The SPI connector is not enabled in firmware."
These connectors are on the image but not in the README:
ETH: say it is Ethernet and that it needs NET_ENABLE = 1.
PPM: this is the FMU timer RC input. Mention it in RC Input.
SBUS OUT: say what it outputs.
SERIAL7 (FMU DEBUG, USART3) does not appear on any image. Show where it is, or mark it not user-accessible.
UART Mapping
"The UARTs are marked Rn and Tn in the above pinouts" is template boilerplate that doesn't apply here, since the connectors are labelled by name with plain TX/RX. Remove it.
Fill in the protocol column for SERIAL3/SERIAL4 (GPS) and SERIAL6/SERIAL7 (None) instead of leaving them blank.
Once the NODMA change above is in, SERIAL3, SERIAL4 and SERIAL7 have no RX DMA. Split the DMA column into TX DMA and RX DMA columns per the template.
Add a blank line between the sentence and the table.
RC Input
On H7, SERIAL6_OPTIONS "15" means the FPort wire goes to the RX pin, and "7" means the TX pin. State which pin to wire it to, otherwise users will guess.
PWM Output
Both group lists say "in 4 groups" but list three, and there are three: MAIN 1-2 / 3-4 / 5-8, and AUX1-4 (TIM5) / AUX5-6 (TIM4) / A7-A8 (TIM12, PWM only). Fix the counts and use the labels.
HAL_WITH_IO_MCU_BIDIR_DSHOT is set, so state which outputs support bi-directional DShot.
CAN
There's no CAN section. Add one naming CAN1/CAN2, and say that both are enabled for DroneCAN by default (defaults.parm sets CAN_P1_DRIVER/CAN_P2_DRIVER to 1), including the power-module connectors.
5V PWM Voltage
The parameter is BRD_PWM_VOLT_SEL, not BRD_PWM_VOL_SEL.
The X6 flight controller supports switching between 5V and 3.3V PWM levels. Switch PWM output pulse level by configuring parameter `BRD_PWM_VOLT_SEL`. 0 for 3.3V (default) and 1 for 5V output.
Re-request my review when these changes are made.
Previous review (2026-10-16)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head Thanks for the updates. Reviewed at d64fe68. The image links, commit squash, buzzer relabel and earlier hwdef fixes are all in, and the board builds. The one thing that needs to change before merge is DMA for the motor outputs. In the generated hwdef.h, TIM4_UP (AUX5-6) shares a DMA stream with the SPI3 IMU and FRAM, and TIM5_UP (AUX1-4) shares one with FRAM, the IOMCU UART and every other UART TX. When the stream is busy, RCOutput skips that DShot frame. Moving SPI ahead of TIM in DMA_PRIORITY caused this (ZeroOneX6's order keeps the timers unshared). Hwurzburg's suggestion works: adding DMA_NOSHARE SPI1* SPI2* SPI3* TIM* together with NODMA on PB7 USART1_RX, PE0 UART8_RX and PD9 USART3_RX gives the IMU buses and both timers their own streams. Please also check against the schematic which pin is nVDD_5V_HIPOWER_EN. The bootloader drives PF12 HIGH, but the firmware uses PG10 for it and treats PF12 as an ADC input. If the bootloader changes, rebuild it. README: the parameter is BRD_PWM_VOLT_SEL. For FPort with SERIAL6_OPTIONS 15 the wire goes to the RX pin. Use the MAIN1-8 / AUX1-6 / A7 / A8 labels and three groups per bank. Fill in the blank protocol cells, drop the Rn/Tn template line, and add the CAN, ETH, PPM and SBUS OUT details from Hwurzburg's last review. Smaller cleanup: remove the zero HAL_HEATER_MAG_OFFSET_RM3100 define and the leftover commented 6X lines, convert hwdef.dat to LF line endings, and drop the executable bit on README.md. Hardware behaviour (DShot timing, power rails, Ethernet) was not tested. |
Global Size Summary (Total Flash delta in bytes)Built from f4720b0 rebased onto 6743a33
|
Previous review (2026-10-20)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head Thanks, the DMA change works. I regenerated hwdef.h at cae4bfe, and the IMU SPI buses and both motor timers now have their own streams. The README fixes from the last round are in too. One thing blocks merge: this commit made libraries/AP_HAL_ChibiOS/hwdef/DroneerX6/hwdef.dat executable. It has no shebang, so the pre-commit check fails, and that is the red CI job. Please run git add --chmod=-x on hwdef.dat, and on README.md too. Smaller items: please check against the schematic which pin is nVDD_5V_HIPOWER_EN. The bootloader uses PF12 (hwdef-bl.dat:62), but the firmware uses PG10 and reads PF12 as an ADC input. If the bootloader changes, rebuild DroneerX6_bl.bin. In the README, bidirectional DShot works on MAIN1-4, and on MAIN7-8 only when MAIN5/6 are left out of the bidir mask. The F103 DShot IO firmware has no capture DMA for MAIN5/6, so line 96 should say that. Also drop the 'changes requested during review / verify against hwdef.h' wording on line 46; the table is correct. It would help to say which outputs BRD_PWM_VOLT_SEL switches, rather than telling users to check the hardware design. Hardware behaviour was not tested. |
Previous review (2026-10-20)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head Thanks, the exec bit is fixed and the README changes are in. Pre-commit now passes at 48ab09f. One thing blocks merge. You moved nVDD_5V_HIPOWER_EN to PG10 in hwdef-bl.dat (https://github.com/ArduPilot/ardupilot/pull/33573/files#diff-a3da52c29dc5df92bf9ebd71fbdb176a806a44b48f574ab53454fe3cf2636fcbR62), but Tools/bootloaders/DroneerX6_bl.bin and DroneerX6_bl.hex were not rebuilt (https://github.com/ArduPilot/ardupilot/pull/33573/files#diff-83f1f7b4c6e269741a3ed3546d464bc8ab6feac9526d088730e68b4d04059af3). The committed BIN is still the PF12 build. Apart from the git version string, it is identical to a bootloader built with line 62 set back to PF12, and it differs throughout from a build at this head. Every DroneerX6 firmware embeds that file as bootloader.bin, so a bootloader update would still drive PF12 (an ADC input in the app) HIGH and leave PG10 undriven. Please run Tools/scripts/build_bootloaders.py DroneerX6 and amend the new .bin and .hex into the 'bootloaders: Add DroneerX6 bootloader' commit. We have not checked against a schematic that PG10 is the right pin, and hardware behaviour was not tested. CI was still running when checked. |
Previous review (2026-10-20)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head Thanks for rebuilding the bootloader. The blocker from the last note is fixed at 4a8c1ca. I rebuilt the DroneerX6 bootloader from this head, and the committed Tools/bootloaders/DroneerX6_bl.bin matches it apart from the embedded git version string (https://github.com/ArduPilot/ardupilot/pull/33573/files#diff-83f1f7b4c6e269741a3ed3546d464bc8ab6feac9526d088730e68b4d04059af3). The .hex matches the .bin. The bootloader now drives PG10 (nVDD_5V_HIPOWER_EN, https://github.com/ArduPilot/ardupilot/pull/33573/files#diff-a3da52c29dc5df92bf9ebd71fbdb176a806a44b48f574ab53454fe3cf2636fcbR62) and leaves PF12 as an input, and the copter firmware embeds the new image. Nothing else changed since 48ab09f, and I found no new problems. The one red CI job, the macOS 'build (CubeOrange)', failed while installing prerequisites, before any build ran. It is unrelated to this PR and should pass on a re-run. We have not checked against a schematic that PG10 is the right pin, and hardware behaviour was not tested. |
Hwurzburg
left a comment
There was a problem hiding this comment.
Thanks. The DMA change, the cleanup and nearly all of the README items are done. What's left:
README.md
- UART mapping: drop the CPU pin names from the SERIAL7 note, they mean nothing to users:
TELEM2 and TELEM3 have RTS/CTS hardware flow control; the other UART connectors do not. SERIAL7 (USART3) is not pinned out to a user connector.
Also use "No" rather than No (`NODMA`) in the RX DMA column, and use the image labels GPS & SAFETY and UART4&I2C in the Connector label column.
- PWM output: I would keep the bi-directional DShot statement to
MAIN1-MAIN4. Those are the outputs the STM32F103 DShot IO firmware tags as BIDIR. The MAIN5-8 timer is commented out in that hwdef as not supported, so leave the conditional MAIN7-8 sentence out rather than document something users can't rely on:
For **bidirectional DShot** on the I/O MCU, `MAIN1`–`MAIN4` are supported when `BRD_IO_DSHOT` = 1. I/O MCU bidirectional DShot is enabled in the firmware using `HAL_WITH_IO_MCU_BIDIR_DSHOT=1`.
- CAN: remove the
CAN_Dx_PROTOCOLsentence. DroneCAN is already the default protocol, so the user has nothing to set. - Battery monitoring: the hwdef defaults
BATT_MONITORto 8 (DroneCAN), so say so instead of asking users to verify:
The X6 flight controller has two six-pin power connectors that support DroneCAN power modules. `BATT_MONITOR` defaults to 8 (DroneCAN).
- Compass: use the stock wording:
The X6 flight controller has a built-in RM3100 compass. Due to potential interference, the autopilot is usually used with an external I2C compass as part of a GPS/Compass combination.
- 5V PWM voltage: the new text says
BRD_PWM_VOLT_SELonly affectsMAIN1–MAIN8. Please confirm that's how the board is wired, since the hwdef alone doesn't show it. If you're not sure, go back to the suggested wording without the MAIN/AUX split.
board_types.txt
The DroneerX6 line uses tabs; every other line uses spaces. Use spaces.
Re-request my review when these changes are made.
Previous review (2026-10-21)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head Two small README and commit-order fixes are left; everything else looks good at 852d436. Since the last note (4a8c1ca), the board_types.txt spacing and nearly all of Hwurzburg's README requests are done, and the bootloader, hwdef.dat and hwdef-bl.dat are unchanged, so the earlier bootloader fix still holds. Should fix
Minor (1)
CI at this head had not started when checked, and hardware behaviour, including whether PG10 is the right pin, was not tested. |
AP_Bootloader: use spaces
Previous review (2026-10-22)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head One small README label fix is left; everything else looks good at dfca927. Since the last note (852d436), the commit order is fixed: the AP_Bootloader board ID commit comes first again, and the hwdef, bootloader and images are unchanged. Should fix
CI at this head was 59 of 61 green with two jobs still running when checked. Hardware behaviour, including whether PG10 is the right pin, was not tested. |
Previous review (2026-10-22)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head No open issues at 2e82bc5, and CI is green (61 of 61). Since the last note (dfca927), the SERIAL4 label in the README is fixed: it now says The bootloader and copter rebuild cleanly at this head, and the committed bootloader matches a fresh build. Hardware behaviour was not tested, including whether PG10 is the right 5V high-power enable pin. |
|
Automated hwdef review ( PR 33573 - base Build
Should-fix
Notes
Generated by Claude via |
andyp1per
left a comment
There was a problem hiding this comment.
LGTM, just a couple of minor nits
bootloaders: update DroneerX6 bootloader bootloader: update bin
hwdef:fix DroneerX6 README.md hwdef: fix DroneerX6 README.md hwdef: modify DroneerX6 hwdef.dat and defaults.parm hwdef: update pinout hwdef: update readme hwdef: update README.md hwdef: update README.md hwdef: update README.md hwdef: update README.md hwdef: modify hwdef
|
Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head No open issues at f4720b0. CI is green apart from one job (check_autotest_options) that was cancelled at the runner's 15-minute limit; a re-run should clear it. Since the last note (2e82bc5), andyp1per's three hwdef items are addressed: AUX1-AUX6 have BIDIR, PI10 EXT1_CS idles high, and PG6 starts at 3.3V in both the firmware and the bootloader. The new capture DMA only shares each motor timer's own stream, and the bootloader, copter and plane build at this head. The committed bootloader matches a fresh build. The author has not yet answered andyp1per's question about I2C pull-ups. Not fully reviewed: hardware behaviour (bidirectional DShot on AUX1-AUX6, PWM voltage, PG10 5V enable, I2C pull-ups, Ethernet, CAN) was not tested, and rover, heli, sub, blimp and tracker were not built. |
Summary
Add board support for DroneerX6 flight controller.
Classification & Testing
Checked by a human programmer
Non-functional change
Tested on hardware
Description
This PR add board support for DroneerX6 flight controller. We have verified that the sensors and interfaces are functioning properly and conducted flight tests.