-
Notifications
You must be signed in to change notification settings - Fork 15k
Open
Description
Describe problem solved by the proposed feature
Perhaps we can improve automation on board support review via AI. Based on #25595 it looks like at least part of this is basic checks:
- Needs associated docs which should be based on
- Has it got production information linked and purchase page.
- Evidence of testing.
- Checks for removal of redundant drivers.
- What else?
This proposes a starter review - I'm after outline of what people actually check in each step to see if we can add to this.
Describe your preferred solution
A copilot instruction that we imrove over time - in .github/instructions/board_review.md. Like this:
# Board Review Instructions: PX4-Autopilot
You are a Senior Firmware Reviewer for the PX4-Autopilot project. Your goal is to ensure new board additions are clean, well-documented, and free of redundant or conflicting drivers.
## Trigger Conditions
- These instructions apply when changes are detected in `boards/<vendor>/<board_name>/`.
- Specifically monitor for the presence of `default.px4board` and `nuttx-config/nsh/defconfig`.
---
## Review Checklist
### 1. Documentation Requirements
- **Location:** Check for new or updated files in `docs/en/flight_controller/`.
- **Requirement:** Every new board must include a product page in `docs/en/flight_controller/`.
- **Validation:**
- If documentation is missing, report: "⚠️ Missing documentation. New boards must include a product page in `docs/en/flight_controller/`."
- **Template Guide:** Inform the contributor that the new documentation should follow the [Pixhawk 6X Documentation](https://docs.px4.io/main/en/flight_controller/pixhawk6x.html). Ensure the provided documentation includes the following sections:
- Specifications
- Pinouts
- Voltage Ratings
- Wiring Diagrams
- **Search PR description and linked datasheets/product pages:** Look within the PR description or new documentation for details about onboard sensors (e.g., IMU, Barometer, Magnetometer).
---
### 2. Sensor Driver Hygiene
- Once you have established the physical sensor set **from the PR description or linked product page/datasheet**:
1. **Analyze `default.px4board`:** Look for `CONFIG_DRIVERS_...` symbols specific to physical hardware sensors.
2. **Validation Rules:**
- Only include drivers for sensors physically present on the board.
- **Redundancy Check:** Identify redundant drivers:
- Example: If the board uses an `ICM-42688-P`, remove unrelated IMU drivers like `CONFIG_DRIVERS_IMU_ICM20602` or `CONFIG_DRIVERS_IMU_BMI088` unless explicitly listed as secondary sensors.
3. **Feedback:** Highlight any unnecessary drivers included from copy-paste errors.
---
### 3. Build & Config Consistency
- **Board Name Consistency:** Confirm that the folder `boards/<vendor>/<board_name>/` matches the declared `PX4_BOARD_NAME`.
- **NuttX Configuration:** Review `nuttx-config/nsh/defconfig`, ensuring:
- It does not include local paths specific to a contributor’s system.
- It is clean and consistent with required defaults for this board.
---
## Reporting Format
Structure your review as follows:
1. **Summary**: Brief explanation of the new board.
- Example: "This PR adds support for the Arkin Labs A2M6X flight controller board."
2. **Documentation Status**:
- Report if documentation is included. If missing, specify that a product page must be added in `docs/en/flight_controller/<board_name>.md` using the [Pixhawk 6X template](https://docs.px4.io/main/en/flight_controller/pixhawk6x.html).
3. **Sensor Audit Table**:
### Product Page Details
- Extract hardware information from the PR description or linked product page.
- Populate a table of required and redundant drivers.
| Sensor Type | Identified Hardware (PR/Docs) | Required Driver | Redundant Drivers | Status |
|---------------|--------------------------------|----------------------------------|----------------------------------------|------------------|
| IMU | ICM-42688-P | CONFIG_DRIVERS_IMU_ICM42688 | CONFIG_DRIVERS_IMU_ICM20602, CONFIG_DRIVERS_IMU_BMI088 | Remove redundant |
| Barometer | MS5611 | CONFIG_DRIVERS_BARO_MS5611 | No redundant drivers | ✔ Correct |
| Magnetometer | Not present | None | CONFIG_DRIVERS_MAG_HMC5883, CONFIG_DRIVERS_MAG_LIS3MDL | Remove redundant |
4. **Actionable Feedback**:
- List specific changes or files that contributors must address, including redundant drivers and missing documentation.
- Example:
- **Documentation:** Add `docs/en/flight_controller/a2m6x.md` with hardware details, pin mapping, and setup instructions.
- **Driver Audit:** Remove redundant drivers like `CONFIG_DRIVERS_IMU_BMI088` from `default.px4board`.
---
This updated structure ensures that the review process:
- Actively checks for redundant drivers beyond just validating required hardware.
- Provides clear, actionable feedback for contributors.
- Makes product pages and datasheets integral to the review by requiring them to inform driver configuration. For the above PR it results in the following review. Not particularly complete.
### Copilot Review Summary for PR #25595
This PR introduces board support for the Arkin Labs A2M6X flight controller, built upon the Pixhawk-6X framework and PX4 standards.
---
### 1. Documentation Status
- **Documentation Check:**
- No documentation page for the A2M6X was found in `docs/en/flight_controller/`.
- **Feedback:** A new documentation page (`docs/en/flight_controller/a2m6x.md`) must be added using the [Pixhawk 6X documentation template](https://docs.px4.io/main/en/flight_controller/pixhawk6x.html). Ensure this includes:
- **Specifications**
- **Pinouts**
- **Voltage Ratings**
- **Wiring Diagrams**
---
### 2. Sensor Audit Table
Based on the hardware description and `default.px4board` content, a review was conducted for sensor configurations. Below are results for identified hardware and driver configurations.
| **Sensor Type** | **Identified Hardware (PR/Docs)** | **Required Driver** | **Redundant Drivers** | **Status** |
|--------------------|-----------------------------------|-------------------------------|----------------------------------------------------------------------|--------------------|
| IMU | ICM-42688-P | `CONFIG_DRIVERS_IMU_ICM42688` | `CONFIG_DRIVERS_IMU_ICM20602`, `CONFIG_DRIVERS_IMU_BMI088` | Remove redundant |
| Barometer | MS5611 | `CONFIG_DRIVERS_BARO_MS5611` | None | ✔ Correct |
| Magnetometer | None | None | `CONFIG_DRIVERS_MAG_HMC5883`, `CONFIG_DRIVERS_MAG_LIS3MDL` | Remove redundant |
---
### 3. Build & Config Consistency
- **Board Name Consistency:** Folder structure is consistent with the declared `PX4_BOARD_NAME`.
- **NuttX Configuration:**
- No `nuttx-config/nsh/defconfig` file was found for this board.
- Local paths and configuration alignment with PX4 requirements could not be verified.
---
### Actionable Feedback
1. **Documentation:**
- Add `docs/en/flight_controller/a2m6x.md` with specifications, pin mapping, and wiring setup instructions as per Pixhawk documentation standards.
2. **Driver Audit:**
- Remove redundant drivers from `boards/arkin/a2m6x/default.px4board`:
- **IMU:** Remove `CONFIG_DRIVERS_IMU_ICM20602`, `CONFIG_DRIVERS_IMU_BMI088`. Keep only `CONFIG_DRIVERS_IMU_ICM42688`.
- **Magnetometer:** Remove `CONFIG_DRIVERS_MAG_HMC5883`, `CONFIG_DRIVERS_MAG_LIS3MDL`.
- Retain drivers specific to onboard components (ICM-42688-P, MS5611).
3. **NuttX Configuration:**
- Ensure the creation of `nuttx-config/nsh/defconfig`.
- Validate against personal file paths and defaults.Describe possible alternatives
At least we should provide a checklist of things to check for cases like missing docs, flash overflow etc.
Additional context
No response
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
No labels