refactor: send the set-home command as COMMAND_INT - #2874
Conversation
Automated PR Review (Claude)0. SummaryVerdict: READY TO MERGE This PR switches the 1. Correctness & Implementation Bugs — ✅2. AGENTS.md Adherence — ✅3. Security — ✅4. Performance — ✅5. UI / UX — ✅6. Code Quality & Style6.1 7. Commit Hygiene — ✅Single logical commit with a descriptive 8. Tests8.1 9. Documentation — ✅10. Nitpicks / Optional10.1 Generated by Claude. This is advisory; a human reviewer must still approve. |
COMMAND_LONG carries the position as floats and has no frame field, so the altitude reference was left implicit. COMMAND_INT carries the position as scaled integers and states the frame explicitly, which is what lets the user pick an altitude reference when setting home. sendCommandInt hardcoded the frame, so it now takes one, defaulting to MAV_FRAME_GLOBAL to leave the existing goTo caller untouched.
Callers pass an altitude of zero, which under MAV_FRAME_GLOBAL means sea level, so setting a home point put home's altitude at 0 m AMSL. That is wrong for any vehicle operating at elevation and skews return-to-launch altitudes. Zero in a home-relative frame instead resolves against the current home, moving it horizontally and leaving its altitude untouched. sendCommandInt keeps defaulting to MAV_FRAME_GLOBAL so goTo is unaffected.
be3f61e to
7b40474
Compare
| altitude: number, | ||
| frame: MavFrame = MavFrame.MAV_FRAME_GLOBAL_RELATIVE_ALT | ||
| ): Promise<void> { | ||
| await this.sendCommandInt(MavCmd.MAV_CMD_DO_SET_HOME, 0, 0, 0, 0, coordinates[0], coordinates[1], altitude, frame) |
There was a problem hiding this comment.
Would be nice if this was less hard-codey, so it's not necessary to loop up the spec to understand some of the fields.
| await this.sendCommandInt(MavCmd.MAV_CMD_DO_SET_HOME, 0, 0, 0, 0, coordinates[0], coordinates[1], altitude, frame) | |
| await this.sendCommandInt( | |
| MavCmd.MAV_CMD_DO_SET_HOME, | |
| 0, // Use the specified location, not the vehicle's current location | |
| 0, 0, 0, // Don't set roll/pitch, and set north(?)-facing yaw | |
| coordinates[0], coordinates[1], altitude, | |
| frame // Reference frame for the coordinates and altitude | |
| ) |
Having checked the spec, perhaps we should be using NaN values for roll/pitch/yaw, so we don't accidentally set a specific heading for the home location that the user doesn't control? 0 and NaN are equivalent for roll+pitch, but feel less intuitive as a non-valid sentinel value, and masks the fact that 0 for yaw is interpreted explicitly instead of ignored.
For future reference, I suspect we'll want to provide at least some method for the user to specify the vehicle's current location as its new home.
| async function setHomeWaypoint(coordinate: [number, number], height: number): Promise<void> { | ||
| async function setHomeWaypoint( | ||
| coordinate: [number, number], | ||
| height: number, |
There was a problem hiding this comment.
height seems unclear given there's no requirement for the value to be above the home (and in fact Sub vehicles typically operate below a surface-based home).
If this method is general we should stick with MAVLink's altitude terminology for clarity and consistency. If instead we're operating on a filtered down subset of use-cases perhaps we could call this distanceAboveHome or similar, though I imagine it will be a challenge to find a really nice term.
Summary
DO_SET_HOMEwent out as aCOMMAND_LONG, which carries latitude and longitude as floats and has no frame field, leaving the altitude reference implicit.COMMAND_INTcarries the position as scaled integers and states the frame explicitly, which is what lets the user pick an altitude reference when setting a home point.Two commits:
Send the command as
COMMAND_INT.setHomeWaypointnow goes throughsendCommandInt. The parameter slots line up with the previoussendCommandLongcall, soparam1stays 0 (use the coordinates carried by the command rather than the vehicle's current position) and lat/lon/alt keep their positions.sendCommandInthardcodedMAV_FRAME_GLOBAL, so it now takes a frame, still defaulting toMAV_FRAME_GLOBALso the existinggoTocaller is untouched.Default the set-home altitude to home-relative. Both call sites pass an altitude of
0. UnderMAV_FRAME_GLOBALthat means 0 m AMSL, so setting a home point was putting home's altitude at sea level — wrong for any vehicle operating at elevation, and it skews return-to-launch altitudes.setHomeWaypointnow defaults toMAV_FRAME_GLOBAL_RELATIVE_ALT, where zero resolves against the current home: home moves horizontally and its altitude is left alone.This is also groundwork for the dialog that will let the user choose the frame and altitude explicitly, which is where a non-default frame will first get passed in.
Test plan
COMMAND_INTis sent withMAV_FRAME_GLOBAL_RELATIVE_ALTand lat/lon arrive scaled by 1e7, not zeroed.DO_SET_HOME(ArduPilot's handling has varied by version and vehicle) — if it rejects the command, the default should move toMAV_FRAME_GLOBALwith a real altitude rather than zero.DO_REPOSITIONstill behaves as before, confirming thesendCommandIntframe default did not regress that caller.Fixes #2871.