Reboot vehicle only for ROVs if heartbeat lost #3790
Open
patrickelectric wants to merge 4 commits intobluerobotics:masterfrom
Open
Reboot vehicle only for ROVs if heartbeat lost #3790patrickelectric wants to merge 4 commits intobluerobotics:masterfrom
patrickelectric wants to merge 4 commits intobluerobotics:masterfrom
Conversation
…reType Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
…wareType enum Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
…ly for ROV We don't want to reboot flying vehicles Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Reviewer's GuideRestricts automatic Ardupilot restarts on heartbeat loss to ROV/ArduSub vehicles by introducing a typed MAVLink firmware enum and using it to gate the heartbeat-based reboot logic, while updating VehicleManager and related helpers to use the new type-safe firmware type API. Sequence diagram for conditional ArduSub-only heartbeat restartsequenceDiagram
participant AutopilotManager
participant VehicleManager
participant Mavlink2Rest
AutopilotManager->>AutopilotManager: auto_restart_ardupilot loop
AutopilotManager->>VehicleManager: is_heart_beating() async
VehicleManager-->>AutopilotManager: alive or failure
alt heartbeat failure
AutopilotManager->>AutopilotManager: increment _heartbeat_fail_count
else heartbeat ok
AutopilotManager->>AutopilotManager: reset _heartbeat_fail_count
end
AutopilotManager->>AutopilotManager: check _heartbeat_fail_count >= _max_heartbeat_failures
alt threshold reached
AutopilotManager->>AutopilotManager: check _firmware_vehicle_type is None
alt firmware type unknown
AutopilotManager->>VehicleManager: get_vehicle_type() async
VehicleManager->>Mavlink2Rest: get_updated_mavlink_message(HEARTBEAT) async
Mavlink2Rest-->>VehicleManager: heartbeat_message
VehicleManager-->>AutopilotManager: MavlinkVehicleType
AutopilotManager->>AutopilotManager: _firmware_vehicle_type = vehicle_type.mavlink_firmware_type()
end
AutopilotManager->>AutopilotManager: check _firmware_vehicle_type == MavlinkFirmwareType.ArduSub
alt firmware is ArduSub
AutopilotManager->>AutopilotManager: restart_ardupilot() async
else firmware is not ArduSub
AutopilotManager->>AutopilotManager: skip restart
end
AutopilotManager->>AutopilotManager: _heartbeat_fail_count = 0
end
Class diagram for MAVLink firmware typing and conditional heartbeat restartclassDiagram
class AutopilotManager {
- VehicleManager vehicle_manager
- int _heartbeat_fail_count
- int _max_heartbeat_failures
- MavlinkFirmwareType _firmware_vehicle_type
- bool should_be_running
+ setup() async
+ auto_restart_ardupilot() async
+ restart_ardupilot() async
+ is_running() bool
}
class VehicleManager {
- Mavlink2Rest mavlink2rest
+ get_vehicle_type() async MavlinkVehicleType
+ get_firmware_vehicle_type() async MavlinkFirmwareType
}
class MavlinkVehicleType {
<<enumeration>>
MAV_TYPE_GENERIC
MAV_TYPE_FIXED_WING
MAV_TYPE_VTOL_DUOROTOR
MAV_TYPE_VTOL_QUADROTOR
MAV_TYPE_VTOL_TILTROTOR
MAV_TYPE_VTOL_RESERVED2
MAV_TYPE_VTOL_RESERVED3
MAV_TYPE_VTOL_RESERVED4
MAV_TYPE_VTOL_RESERVED5
MAV_TYPE_QUADROTOR
MAV_TYPE_COAXIAL
MAV_TYPE_HELICOPTER
MAV_TYPE_ANTENNA_TRACKER
MAV_TYPE_GCS
MAV_TYPE_AIRSHIP
MAV_TYPE_FREE_BALLOON
MAV_TYPE_ROCKET
MAV_TYPE_GROUND_ROVER
MAV_TYPE_SURFACE_BOAT
MAV_TYPE_SUBMARINE
MAV_TYPE_HEXAROTOR
MAV_TYPE_OCTOROTOR
MAV_TYPE_TRICOPTER
MAV_TYPE_FLAPPING_WING
MAV_TYPE_KITE
MAV_TYPE_ONBOARD_CONTROLLER
MAV_TYPE_VTOL_RESERVED6
MAV_TYPE_VTOL_RESERVED7
MAV_TYPE_VTOL_RESERVED8
MAV_TYPE_VTOL_RESERVED9
MAV_TYPE_GIMBAL
MAV_TYPE_ADSB
MAV_TYPE_PARAFOIL
MAV_TYPE_DODECAROTOR
MAV_TYPE_CAMERA
MAV_TYPE_CHARGING_STATION
MAV_TYPE_FLARM
MAV_TYPE_SERVO
MAV_TYPE_ODID
MAV_TYPE_DECAROTOR
MAV_TYPE_BATTERY
MAV_TYPE_PARACHUTE
MAV_TYPE_LOG
MAV_TYPE_OSD
MAV_TYPE_IMU
MAV_TYPE_GPS
MAV_TYPE_WINCH
+ mavlink_firmware_type() MavlinkFirmwareType
+ is_actually_a_vehicle() bool
}
class MavlinkFirmwareType {
<<enumeration>>
ArduPlane
ArduCopter
ArduRover
ArduSub
Unknown
}
class Mavlink2Rest {
+ get_updated_mavlink_message(message_name) async dict
}
AutopilotManager --> VehicleManager : uses
VehicleManager --> Mavlink2Rest : uses
VehicleManager --> MavlinkVehicleType : returns
VehicleManager --> MavlinkFirmwareType : returns
MavlinkVehicleType --> MavlinkFirmwareType : maps
AutopilotManager --> MavlinkFirmwareType : caches
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
patrickelectric
requested review from
Williangalvani,
joaoantoniocardoso and
joaomariolago
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The cached
_firmware_vehicle_typeis only populated lazily on heartbeat failure and never reset on (re)start, which could make the restart logic stale if the vehicle type changes; consider refreshing or clearing this cache whenever Ardupilot is (re)started. - In
auto_restart_ardupilot, the broadexcept Exception: passaroundget_vehicle_type()failures fully swallows errors and hides why a restart didn’t occur; consider at least debug-level logging or narrowing the exception to avoid silently ignoring unexpected issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The cached `_firmware_vehicle_type` is only populated lazily on heartbeat failure and never reset on (re)start, which could make the restart logic stale if the vehicle type changes; consider refreshing or clearing this cache whenever Ardupilot is (re)started.
- In `auto_restart_ardupilot`, the broad `except Exception: pass` around `get_vehicle_type()` failures fully swallows errors and hides why a restart didn’t occur; consider at least debug-level logging or narrowing the exception to avoid silently ignoring unexpected issues.
## Individual Comments
### Comment 1
<location> `core/services/ardupilot_manager/autopilot_manager.py:228-231` </location>
<code_context>
logger.warning("Consecutive heartbeat failures threshold reached — restarting Ardupilot.")
try:
- await self.restart_ardupilot()
+ if self._firmware_vehicle_type is None:
+ try:
+ vehicle_type = await self.vehicle_manager.get_vehicle_type()
+ self._firmware_vehicle_type = vehicle_type.mavlink_firmware_type()
+ except Exception:
+ pass
</code_context>
<issue_to_address>
**suggestion:** Reuse VehicleManager.get_firmware_vehicle_type instead of duplicating logic.
Calling `vehicle_manager.get_firmware_vehicle_type()` here would centralize firmware-type resolution in one place and avoid this mapping logic drifting from the shared implementation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
228
to
231
| if self._firmware_vehicle_type is None: | ||
| try: | ||
| vehicle_type = await self.vehicle_manager.get_vehicle_type() | ||
| self._firmware_vehicle_type = vehicle_type.mavlink_firmware_type() |
There was a problem hiding this comment.
suggestion: Reuse VehicleManager.get_firmware_vehicle_type instead of duplicating logic.
Calling vehicle_manager.get_firmware_vehicle_type() here would centralize firmware-type resolution in one place and avoid this mapping logic drifting from the shared implementation.
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
patrickelectric
force-pushed
the
reboot-only-rov
branch
from
dfc8dea to
0f410ae
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Limit automatic Ardupilot restarts on heartbeat loss to ROV (ArduSub) vehicles and introduce a typed MAVLink firmware classification.
New Features:
Bug Fixes:
Enhancements: