Skip to content

AP_DroneCAN: fix Himark servo telemetry index - #34586

Merged
IamPete1 merged 1 commit into
ArduPilot:masterfrom
CleiK:fix-himark-servo-telem-index
Oct 2, 2026
Merged

IamPete1 merged 1 commit into
ArduPilot:masterfrom
CleiK:fix-himark-servo-telem-index

Conversation

@CleiK

@CleiK CleiK commented Oct 2, 2026

Copy link
Copy Markdown

Summary

Fix Himark DroneCAN servo telemetry (CSRV) being logged one channel too low, with the servo on SERVO1 missing entirely, since 4.7.0.

Classification & Testing (check all that apply and add your own)

  • Checked by a human programmer
  • Non-functional change
  • No-binary change
  • Infrastructure change (e.g. unit tests, helper scripts)
  • Automated test(s) verify changes (e.g. unit test, autotest)
  • Tested manually, description below (e.g. SITL)
  • Tested on hardware
  • Logs attached
  • Logs available on request

Built for CubeOrangePlus (Plane).

Hardware: CubeOrangePlus with 8 Himark DroneCAN servos on SERVO1-8, CAN_D1_UC_OPTION bit 6 (UseHimarkServo) set.

  • The DroneCAN GUI bus monitor shows the servo on SERVO1 sending com.himark.servo.ServoInfo with servo_id = 0, SERVO2 with servo_id = 1
  • Before: CSRV[0..6] hold servos 2..8, and servo 1 is absent.
  • After: CSRV[0..7] hold servos 1..8.

Description

Himark ServoInfo.servo_id is zero indexed: it matches the servo's position in the com.himark.servo.ServoCmd array, which SRV_send_himark() fills starting with SERVO1 at cmd.data[0]. The DSDL also documents it as "servo ID from 0 to 17".

#31577 (880b978) changed the servo telemetry handlers to id - 1 because uavcan.equipment.actuator.Status.actuator_id is one indexed. The same change was applied to handle_himark_servoinfo(), which shifted every Himark servo's telemetry down by one channel. For servo_id = 0, the index wraps to 255, AP_Servo_Telem::update_telem_data() rejects it, and that servo's telemetry is never logged.

This PR uses servo_id directly. The actuator Status and Volz handlers are unchanged.

Affects 4.7.0 and 4.7.1, so this is a backport candidate for 4.7.

The Himark ServoInfo servo_id is zero indexed: it matches the servo
position in the com.himark.servo.ServoCmd array, where cmd.data[0] is
SERVO1. Subtracting one shifted the telemetry of every Himark servo down
by one channel and dropped the servo with servo_id 0, whose index
wrapped to 255.

The one indexing was introduced for uavcan.equipment.actuator.Status,
whose actuator_id is one indexed, and was applied to Himark by mistake.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@IamPete1 IamPete1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is correct.

@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown

Global Size Summary (Total Flash delta in bytes)

Built from 4a95837 rebased onto cafe674

Board Periph tracker blimp bootloader copter heli iofw plane rover sub
DrotekP3Pro _ _ _ * _ _ _ _ _ _
Durandal _ -4 -4 _ -4 -4 _ -4 -4 -4
Hitec-Airspeed * _ _ _ _ _ _ _ _ _
KakuteF7 _ * * _ * * _ * * *
MambaH743v4 _ * * _ * * _ * * *
MatekF405-CAN _ _ _ * _ _ _ _ _ _
MatekF405 _ * * _ * * _ * * *
MatekH743-bdshot _ -4 -4 _ -4 -4 _ -4 -4 -4
Pixhawk1-1M _ * * _ * * _ * * *
disco _ * * _ * * _ * * *
f103-GPS * _ _ _ _ _ _ _ _ _

@AP-Review

Copy link
Copy Markdown

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.
Verdict: ACCEPT

Reviewed at head 4a958370bc.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/ardupilot/34586/1.html#pr34586

Thanks, this looks right. The Himark ServoInfo servo_id is zero-based: the DSDL gives 0 to 17, and SRV_send_himark sends SERVO1 in cmd.data[0]. So storing telemetry at msg.servo_id (AP_DroneCAN.cpp:1494) fixes the shift down by one channel, and stops servo 0 being dropped through the uint8 wrap to 255. The - 1 came from the actuator_id change in 880b978, where the IDs really are one-based. The actuator and Volz handlers keep their - 1, which is still correct. A host-side regression test of the handler passes for all servo IDs at this head and fails on the old code for ID 0. I could not test with real Himark servos or on a live CAN bus.

@IamPete1
IamPete1 merged commit e204ca7 into ArduPilot:master Oct 2, 2026
107 checks passed
@IamPete1

IamPete1 commented Oct 2, 2026

Copy link
Copy Markdown
Member

Thanks!

@CleiK
CleiK deleted the fix-himark-servo-telem-index branch October 2, 2026 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants