- Repository: PX4/PX4-Autopilot
- Reviewer: @dagar (Daniel Agar)
- Total PRs sampled: 200 (from a pool of 3,762 reviewed PRs)
- PRs with substantive comments: 99 out of 200 (49.5%)
- PR number range: #24002 - #25887 (spanning ~1.5 years of reviews)
- Data source: GitHub API - PR review comments, inline comments, and issue comments
@dagar is the architectural gatekeeper of PX4. His reviews consistently prioritize modularity, safety, simplicity, and correctness over feature velocity. He has deep knowledge of every subsystem (EKF2, uORB, MAVLink, drivers, build system, commander) and uses that knowledge to catch subtle issues that less experienced reviewers would miss.
The single most frequent review theme. Dagar consistently redirects code to the right module and pushes for clean separation of concerns.
Core principles:
- Put logic in the module that naturally owns the data
- Avoid cross-module dependencies (especially commander depending on EKF2 internals)
- Prefer parameter-driven configuration over compile-time decisions
- Build-time code generation over runtime detection
- Prefer modular, per-driver parameters over monolithic configuration
Representative examples:
- "commander can't use an EKF2 parameter via module params because then it becomes a hard dependency. It might be less bad if implemented within ekf2." (PR #24193)
- "This is fine as a cleanup for now, but I think ultimately it would be better if an airframe could be configured entirely from scratch using only parameters." (PR #25839)
- "I'm not sure I agree with merging all the super simple little simulation modules." (PR #25582)
- "We don't need a generic 'RAM_CONSTRAINED_SYSTEM', we can simply do a real kconfig level NUM_MISSIONS_SUPPORTED." (PR #24252)
- "Would it instead make sense to solve this via shutdown hooks in the output modules?" (PR #24290)
- "having separate classes could make each individually a bit easier to use, harder to misuse." (PR #24173)
"Why?" is dagar's signature review comment. He challenges every addition to justify its existence.
Representative examples:
- "Is this real or leftover?", "delete unless you're actually using it", "Do you actually want/need this or was it just copied?" (PR #25102)
- "Why?" - single-word challenge (PRs #24734, #24600)
- "Why 1 ms? If the delay matters add a comment, otherwise don't sprinkle in magic timing." (PR #24691)
- "Are users really going to configure this?" (PR #24710)
- "I don't see why this should be calling param_save_default, the param_set will trigger an auto save in the background. Can you remove all the param_save_default calls?" (PR #25412)
Deeply paranoid about edge cases, especially in flight-critical code paths.
Sub-patterns:
- Race conditions and partially-configured states
- Sensor validation and recovery from disconnects
- EKF state reset protection
- Actuator safety during reboots
- Loop bounds as safety nets
Representative examples:
- "Couldn't you still slip in a partially configured state?" (PR #25599)
- "Upper bound on loops just to be paranoid." (PR #25156)
- "There's been a long history of seemingly random unexplainable crap happening on (probably cheap) boards and sensors, so as a result we're paranoid and try to verify everything." (PR #24691)
- "At a minimum if you disconnect and reconnect the sensor while it's running the driver should be able to catch that and reset/recover." (PR #24691)
- "Are we completely sure additional protection isn't needed so that we only start fake_pos as a last resort given it will reset pos/vel on start?" (PR #24247)
- "please don't overload something as critical as manual control loss" (PR #25243)
- "If anything we might want to reject an impossibly small loiter radius in the first place." (PR #24253)
Names must accurately describe what the code does. No abbreviations unless universal, no misleading terms.
Representative examples:
- "Maybe something like custom init or additional init? Calling it dynamic init, when it's baked into the static configuration seems not quite right." (PR #25449)
- "Does this actually need to be abbreviated?" (PR #25498)
- "We're slowly moving the naming from GPS -> GNSS, so for anything new we might as well start there." (PR #25012)
- "underscore uppercase naming is reserved in c++" (PR #25206)
- "Can we spell it out everywhere instead?" (PR #24721 - rejecting abbreviation of "spacecraft")
- "This isn't static allocation, it's a fixed size pool allocator vs one that can grow." (PR #25716)
- "File descriptor is a pretty specific thing." (PR #24022 - correcting terminology)
Prefers removing code over adding it. Opts into complexity only when justified.
Representative examples:
- "Let's just change the default to 2 and consider dropping the autodetection code." (PR #25583)
- "Let's opt in instead of out." (PR #25052 - suggesting allowlist over blocklist)
- "You could probably just do the software reset regardless (first). If the GPIO reset worked it won't matter." (PR #24691)
- "How exhaustively do we want/need to enforce the innovation gate minimum? Maybe we should just do it once during parameter update rather than all over the place every sample." (PR #24588)
- "You could just drop this completely, just limit it to publishing above when the timestamp_sample actually changes." (PR #25385)
Validate at boundaries, handle failure explicitly, don't spam logs.
Representative examples:
- "At a minimum we should probably print an error if the node ID value isn't valid." (PR #25669)
- "Can you zero SENS_DPRES_OFF on appropriate failures rather than only setting SENS_DPRES_OFF on success?" (PR #25412)
- "Be careful these can't spam." (PR #24691 - about error prints)
- "There's really no reason it should fail, but it wouldn't hurt to check that registerCallback() succeeded." (PR #24727)
- "What if libOpticalFlowSystem.so doesn't exist?" (PR #24441)
Treats uORB messages as API contracts. Deep knowledge of timestamp semantics, device IDs, and publication patterns.
Representative examples:
- "The timestamp is publication metadata, the timestamp_sample should be as close to the physical sample as possible." (PR #25712)
- "Additionally please include a device_id field so we know which receiver is providing this information." (PR #25012)
- "I'd also try to avoid making msgs versioned by default and only do it once strictly needed." (PR #25693)
- "Please drop all of this custom send interval, the mavlink module is in control of the rate per stream." (PR #25012)
- "Ideally vtol_vehicle_status would publish immediately on any actual change, otherwise at 1 Hz." (PR #24582)
Active in build infrastructure. Prefers cmake over Makefiles, proper Kconfig defaults, and clean CI configuration.
Representative examples:
- "The Makefile is optional and often bypassed by IDEs. Can you push this into actual cmake?" (PR #25353)
- "The easiest thing would be to generate all of this at build time where everything is easily available." (PR #25414)
- "We should slip it into the Dockerfile. Keep in sync with Tools/docker_run.sh." (PR #24022)
- "once things settle we should really strip it down to only actual PX4 tags" (PR #24027)
Every numeric literal needs justification or a named constant.
Representative examples:
- "Instead of the magic numbers put the actuator_function constants in the msg." (PR #25849)
- "Any justification for the magic 25%? Why not 10%?" (PR #25373)
- "Not that it really matters here, but generally don't use floating point for absolute time." (PR #25534)
Attention to integer overflow, proper numeric types, and compile-time checks.
Representative examples:
- "Don't just multiply by -1, we need to respect if it's saturated to detect clipping. The range of an INT16 is -32,768 to 32,767." (PR #24691)
- "Just to be safe I would either limit the configurable integration interval or bump the rest to uint32_t." (PR #24202)
- Suggested
static_assert(MessageNewer::MESSAGE_VERSION == 1);for compile-time verification (PR #24716)
Use what the framework provides instead of reinventing.
Representative examples:
- "Why not use the actual _retries mechanism in I2C?" (PR #24163)
- "PX4_ERR (and PX4_WARN/PX4_INFO) include the module in the output." (PR #24341)
- "use the constants from mavlink" (PR #25012)
Knows PX4 param system internals. Dislikes driver-level parameter hacks.
Representative examples:
- "param_set will trigger an auto save in the background" - knows internal behavior (PR #25412)
- "SENS_DPRES_REV really shouldn't have been done in the actual drivers." (PR #25412)
- "Having a separate parameter per driver/module is intentional to keep it modular." (PR #24288)
Asks for evidence. Uses CI as a gate. Advocates for replay testing.
Representative examples:
- "This looks correct, can you comment on testing?" (PR #25618)
- "We should review a few logs side by side." (PR #24831)
- "We should really get some basic replay testing in place." (PR #24438)
- "Have you tested the driver on actual hardware?" (PR #24014)
Comments should explain "why", not "what". No comments for obvious code.
Representative examples:
- "At least add a comment with a TODO." (PR #25353)
- "At least add a comment explaining why the time constant is 1?" (PR #25224)
- "If the delay matters add a comment, otherwise don't sprinkle in magic timing." (PR #24691)
Considers how less-technical users interact with parameters and messages.
- "a lot of uninformed users can't resist the urge to select the biggest number (even if they have a worse time)" (PR #24240 - on hiding DShot 1200)
- Improved preflight failure messages for QGC users (PR #24080)
- "Can you revert the submodule changes?" (PR #25855)
- "There's no actual airframe file." (PR #25833 - PR incomplete)
- "The PR is reporting conflicts and I can't merge. Can you rebase?" (PR #24733)
- "Might as well use starting conditions consistently." (PR #25634)
- "let's keep this in the same order as the enum" (PR #25350)
- Numbering consistency in airframe IDs (PR #25405)
- "Do you have any documentation for this board? Can it be purchased online?" (PR #25364)
- Board naming convention: "alternatively you could name the board vololand_narinfc-h7" (PR #25362)
| Rank | Category | Count | % of Comments |
|---|---|---|---|
| 1 | Architecture / Design Patterns | 29 | 18.7% |
| 2 | Safety-Critical / Defensive Programming | 17 | 11.0% |
| 3 | Naming Conventions / Clarity | 15 | 9.7% |
| 4 | Code Simplification / Dead Code | 14 | 9.0% |
| 5 | Questioning Necessity ("Why?") | 13 | 8.4% |
| 6 | uORB / Messaging / Data Model | 12 | 7.7% |
| 7 | Build System / CI / CMake | 11 | 7.1% |
| 8 | Error Handling / Validation | 10 | 6.5% |
| 9 | Testing / Verification | 7 | 4.5% |
| 10 | Type Safety / Overflow | 6 | 3.9% |
| 11 | Parameter Handling | 5 | 3.2% |
| 12 | Magic Numbers / Constants | 5 | 3.2% |
| 13 | PR Scope / Hygiene | 4 | 2.6% |
| 14 | Leveraging Existing Mechanisms | 4 | 2.6% |
| 15 | Consistency | 3 | 1.9% |
| 16 | Documentation / Comments | 3 | 1.9% |
| 17 | User Experience | 2 | 1.3% |
| 18 | Board/Hardware Support | 2 | 1.3% |
| TOTAL | 162 |
The number one concern. Every module should own its own data and logic. Cross-module dependencies are a code smell. If commander needs EKF2 data, it should go through a uORB topic, not import EKF2 parameters. If a fix applies to one board, ask if it should apply to all boards.
Actively seeks to remove autodetection logic, unnecessary abstractions, redundant parameter saves, and dead code. Will challenge every addition with "Why?" or "Is this real or leftover?" Prefers changing a default over adding runtime detection.
In a flight controller, edge cases kill. He adds loop bounds "just to be paranoid," checks for partially-configured states, worries about sensor disconnect/reconnect, and refuses to let critical paths like RC loss be overloaded for convenience.
A name that doesn't match behavior is a bug. Abbreviations need justification. C++ naming rules must be followed. When the project renames a concept (GPS to GNSS), all new code should use the new name.
PX4 has extensive infrastructure (uORB, I2C retry mechanisms, param auto-save, PX4_ERR macros, MAVLink stream rate control). Contributors should use these rather than reimplementing functionality.
Rather than checking conditions at every consumer, reject bad data at the source. Don't sprinkle validation "all over the place every sample" when you can do it "once during parameter update."
Asks for test results, flight logs, side-by-side comparisons, and hardware testing before merging. Uses CI as a hard gate: "Nevermind, CI is unhappy."
Even at the driver level, considers how parameter choices will confuse users ("uninformed users can't resist the urge to select the biggest number") and how error messages will appear in QGC.
- Concise: Most comments are 1-2 sentences. Sometimes just "Why?"
- Direct: No hedging or softening. Says what he means.
- Constructive: Almost always provides an alternative approach or specific code suggestion
- Pragmatic: Accepts interim solutions while noting the ideal direction: "This is fine as a cleanup for now, but I think ultimately..."
- Deep knowledge: References specific internal behaviors (param auto-save triggers, I2C_RESET side effects, uORB timestamp semantics) that only someone who wrote the code would know
- Broad scope: Thinks beyond the PR to how the change fits in the system, whether it applies to other boards, and how it affects end users
- Occasionally expressive: Uses emoji when code quality is particularly bad (PR #25353: "🤮")