Added flow control for MAVLink telemetry #14651
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (6)
You can disable this status message by setting the WalkthroughReworks MAVLink core headers (pragma once, optional C++ Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant ML as MAVLink headers
participant Chan as Channel
participant Peer as Remote
rect rgba(230,240,255,0.6)
note over App,ML: Status-aware packing/encoding
App->>ML: mavlink_msg_*_pack_status(..., _status, msg)
ML->>ML: assemble payload (byte-swap/alignment or struct path)
ML-->>App: msg (payload, id, len, crc)
end
alt MAVLINK_COMMAND_24BIT enabled
ML->>ML: use 24-bit message-info (id prefix)
else legacy metadata
ML->>ML: use non-24-bit message-info
end
ML->>Chan: mavlink_finalize_message_buffer(msg, MIN_LEN, LEN, CRC)
Chan->>Peer: transmit frame (STX=253)
rect rgba(240,255,240,0.6)
note over Peer,ML: Receive and decode (len-guarded)
Peer->>ML: frame received
ML->>ML: parse, len-guard copy, zero-init remainder if needed
ML-->>Peer: extracted fields / struct via getters
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Suggested labels
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Pre-merge checks and finishing touches (3 passed)✅ Passed checks (3 passed)
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Pre-merge checks and finishing touches (3 passed)✅ Passed checks (3 passed)
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Pre-merge checks and finishing touches (3 passed)✅ Passed checks (3 passed)
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Pre-merge checks and finishing touches (3 passed)✅ Passed checks (3 passed)
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Pre-merge checks and finishing touches (3 passed)✅ Passed checks (3 passed)
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Pre-merge checks and finishing touches (3 passed)✅ Passed checks (3 passed)
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Pre-merge checks and finishing touches (3 passed)✅ Passed checks (3 passed)
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Pre-merge checks and finishing touches (3 passed)✅ Passed checks (3 passed)
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Pre-merge checks and finishing touches (3 passed)✅ Passed checks (3 passed)
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Pre-merge checks and finishing touches (3 passed)✅ Passed checks (3 passed)
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Pre-merge checks and finishing touches (3 passed)✅ Passed checks (3 passed)
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Pre-merge checks and finishing touches (3 passed)✅ Passed checks (3 passed)
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Pre-merge checks and finishing touches (3 passed)✅ Passed checks (3 passed)
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Pre-merge checks and finishing touches (3 passed)✅ Passed checks (3 passed)
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Pre-merge checks and finishing touches (3 passed)✅ Passed checks (3 passed)
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Pre-merge checks and finishing touches (3 passed)✅ Passed checks (3 passed)
Pre-merge checks and finishing touches (3 passed)✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Do you want to test this code? You can flash it directly from Betaflight Configurator:
WARNING: It may be unstable. Use only for testing! |
|
@haslinghuis |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (47)
lib/main/MAVLink/common/mavlink_msg_available_modes_monitor.h (4)
1-1: Add an autogenerated preamble to discourage manual edits.Generated MAVLink headers typically start with a “DO NOT EDIT – autogenerated by mavgen” preamble. Adding it avoids accidental manual changes and clarifies source of truth (XML + generator).
Apply:
+// Auto-generated by mavgen (MAVLink v2, common.xml). DO NOT EDIT. +// To change this file: modify the XML and regenerate the headers. #pragma once
7-9: Clarify seq wrap/initialization semantics in the XML.The doc doesn’t state whether seq wraps at 255 and what the initial value is. Please update the XML comment (preferred) so consumers know reset/wrap behavior.
I can propose XML wording if you confirm intended semantics.
251-258: Zero the whole struct in the aligned decode path (future-proof for extensions).Minor safety improvement: zero the full struct rather than only the base length. No functional change here, but robust if the struct gains extension fields later.
- memset(available_modes_monitor, 0, MAVLINK_MSG_ID_AVAILABLE_MODES_MONITOR_LEN); + memset(available_modes_monitor, 0, sizeof(*available_modes_monitor));Note: If this header is generated, prefer applying this in the generator template.
192-199: Doc nit: “@param struct” uses a reserved word and doesn’t match the parameter.Change “@param struct” to “@param available_modes_monitor” to avoid confusion.
- * @param struct The MAVLink struct to serialize + * @param available_modes_monitor The MAVLink struct to serialize(If generated, fix in the template.)
lib/main/MAVLink/checksum.h (1)
3-7: Tighten conditional closing for readabilityThe open/close macros are functionally correct, but mirroring the opening
#if/#elifin the closing block improves maintainability and avoids future mismatches.-#if defined(MAVLINK_USE_CXX_NAMESPACE) || defined(__cplusplus) -} -#endif +#if defined(MAVLINK_USE_CXX_NAMESPACE) +} // namespace mavlink +#elif defined(__cplusplus) +} // extern "C" +#endifAlso applies to: 93-95
lib/main/MAVLink/common/mavlink_msg_command_long.h (1)
126-146: Fix param name in docs to match signatureThe doc block says
@param statusbut the function parameter is_status. Align them to avoid confusion.- * @param status MAVLink status structure + * @param _status MAVLink status structureAlso applies to: 181-186
lib/main/MAVLink/common/mavlink_msg_cellular_status.h (2)
12-14: Fix repeated typo “in in” and clarify phrasingMultiple doc strings read “status in in …”. Suggest “status is in …”.
- uint8_t failure_reason; /*< Failure reason when status in in CELLULAR_STATUS_FLAG_FAILED*/ + uint8_t failure_reason; /*< Failure reason when status is in CELLULAR_STATUS_FLAG_FAILED*/ @@ - * @param failure_reason Failure reason when status in in CELLULAR_STATUS_FLAG_FAILED + * @param failure_reason Failure reason when status is in CELLULAR_STATUS_FLAG_FAILED @@ - * @param failure_reason Failure reason when status in in CELLULAR_STATUS_FLAG_FAILED + * @param failure_reason Failure reason when status is in CELLULAR_STATUS_FLAG_FAILED @@ - * @param failure_reason Failure reason when status in in CELLULAR_STATUS_FLAG_FAILED + * @param failure_reason Failure reason when status is in CELLULAR_STATUS_FLAG_FAILED @@ - * @return Failure reason when status in in CELLULAR_STATUS_FLAG_FAILED + * @return Failure reason when status is in CELLULAR_STATUS_FLAG_FAILEDAlso applies to: 62-69, 109-117, 158-167, 245-252, 350-355
102-120: Doc param name for status structSame nit as COMMAND_LONG: doc uses
@param statusbut the argument is_status. Align docs.- * @param status MAVLink status structure + * @param _status MAVLink status structureAlso applies to: 145-151
lib/main/MAVLink/common/mavlink_msg_battery_status.h (1)
166-202: Unify array assignment helpers for consistency.pack_status uses mav_array_memcpy while pack/chan use mav_array_assign_uint16_t. Prefer one style.
Apply:
- mav_array_memcpy(packet.voltages, voltages, sizeof(uint16_t)*10); - mav_array_memcpy(packet.voltages_ext, voltages_ext, sizeof(uint16_t)*4); + mav_array_assign_uint16_t(packet.voltages, voltages, 10); + mav_array_assign_uint16_t(packet.voltages_ext, voltages_ext, 4);lib/main/MAVLink/common/mavlink_msg_battery_info.h (2)
23-23: Doc nit: uint8_t cannot be “-1”.Clarify sentinel to 255 in comments to avoid confusion.
- uint8_t state_of_health; /*< [%] State of Health (SOH) estimate. Typically 100% at the time of manufacture and will decrease over time and use. -1: field not provided.*/ + uint8_t state_of_health; /*< [%] State of Health (SOH) estimate. Typically 100% at the time of manufacture and will decrease over time and use. 255: field not provided.*/- * @return [%] State of Health (SOH) estimate. Typically 100% at the time of manufacture and will decrease over time and use. -1: field not provided. + * @return [%] State of Health (SOH) estimate. Typically 100% at the time of manufacture and will decrease over time and use. 255: field not provided.Also applies to: 583-587
209-267: Unify string array helpers.pack_status uses mav_array_memcpy for char arrays while other paths use mav_array_assign_char. Prefer a single helper for consistency and safer padding.
- mav_array_memcpy(packet.manufacture_date, manufacture_date, sizeof(char)*9); - mav_array_memcpy(packet.serial_number, serial_number, sizeof(char)*32); - mav_array_memcpy(packet.name, name, sizeof(char)*50); + mav_array_assign_char(packet.manufacture_date, manufacture_date, 9); + mav_array_assign_char(packet.serial_number, serial_number, 32); + mav_array_assign_char(packet.name, name, 50);lib/main/MAVLink/common/mavlink.h (1)
9-10: Make XML hash literal explicitly 64-bit to avoid compiler warningsSome toolchains warn on large unsuffixed integer literals. Consider adding an LL suffix.
-#define MAVLINK_PRIMARY_XML_HASH -8828250664837709582 +#define MAVLINK_PRIMARY_XML_HASH -8828250664837709582LLlib/main/MAVLink/common/mavlink_msg_camera_trigger.h (1)
207-214: Doc nit: parameter name in comment“@param struct The MAVLink struct to serialize” should name the parameter “camera_trigger” for clarity. If these headers are vendored/generated, consider fixing upstream rather than hand-editing locally.
- * @param struct The MAVLink struct to serialize + * @param camera_trigger The MAVLink struct to serializelib/main/MAVLink/common/mavlink_msg_attitude_target.h (2)
13-14: Typo in docs: “reverse trust” → “reverse thrust” (optional, upstream)Purely a doc nit; if these headers are generated, prefer fixing upstream to avoid divergence.
- * @param thrust Collective thrust, normalized to 0 .. 1 (-1 .. 1 for vehicles capable of reverse trust) + * @param thrust Collective thrust, normalized to 0 .. 1 (-1 .. 1 for vehicles capable of reverse thrust)Also applies to: 62-69, 107-114, 155-162, 239-246, 390-395
120-139: Minor inconsistency: assign vs memcpy macros across branchespack_status() uses mav_array_memcpy while pack()/pack_chan()/send() used assign. After applying the replacements above, this will be consistent.
lib/main/MAVLink/common/mavlink_msg_change_operator_control_ack.h (1)
7-11: Minor doc nits (optional, upstream)
- “ID of the GCS this message ” is incomplete; consider “ID of the GCS for this message”.
- Trailing spaces in several doc lines.
If vendored/generated, prefer upstream fixes.- uint8_t gcs_system_id; /*< ID of the GCS this message */ + uint8_t gcs_system_id; /*< ID of the GCS for this message */Also applies to: 50-53, 197-200, 270-297
lib/main/MAVLink/common/mavlink_msg_auth_key.h (2)
85-89: Unify char-array copy helper for consistency.Use the same helper as other aligned-field paths to reduce macro surface area and avoid build-time surprises on platforms missing one of the helpers.
Apply:
- mav_array_memcpy(packet.key, key, sizeof(char)*32); + mav_array_assign_char(packet.key, key, 32);Note: If these headers are auto-generated, consider fixing in the generator rather than hand-editing.
38-46: Minor wording nit in Doxygen briefs.“Pack a auth_key message” → “Pack an auth_key message” for grammar.
Also applies to: 66-75, 99-107, 169-175, 192-197
lib/main/MAVLink/common/mavlink_msg_change_operator_control.h (2)
106-112: Prefer a single char-array helper across codepaths.Align with other functions using assign helper for clarity and portability.
Apply:
- mav_array_memcpy(packet.passkey, passkey, sizeof(char)*25); + mav_array_assign_char(packet.passkey, passkey, 25);If generated, amend the generator.
10-11: Docs nits: unit tag and null-termination wording.
- “[rad]” before “version” is misleading (version is unitless).
- Comment promises “NULL terminated,” but wire format is fixed 25 bytes; null-termination is not guaranteed if fully filled.
Recommend:
- Drop the “[rad]” tag from “version” docs.
- Reword passkey description to “Up to 25 bytes; null-terminated if shorter, otherwise padded/truncated per MAVLink.”
Also applies to: 53-57, 89-93, 128-132, 199-207, 309-314
lib/main/MAVLink/common/mavlink_msg_camera_fov_status.h (1)
88-101: Document or assert that q must be non-NULL
All internal call sites supply a valid pointer, but the pack/send APIs assumeqis non-NULL; add a note in the header or anassert(q)in debug builds to catch misuse.lib/main/MAVLink/common/mavlink_msg_camera_image_captured.h (1)
103-103: Potential OOB read from file_url caller buffer.
_mav_put_char_array(..., file_url, 205)will memcpy 205 bytes fromfile_url. Ensure all call-sites pass a buffer of at least 205 bytes (including any terminator) to avoid UB.I can help audit Betaflight MAVLink send paths for callers that pass shorter C-strings if you want a quick scan.
Consider a local wrapper at call sites:
char tmp[205] = {0}; size_t n = strnlen(file_url, sizeof(tmp)); memcpy(tmp, file_url, n); // pass tmp to pack/sendAlso applies to: 160-160, 221-221, 371-371
lib/main/MAVLink/common/mavlink_msg_adsb_vehicle.h (1)
112-112: Callsign copy reads 9 bytes unconditionally.Ensure callers pass a 9-byte buffer (8 chars + NUL). Passing a shorter C-string pointer risks over-read.
Use the same call-site wrapper approach as noted for
file_url.If helpful, I can scan for
mavlink_msg_adsb_vehicle_*producers to confirm argument sources.Also applies to: 175-175, 242-242, 343-343, 402-402
lib/main/MAVLink/common/mavlink_msg_command_ack.h (1)
240-266: Optional: only set progress when IN_PROGRESS.At call-sites, populate
progress(0–100 or 255) only whenresult == MAV_RESULT_IN_PROGRESS; otherwise set to 0/255 consistently.I can propose a tiny helper to centralize ACK creation if desired.
lib/main/MAVLink/common/mavlink_msg_command_int.h (1)
372-384: Doc nit: parameter name “struct”“@param struct” is confusing and clashes with the C keyword. Rename in the comment for clarity.
- * @param struct The MAVLink struct to serialize + * @param command_int The MAVLink struct to serializelib/main/MAVLink/common/mavlink_msg_att_pos_mocap.h (1)
7-14: Payload growth to 120 bytes: consider bandwidth impactAdding covariance expands this message from 36 to 120 bytes. With flow control tied to TX buffer headroom, this can increase backpressure. Consider gating the covariance field behind a runtime option, or suppressing it when TX buffer is near the mavlink_min_txbuff threshold.
Also applies to: 16-19
lib/main/MAVLink/common/mavlink_msg_cellular_config.h (3)
8-15: Treat PIN/PUK/APN as sensitive; avoid logging and dumpsThese fields can contain secrets. Ensure upstream telemetry/CLI logs, blackbox, and debug dumps redact these fields, and that core dumps are disabled or filtered in release builds.
297-304: Doc nit: parameter name “struct”Same doc wording issue as elsewhere; use a descriptive name.
- * @param struct The MAVLink struct to serialize + * @param cellular_config The MAVLink struct to serialize
78-107: No unsafe uses of mavlink_msg_cellular_config_pack/send detected
All existing callers (in testsuite.h) pass fixed-size char arrays, so there’s no over-read in the current code. Optional: for library consumers who pass C-strings, consider adding a helper to pad/truncate into the 16/32-byte fields (e.g.bf_str_to_field) before packing.lib/main/MAVLink/common/mavlink_msg_ais_vessel.h (1)
132-133: Replace magic numbers with field-length macros (nit).Use MAVLINK_MSG_AIS_VESSEL_FIELD_CALLSIGN_LEN and _NAME_LEN to improve readability and reduce drift if definitions change.
- _mav_put_char_array(buf, 31, callsign, 7); - _mav_put_char_array(buf, 38, name, 20); + _mav_put_char_array(buf, 31, callsign, MAVLINK_MSG_AIS_VESSEL_FIELD_CALLSIGN_LEN); + _mav_put_char_array(buf, 38, name, MAVLINK_MSG_AIS_VESSEL_FIELD_NAME_LEN);(repeat similarly in other pack/send/get helpers)
Also applies to: 152-153, 207-208, 227-228, 286-287, 307-308, 399-401, 419-421, 466-468, 487-488, 635-636, 645-646
lib/main/MAVLink/common/mavlink_msg_can_filter_modify.h (1)
77-77: Prefer the provided length macro (nit).Replace hard-coded 16 with MAVLINK_MSG_CAN_FILTER_MODIFY_FIELD_IDS_LEN for clarity.
- _mav_put_uint16_t_array(buf, 0, ids, 16); + _mav_put_uint16_t_array(buf, 0, ids, MAVLINK_MSG_CAN_FILTER_MODIFY_FIELD_IDS_LEN);(similar in assign/memcpy and getters)
Also applies to: 86-86, 119-119, 128-128, 165-165, 174-174, 246-246, 254-255, 290-291, 299-300, 367-368
lib/main/MAVLink/common/mavlink_msg_canfd_frame.h (2)
77-77: Replace magic 64 with the field macro (nit).Use MAVLINK_MSG_CANFD_FRAME_FIELD_DATA_LEN for consistency.
- _mav_put_uint8_t_array(buf, 8, data, 64); + _mav_put_uint8_t_array(buf, 8, data, MAVLINK_MSG_CANFD_FRAME_FIELD_DATA_LEN);(apply similarly across pack/send/get helpers)
Also applies to: 86-86, 119-119, 128-128, 165-165, 174-175, 245-246, 254-255, 290-291, 299-300, 365-367
16-18: Integration note: larger payloads and new flow-control.CANFD_FRAME (72B) increases burst sizes; ensure the new MAVLink telemetry flow control (mavlink_min_txbuff) accounts for these when computing safe resume thresholds.
lib/main/MAVLink/common/mavlink_msg_button_change.h (1)
7-11: Pack the struct to avoid padding issues in send_struct paths.On some compilers/ABIs the struct may get trailing padding; packing keeps field offsets stable when using the aligned-fields direct-copy path.
-typedef struct __mavlink_button_change_t { +MAVPACKED( +typedef struct __mavlink_button_change_t { uint32_t time_boot_ms; /*< [ms] Timestamp (time since system boot).*/ uint32_t last_change_ms; /*< [ms] Time of last change of button state.*/ uint8_t state; /*< Bitmap for state of buttons.*/ -} mavlink_button_change_t; +}) mavlink_button_change_t;lib/main/MAVLink/common/mavlink_msg_attitude_quaternion_cov.h (2)
7-14: Pack the struct to guarantee layout for aligned-fields direct copy.-typedef struct __mavlink_attitude_quaternion_cov_t { +MAVPACKED( +typedef struct __mavlink_attitude_quaternion_cov_t { uint64_t time_usec; /* ... */ float q[4]; /* ... */ float rollspeed; /* ... */ float pitchspeed; /* ... */ float yawspeed; /* ... */ float covariance[9]; /* ... */ -} mavlink_attitude_quaternion_cov_t; +}) mavlink_attitude_quaternion_cov_t;
123-131: Use consistent array-copy helper.Other pack variants use mav_array_assign_float; align for consistency.
- mav_array_memcpy(packet.q, q, sizeof(float)*4); - mav_array_memcpy(packet.covariance, covariance, sizeof(float)*9); + mav_array_assign_float(packet.q, q, 4); + mav_array_assign_float(packet.covariance, covariance, 9);lib/main/MAVLink/common/mavlink_msg_camera_settings.h (1)
10-12: Nit: minor doc wording/spacing.Optional: replace double spaces and clarify “0 means MAVLink camera” phrasing for consistency.
- uint8_t camera_device_id; /*< Camera id of a non-MAVLink camera attached to an autopilot (1-6). 0 if the component is a MAVLink camera (with its own component id).*/ + uint8_t camera_device_id; /*< Camera id of a non-MAVLink camera attached to an autopilot (1–6). Use 0 if the component is a MAVLink camera (with its own component id). */Also applies to: 33-34, 58-60, 99-101, 223-225, 336-341
lib/main/MAVLink/common/mavlink_msg_actuator_output_status.h (1)
58-63: Null-safety for actuator array.APIs assume actuator != NULL. If any caller intends to omit/zero it, ensure a zero[32] buffer is passed. Don’t hand-edit this header; enforce at call sites or via generator templates.
Example at call sites:
static const float kZeros32[32] = {0}; mavlink_msg_actuator_output_status_send(chan, t, active, kZeros32);Also applies to: 199-204
lib/main/MAVLink/common/mavlink_msg_attitude_quaternion.h (1)
92-93: Null-safety for repr_offset_q.APIs assume repr_offset_q != NULL. If a caller wants “field not supported”, pass {0,0,0,0}. Don’t modify this header; enforce at call sites or in generator.
Example:
static const float kZeros4[4] = {0}; mavlink_msg_attitude_quaternion_send(chan, t, q1, q2, q3, q4, rs, ps, ys, kZeros4);Also applies to: 104-105, 143-144, 155-156, 198-199, 210-211, 287-289, 299-301, 338-340, 350-352
lib/main/MAVLink/common/mavlink_msg_camera_thermal_range.h (1)
312-318: Fix doc param name (“struct” placeholder).
Minor doc nit: replace “@param struct” with the actual parameter name.- * @param struct The MAVLink struct to serialize + * @param camera_thermal_range The MAVLink struct to serializelib/main/MAVLink/common/mavlink_msg_can_frame.h (1)
236-247: Clarify/data-length handling for CAN data payload.
The fixed 8-byte copy ignoreslen. That’s normal for MAVLink fixed arrays, but make sure all call sites only pass buffers with at least 8 valid bytes and clamplen <= 8for classical CAN.If desired, add a light-weight runtime guard in callers (not here, since these headers are generated) to enforce
len <= MAVLINK_MSG_CAN_FRAME_FIELD_DATA_LEN.Also applies to: 281-292, 365-390
lib/main/MAVLink/common/mavlink_msg_altitude.h (1)
56-60: Grammar nit: “an altitude message.”
Tiny doc fix; keeps comments clean.- * @brief Pack a altitude message + * @brief Pack an altitude message- * @brief Pack a altitude message + * @brief Pack an altitude message- * @brief Pack a altitude message on a channel + * @brief Pack an altitude message on a channelAlso applies to: 102-110, 153-159
lib/main/MAVLink/common/mavlink_msg_available_modes.h (2)
122-129: Unify char-array copy macro for mode_name.Else path uses mav_array_memcpy while other paths use mav_array_assign_char. Align for consistency (and safer termination semantics if provided by the macro).
- mav_array_memcpy(packet.mode_name, mode_name, sizeof(char)*35); + mav_array_assign_char(packet.mode_name, mode_name, 35);If these headers are generated, consider fixing it in the generator to avoid local drift.
77-77: Prefer using the provided field-length macro instead of hardcoded 35.Use MAVLINK_MSG_AVAILABLE_MODES_FIELD_MODE_NAME_LEN for maintainability.
Also applies to: 86-86, 119-119, 128-128, 165-165, 174-174, 245-245, 254-254, 290-290, 299-299, 367-367
lib/main/MAVLink/common/mavlink_msg_camera_tracking_geo_status.h (1)
591-594: Guard access to extension field getters.camera_device_id is beyond MIN_LEN (offset 49). Callers using the getter should ensure msg->len >= 50 or use the decode() path (which zero-fills missing extension fields).
If helpful, I can provide a tiny wrapper that checks msg->len before reading extension bytes.
lib/main/MAVLink/common/mavlink_msg_camera_tracking_image_status.h (1)
510-513: Add a length check or use decode() for extension‐field getters
camera_device_id is at offset 31 (beyond MIN_LEN 31); guardmsg->len >= MAVLINK_MSG_ID_CAMERA_TRACKING_IMAGE_STATUS_LENbefore calling the getter or prefermavlink_msg_camera_tracking_image_status_decode()for safe access.lib/main/MAVLink/common/mavlink_msg_autopilot_state_for_gimbal_device.h (1)
178-193: Use consistent array-copy helper for q.Elsewhere in this file you use mav_array_assign_float for q; here it switches to mav_array_memcpy. For consistency/readability, prefer one helper.
Apply this diff:
- mav_array_memcpy(packet.q, q, sizeof(float)*4); + mav_array_assign_float(packet.q, q, 4);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
lib/main/MAVLink/checksum.h(6 hunks)lib/main/MAVLink/common/mavlink.h(2 hunks)lib/main/MAVLink/common/mavlink_msg_actuator_control_target.h(7 hunks)lib/main/MAVLink/common/mavlink_msg_actuator_output_status.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_adsb_vehicle.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_ais_vessel.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_altitude.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_att_pos_mocap.h(6 hunks)lib/main/MAVLink/common/mavlink_msg_attitude.h(8 hunks)lib/main/MAVLink/common/mavlink_msg_attitude_quaternion.h(6 hunks)lib/main/MAVLink/common/mavlink_msg_attitude_quaternion_cov.h(6 hunks)lib/main/MAVLink/common/mavlink_msg_attitude_target.h(7 hunks)lib/main/MAVLink/common/mavlink_msg_auth_key.h(7 hunks)lib/main/MAVLink/common/mavlink_msg_autopilot_state_for_gimbal_device.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_autopilot_version.h(0 hunks)lib/main/MAVLink/common/mavlink_msg_available_modes.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_available_modes_monitor.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_battery_info.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_battery_status.h(6 hunks)lib/main/MAVLink/common/mavlink_msg_button_change.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_camera_capture_status.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_camera_fov_status.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_camera_image_captured.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_camera_information.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_camera_settings.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_camera_thermal_range.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_camera_tracking_geo_status.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_camera_tracking_image_status.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_camera_trigger.h(7 hunks)lib/main/MAVLink/common/mavlink_msg_can_filter_modify.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_can_frame.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_canfd_frame.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_cellular_config.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_cellular_status.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_change_operator_control.h(7 hunks)lib/main/MAVLink/common/mavlink_msg_change_operator_control_ack.h(7 hunks)lib/main/MAVLink/common/mavlink_msg_collision.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_command_ack.h(6 hunks)lib/main/MAVLink/common/mavlink_msg_command_cancel.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_command_int.h(7 hunks)lib/main/MAVLink/common/mavlink_msg_command_long.h(7 hunks)
💤 Files with no reviewable changes (1)
- lib/main/MAVLink/common/mavlink_msg_autopilot_version.h
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ledvinap
PR: betaflight/betaflight#14578
File: src/main/telemetry/jetiexbus.c:547-556
Timestamp: 2025-08-12T18:26:17.016Z
Learning: For Betaflight Jeti ExBus backport PR #14578: ledvinap prefers to keep the PR focused on backporting fixes from iNavFlight rather than expanding into broader architectural improvements. Additional improvements to TX/RX state machine should be handled in separate PRs.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI / Build (STM32H723)
🔇 Additional comments (97)
lib/main/MAVLink/common/mavlink_msg_available_modes_monitor.h (3)
21-36: Confirm MAVLink v2 is enforced where this header is used.Message ID 437 (>255) requires v2 frames. Ensure build flags/telemetry path cannot slip into MAVLink v1.
Would you like a quick repo-wide check script to surface MAVLink v2 defines/usages on telemetry builds?
47-64: LGTM: Pack/encode/send/get paths match mavgen patterns.Byte-swap/alignment branches, finalize calls, and convenience variants look correct for a 1-byte payload.
Also applies to: 76-97, 108-126, 136-167, 175-190, 197-204, 206-229, 240-243
16-17: Verify CRC extra and ID uniqueness
- No collisions for message ID 437 found in
lib/main/MAVLink/common.- Upstream
pymavlink.dialects.v20.commondoesn’t expose CRC mappings—manually confirm that the common dialect XML defines CRC extra 30 for message 437.lib/main/MAVLink/checksum.h (3)
1-1: LGTM on pragma-once swapSwitching to
#pragma onceis fine for this repo and compilers we target.
27-35: Vendored MAVLink headers unchanged from upstream
Script output confirms checksum.h (and related mavlink_helpers.h, mavlink_types.h, mavlink_sha256.h) still useX25_INIT_CRC/X25_VALIDATE_CRCalongsideCRC16_MCRF4XXdocs exactly as in the upstream generator, with no local-only edits detected.
3-7: Namespace opt-in safe: no C++ TU calls CRC functions
No translation unit compiled as C++ invokes crc_accumulate/crc_init/crc_calculate/crc_accumulate_buffer, so opting into mavlink:: namespace won’t break linkage.lib/main/MAVLink/common/mavlink_msg_command_long.h (3)
31-49: Message info layout/offsets match MAVLink v2 COMMAND_LONGField ordering and offsets (param1..7 at 0..24, command at 28, target_system 30, target_component 31, confirmation 32) look correct, and CRC 152 aligns with spec.
Also applies to: 50-66
307-341: Convenience senders: OKThe convenience send paths mirror the packers and use correct min/len/CRC. No issues spotted.
Also applies to: 356-399
521-540: Decode path: correct zero-fill and bounded copyThe safe copy with zero-initialization for short frames is correct and prevents reading uninitialized bytes.
lib/main/MAVLink/common/mavlink_msg_cellular_status.h (2)
7-15: New message definition looks correctPayload size (10), offsets, and CRC (72) match the upstream definition for CELLULAR_STATUS (ID 334).
Also applies to: 17-24
71-100: Pack/encode/send/decode paths are consistentByte-swap/alignment branches, finalize calls, and getters are all consistent with generator patterns.
Also applies to: 118-151, 168-198, 208-226, 236-240, 255-281, 296-330, 413-428
lib/main/MAVLink/common/mavlink_msg_battery_status.h (2)
6-22: Struct layout/offsets look consistent with LEN/MIN_LEN.Field ordering matches documented offsets; 54/36 lengths align with extension semantics.
24-33: Approved: CRC and MIN_LEN match upstream. CRC_EXTRA remains 154 and base payload MIN_LEN 36 as per common.xml, and full LEN 54 covers the MAVLink 2 extension fields. (docs.ros.org)lib/main/MAVLink/common/mavlink_msg_command_cancel.h (2)
7-12: Message definition is minimal and correct-sized.4-byte payload and field offsets (0,2,3) are coherent.
23-41: Please compute the CRC_EXTRA for MESSAGE ID 80 (COMMAND_CANCEL) directly from the upstream common.xml now so we can confirm the correct CRC matches our MESSAGE_INFO.lib/main/MAVLink/common/mavlink_msg_battery_info.h (3)
7-28: Layout/offsets align with 140-byte payload.Offsets and lengths for numeric fields and strings look consistent.
420-447: Caller contract for fixed-size strings._mav_put_char_array copies exact lengths; callers must pass buffers of at least the declared sizes (9/32/50) with zero padding. If we ever source these from shorter C strings, ensure local zero-padded staging buffers.
Would you like a small helper to wrap/zero-pad these three fields before send/pack in Betaflight’s MAVLink emitters?
35-37: Stop CRC_EXTRA sanity check against common.xml
Upstream common.xml (v1.0) contains no<crc_extra>entries for BATTERY_INFO; CRC 26 is generated by mavgen—verify against the MAVLink CRC extra table instead.Likely an incorrect or invalid review comment.
lib/main/MAVLink/common/mavlink.h (2)
27-29: 24-bit command support enable looks correctCentral toggle is set; dependent message headers align with this path. No issues spotted.
11-13: Verify consumer support for MAVLink v2 framing
Default STX is now 253 in common/standard/minimal headers; runtime parsing still accepts v1 (0xFE) and v2 (0xFD), and no compile-time overrides for MAVLink version were found. Confirm all downstream radios/GCS/tools accept v2 frames; if mixed v1/v2 environments exist, implement a compile-time or runtime switch or per-channel version negotiation.lib/main/MAVLink/common/mavlink_msg_attitude.h (2)
27-55: 24-bit MAVLINK_MESSAGE_INFO variant is consistentID-first 24BIT path and classic path both look correct; field offsets/CRC match LEN=28.
118-151: Status-aware pack/finalize path LGTMpack_status uses mavlink_finalize_message_buffer with CRC extra; aligned and byte-swap branches mirror pack(). Good.
lib/main/MAVLink/common/mavlink_msg_actuator_control_target.h (1)
13-17: Field layout/offsets/lengths look consistentLEN/MIN_LEN=41, offsets: time_usec@0, controls@8, group_mlx@40. Getters/setters align; CRC=181 matches spec.
Also applies to: 23-41, 119-127, 260-287, 302-305
lib/main/MAVLink/common/mavlink_msg_camera_trigger.h (4)
22-39: 24-bit MESSAGE_INFO variant and MIN_LEN macros look correctNumeric ID, field offsets, and MIN_LEN values align with the payload layout (time_usec@0, seq@8, LEN=12). No issues spotted.
51-70: Finalize path uses MIN_LEN consistentlyUsing MAVLINK_MSG_ID_CAMERA_TRIGGER_MIN_LEN in finalize_message is correct and guards short-frame cases.
83-106: Status-aware pack helper is consistent with buffer finalizationThe status-based variant mirrors the pack path and uses mavlink_finalize_message_buffer. Looks good.
280-287: Safer decode pathZero-initialization and length-capped memcpy prevent over-reads. Good defensive change.
lib/main/MAVLink/common/mavlink_msg_attitude_target.h (5)
27-54: 24-bit MESSAGE_INFO and offsets look alignedID=83, field count 7, offsets {time_boot_ms@0, q@4, body_*@20/24/28, thrust@32, type_mask@36} match the struct and pack layout. CRC 22 and LEN/MIN_LEN=37 are consistent.
405-418: Decode safety improvements are goodZeroing and length-capped copy are correct and symmetric with the getters in the byte-swap path.
71-99: General: finalize paths and MIN_LEN usage look correctUse of *_MIN_LEN with finalize_message(_chan/_buffer) is consistent and correct for truncated payloads.
Also applies to: 116-147, 164-192, 202-206, 216-219, 230-233, 249-272, 296-319, 330-395
1-5: Sanity check: public IDs and CRCMAVLINK_MSG_ID_ATTITUDE_TARGET=83 and CRC=22 match upstream definitions for this message.
1-324: mav_array_assign_float macro verified: It’s defined in lib/main/MAVLink/protocol.h (line 205) and used throughout via mav_array_memcpy; no missing definitions, builds will succeed.lib/main/MAVLink/common/mavlink_msg_change_operator_control_ack.h (4)
23-42: 24-bit MESSAGE_INFO variant and constants are consistentID=6, LEN/MIN_LEN=3, CRC=104 all line up with the payload layout.
55-76: Pack/finalize paths use MIN_LEN and CRC correctlyBoth regular and status-aware flows look correct.
Also applies to: 90-115, 128-150
203-221: Convenience send path is consistentByte-swap and aligned-field branches mirror pack logic; finalize uses MIN_LEN; no issues.
305-316: Decode path improvement is correctZeroing and length-checked memcpy mirrors other updated headers.
lib/main/MAVLink/common/mavlink_msg_auth_key.h (5)
11-15: MIN_LEN macro addition looks correct.LEN and MIN_LEN both 32 match the single 32-byte field. No issues.
21-36: 24-bit metadata block is consistent.Including the numeric ID first in MAVLINK_COMMAND_24BIT path matches MAVLink v2 expectations.
47-64: Finalize with MIN_LEN/CRC is correct.Switch to MIN_LEN-aware finalize is good; prevents overcounting trimmed payloads.
169-190: Send helpers updated to MIN_LEN/CRC — good.Matches other messages and avoids length mismatches on trimmed frames.
251-260: Length-aware decode is safer.Zero-init + bounded memcpy handles short payloads correctly.
lib/main/MAVLink/common/mavlink_msg_change_operator_control.h (4)
14-18: MIN_LEN addition is correct.28 bytes matches 3×uint8 + 25-char passkey.
59-80: Finalize now uses MIN_LEN/CRC — correct.Prevents issues with trimmed payloads.
210-227: Send path matches packing and finalization — LGTM.
324-334: Decode path is robust.Explicit getters for unaligned and len-bounded memcpy for aligned builds are correct.
lib/main/MAVLink/common/mavlink_msg_camera_fov_status.h (6)
21-28: Lengths/CRC align with field layout.LEN=53, MIN_LEN=52 (extension field camera_device_id) is correct; CRC 22 matches upstream.
31-66: 24-bit and legacy info macros look correct.Field offsets and counts are consistent across both branches.
87-123: Pack + MIN_LEN finalize — LGTM.Offsets and ordering are correct; q[4] placed at 28, scalar fields follow.
144-183: Status-aware finalize path is correct.Buffer finalize uses MIN_LEN/CRC as expected.
301-333: Convenience send helper mirrors pack — good.
511-530: Decode is safe across builds.Getter-based for unaligned, len-bounded memcpy for aligned builds.
lib/main/MAVLink/common/mavlink_msg_camera_image_captured.h (3)
1-28: LGTM: IDs, lengths, CRCs match upstream MAVLink 2.Message ID 263, LEN/MIN_LEN 255, CRC 133 all look correct. Packing offsets align with field layout.
349-350: MAVLink payload size requirement satisfied:MAVLINK_MAX_PAYLOAD_LENis defined as 255 inlib/main/MAVLink/mavlink_types.h.
116-117: Ignore nonstandard-macro warning
Themav_array_assign_charandmav_array_assign_floathelpers are defined in lib/main/MAVLink/protocol.h, so these calls will compile as‐is—no change needed.Likely an incorrect or invalid review comment.
lib/main/MAVLink/common/mavlink_msg_adsb_vehicle.h (2)
23-31: LGTM: message shape matches MAVLink (ID 246, LEN 38, CRC 184).Offsets/field sizes compute to 38 bytes; CRC and constants look consistent.
128-128: Dropmav_array_assign_charreplacement suggestion
mav_array_assign_charis deliberately defined in protocol.h to use strncpy for safe string fields and is standard in this codebase; replacing it with mav_array_memcpy would change semantics and break consistency. No change needed.Likely an incorrect or invalid review comment.
lib/main/MAVLink/common/mavlink_msg_command_ack.h (2)
16-19: LGTM: COMMAND_ACK extensions wired correctly.MIN_LEN 3 and LEN 10 keep v1 compatibility while exposing v2 extensions (
progress,result_param2,target_*). CRC 143 is correct.
67-69: No downstream call-sites for signature change
Search across the repository found no external calls tomavlink_msg_command_ack_packormavlink_msg_command_ack_sendoutside the vendored MAVLink directory—only internal tests and convenience wrappers withinlib/main/MAVLink. No updates are required in Betaflight code.Likely an incorrect or invalid review comment.
lib/main/MAVLink/common/mavlink_msg_command_int.h (3)
592-595: Good: length-clamped decode protects against short framesZeroing and memcpy with min(msg->len, LEN) prevents over-read. No action needed.
33-52: 24-bit metadata gating is consistently enabled
MAVLINK_COMMAND_24BIT is unconditionally defined to 1 in each dialect’s mavlink.h (common, minimal, standard) and there are no build flags or add_definitions overriding it, so the metadata shape remains stable.
199-205: MAVLINK_CRC_EXTRA is uniformly defined
Verified that each protocol variant header (lib/main/MAVLink/{common,minimal,standard}/mavlink.h) defaultsMAVLINK_CRC_EXTRAto 1, ensuring consistent CRC-extra gating across allmavlink_finalize_message_bufferpaths. No changes needed.lib/main/MAVLink/common/mavlink_msg_att_pos_mocap.h (3)
361-369: Accessor addition looks goodget_covariance() mirrors layout (offset 36, length 21). OK.
81-89: Portability concern resolved
The vendored core’s protocol.h defines mav_array_assign_float as a memcpy wrapper (#define at protocol.h:205), so using it carries no portability risk. Consistent use of mav_array_memcpy directly remains an optional style choice.
68-93: Ignore covariance null-check suggestion
No calls tomavlink_msg_att_pos_mocap_pack(_chan|_send)exist outside the vendor code; the only invocations intestsuite.halways pass a validcovariancearray. The MAVLink spec itself expects unknown covariance to be NaN-initialized—not signaled via a NULL pointer—so there’s no null-dereference risk in the current codebase.Likely an incorrect or invalid review comment.
lib/main/MAVLink/common/mavlink_msg_cellular_config.h (1)
18-25: Metadata/length/CRC values look consistentIDs, LEN/MIN_LEN, and CRC (245) align. No action.
Also applies to: 31-60
lib/main/MAVLink/common/mavlink_msg_ais_vessel.h (3)
7-25: LGTM: layout, lengths, CRC match MAVLink 2 common.Struct field order, offsets, total payload (58), field count (17), and CRC (243) look correct and consistent across 24-bit and non-24-bit info macros.
Also applies to: 27-36, 38-85
23-24: Callers must zero-pad callsign/name and avoid NULL.Pack paths copy exactly 7/20 bytes; uninitialized tail bytes can leak garbage and alter CRC. Ensure inputs are at least those lengths and zero-padded.
Also applies to: 132-133, 399-401, 635-636, 643-646
152-153: Check helper availability consistency.This file mixes mav_array_assign_char and mav_array_memcpy. That’s fine if both helpers exist in our mavlink_helpers; please confirm our subtree has both.
Also applies to: 227-228
lib/main/MAVLink/common/mavlink_msg_can_filter_modify.h (2)
7-14: LGTM: struct, offsets, lengths, CRC look consistent.Payload (37), ids[16] at offset 0, scalar offsets 32–36, CRC 8 align with expectations.
Also applies to: 16-25, 26-51
67-78: Guard num_ids and ids buffer size at call sites.Packers always copy 16 uint16s; passing num_ids > 16 or a shorter ids buffer risks OOB read and garbage data transmission. Ensure num_ids ≤ 16 and the array is zero-padded to length 16.
Also applies to: 80-87, 109-120, 122-129, 154-166, 168-176
lib/main/MAVLink/common/mavlink_msg_canfd_frame.h (2)
7-14: LGTM: definition, offsets, lengths, CRC are coherent.72-byte payload, data[64] at offset 8, scalar offsets 0–7, CRC 4; both 24-bit and non-24-bit info look correct.
Also applies to: 16-25, 26-51
67-78: Validate len ∈ [0,64] and zero-pad data.Packers copy 64 bytes regardless of len; ensure callers bound len and provide a 64-byte, zero-padded buffer to avoid leaking stale memory and to keep CRC deterministic.
Also applies to: 80-87, 109-120, 122-129, 154-166, 168-176, 236-246, 248-256, 281-291, 293-301
lib/main/MAVLink/common/mavlink_msg_button_change.h (2)
55-76: LGTM: pack/encode/send/decode paths are consistent with offsets and lengths.Also applies to: 90-115, 128-150, 201-221, 236-263, 305-316
13-21: Please fetch and share the official MAVLink common.xml entry for BUTTON_CHANGE—including its message ID, field definitions, payload length, and CRC_EXTRA—to verify these values.lib/main/MAVLink/common/mavlink_msg_attitude_quaternion_cov.h (3)
16-23: CRC/length changed (LEN=72, CRC=167). Verify ecosystem upgrade.Make sure the entire MAVLink toolchain (generator, GCS, log parsers) is aligned to these values to avoid decode failures.
311-391: Decode path zero-initialization with len guard looks correct.
7-14: Legacy API removed; update downstream consumers
No references tomavlink_msg_attitude_quaternion_cov_get_time_boot_msor.time_boot_msremain in theattitude_quaternion_covmessage definition. Confirm all external tools (OSD overlays, GCS integrations, log-parsers) are updated to use the new 64-bittime_usecfield and regenerated CRC.lib/main/MAVLink/common/mavlink_msg_camera_settings.h (2)
6-14: Good: struct is packed; fields/offsets match LEN/MIN_LEN.
15-22: Extension fields present (MIN_LEN=5, LEN=14): verify older peers tolerate extensions.Older senders/receivers may only send/expect 5-byte payloads; your decode path handles this. Ensure any local encode usage is ok with peers that ignore extensions.
Also applies to: 50-88, 217-251, 266-296, 353-372
lib/main/MAVLink/common/mavlink_msg_actuator_output_status.h (2)
13-21: Header constants and field layout look consistent (ID=375, LEN=140, CRC=251).Offsets 0/8/12 match the struct packing; MIN_LEN == LEN for a non‑extension message. LGTM.
Also applies to: 23-42
1-3: Confirm these headers are generator outputs, not hand-edited.To keep the PR focused (per Betaflight maintainers’ preferences), please confirm the MAVLink generator commit/hash and common.xml revision used to produce this file and note it in the PR description.
lib/main/MAVLink/common/mavlink_msg_collision.h (2)
17-24: CRC/length/offsets check out (ID=247, LEN=19, CRC=81).Byte layout (id/float*3/src/action/threat) and offsets (0..18) are correct. LGTM.
Also applies to: 27-40
27-54: Field ordering in MESSAGE_INFO differs from wire order—verify any introspection users.The offsets are correct, but some tools may assume order parallels wire layout. If Betaflight/OSD tools rely on MESSAGE_INFO ordering, verify no regressions.
lib/main/MAVLink/common/mavlink_msg_attitude_quaternion.h (3)
19-23: Extension field addition looks correct.repr_offset_q[4] at offset 32 with LEN=48 and MIN_LEN=32 matches MAVLink 2 extensions. CRC unchanged (246). LGTM.
Also applies to: 27-60
108-110: MIN_LEN vs LEN finalization—double-check MAVLink v1 interop (if any).With MIN_LEN=32 and LEN=48, v1 links should see 32-byte payloads. If Betaflight is MAVLink 2-only, ignore; otherwise verify ground stations on v1 behave as expected.
79-81: All pack/pack_chan/send calls updated pack/pack_chan/send now include the new repr_offset_q; encode/encode_chan stay unchanged by design. No other instances found.lib/main/MAVLink/common/mavlink_msg_camera_thermal_range.h (1)
1-112: LGTM: message definition, sizes, offsets, and CRC look consistent with MAVLink 2.0 patterns.
Packing/finalization paths and getters/decoder are aligned with common generator output. No functional issues spotted.Also applies to: 171-222, 366-485
lib/main/MAVLink/common/mavlink_msg_can_frame.h (1)
1-52: Verify MAVLink CAN_FRAME ID/CRC
ID 386 and CRC 132 are correctly defined in mavlink_msg_can_frame.h; confirm these values match the MAVLink dialect commit used to generate all headers.lib/main/MAVLink/common/mavlink_msg_altitude.h (1)
1-100: LGTM: structure, sizes, offsets, and finalize paths match spec.
No functional concerns; decode path uses the standard memset+memcpy fallback correctly.Also applies to: 200-240, 332-429
lib/main/MAVLink/common/mavlink_msg_available_modes.h (2)
16-23: Header constants/lengths/CRC look correct.IDs, lengths (46), and CRC (134) align with the payload layout.
86-86: Reference correct MAVLink helper in protocol.h
mav_array_assign_char and mav_array_memcpy are both defined in lib/main/MAVLink/protocol.h – assign_char uses strncpy (with null-check) and memcpy handles NULL sources – their semantics match and no additional definitions are needed.lib/main/MAVLink/common/mavlink_msg_camera_tracking_geo_status.h (2)
24-31: Header constants/lengths/CRC look correct.LEN 50, MIN_LEN 49, CRC 18 match field layout and offsets.
401-401: No action required for payload size. DefaultMAVLINK_MAX_PAYLOAD_LENis defined as 255, which exceeds the ≥50-byte requirement forCAMERA_TRACKING_GEO_STATUS, so the send_buf path is supported.lib/main/MAVLink/common/mavlink_msg_camera_tracking_image_status.h (1)
21-28: Header constants/lengths/CRC look correct.LEN 32, MIN_LEN 31, CRC 126 align with field layout and offsets.
lib/main/MAVLink/common/mavlink_msg_camera_capture_status.h (1)
6-16: LGTM: struct layout, offsets, LEN/MIN_LEN, and getters are consistent.
- Field sizes sum to 23 bytes as declared; offsets in INFO macros and getters match packing.
- MIN_LEN=18 aligns with base fields before extensions.
Also applies to: 18-25, 28-42, 75-107, 266-295, 351-456
lib/main/MAVLink/common/mavlink_msg_autopilot_state_for_gimbal_device.h (1)
6-21: LGTM: packed struct and metadata look correct.
- Offsets in INFO macros match the packed layout; LEN=57/MIN_LEN=53 consistent with extensions starting at offset 53.
Also applies to: 23-31, 33-72
lib/main/MAVLink/common/mavlink_msg_camera_information.h (1)
333-394: Verify zero-padding in char-array helpers
Review mav_array_assign_char and mav_array_memcpy in protocol.h to confirm they zero-fill any remaining bytes in fixed-length fields (vendor_name, model_name, cam_definition_uri); update these helpers to pad with zeros if they don’t.
The Coderabbitay shows many errors in lib. The olde BF Mavlink lib files were not full. The some usefull functions were missed. |
I trust this doesn’t block? |
It doesn't block. |
|
@coderabbitai, thank you! |
|
Tip For best results, initiate chat on the files or code changes. @demvlad You're very welcome! I'm glad the suggestions were helpful. Take your time implementing the changes, and feel free to tag me again once you've pushed the updates. I'll be happy to review the revised code and help ensure everything looks good for the MAVLink telemetry flow control implementation. Looking forward to seeing the improvements! 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
lib/main/MAVLink/common/mavlink_msg_can_frame.h (1)
128-128: Standardize on mav_array_memcpy and use the field-length macro.Avoid
sizeof(uint8_t)*8andmav_array_assign_uint8_tto reduce build variance across vendor drops.- mav_array_memcpy(packet.data, data, sizeof(uint8_t)*8); + mav_array_memcpy(packet.data, data, MAVLINK_MSG_CAN_FRAME_FIELD_DATA_LEN);- mav_array_assign_uint8_t(packet->data, data, 8); + mav_array_memcpy(packet->data, data, MAVLINK_MSG_CAN_FRAME_FIELD_DATA_LEN);Also applies to: 299-299
lib/main/MAVLink/common/mavlink_msg_camera_information.h (1)
7-24: Resolved: struct is properly packed for on-wire memcpy.The wire struct now uses MAVPACKED, fixing the earlier risk of padding-induced corruption in the aligned/no-byteswap path. Looks good.
lib/main/MAVLink/common/mavlink_msg_attitude_target.h (1)
309-318: Fix build-breaker: replace mav_array_assign_float in send_buf()This is the last remaining assign helper; use the memcpy variant for portability (consistent with other changes).
- mav_array_assign_float(packet->q, q, 4); + mav_array_memcpy(packet->q, q, sizeof(packet->q));
🧹 Nitpick comments (10)
lib/main/MAVLink/common/mavlink_msg_can_frame.h (2)
77-77: Replace magic number 8 with MAVLINK_MSG_CAN_FRAME_FIELD_DATA_LEN for clarity and safety.Keeps array lengths self-documenting and prevents future drift.
- _mav_put_uint8_t_array(buf, 8, data, 8); + _mav_put_uint8_t_array(buf, 8, data, MAVLINK_MSG_CAN_FRAME_FIELD_DATA_LEN);- _mav_put_uint8_t_array(buf, 8, data, 8); + _mav_put_uint8_t_array(buf, 8, data, MAVLINK_MSG_CAN_FRAME_FIELD_DATA_LEN);- _mav_put_uint8_t_array(buf, 8, data, 8); + _mav_put_uint8_t_array(buf, 8, data, MAVLINK_MSG_CAN_FRAME_FIELD_DATA_LEN);- _mav_put_uint8_t_array(buf, 8, data, 8); + _mav_put_uint8_t_array(buf, 8, data, MAVLINK_MSG_CAN_FRAME_FIELD_DATA_LEN);- _mav_put_uint8_t_array(buf, 8, data, 8); + _mav_put_uint8_t_array(buf, 8, data, MAVLINK_MSG_CAN_FRAME_FIELD_DATA_LEN);- return _MAV_RETURN_uint8_t_array(msg, data, 8, 8); + return _MAV_RETURN_uint8_t_array(msg, data, MAVLINK_MSG_CAN_FRAME_FIELD_DATA_LEN, 8);Also applies to: 119-119, 165-165, 245-245, 290-290, 367-367
67-92: Optionally clamp the len field to 8 to avoid inconsistent frames.Prevents encoding frames with len > 8, which peers may treat as invalid even though only 8 bytes are copied.
static inline uint16_t mavlink_msg_can_frame_pack(uint8_t system_id, uint8_t component_id, mavlink_message_t* msg, uint8_t target_system, uint8_t target_component, uint8_t bus, uint8_t len, uint32_t id, const uint8_t *data) { + if (len > MAVLINK_MSG_CAN_FRAME_FIELD_DATA_LEN) { + len = MAVLINK_MSG_CAN_FRAME_FIELD_DATA_LEN; + } #if MAVLINK_NEED_BYTE_SWAP || !MAVLINK_ALIGNED_FIELDSRepeat the same 3-line clamp at the start of:
- mavlink_msg_can_frame_pack_status(...)
- mavlink_msg_can_frame_pack_chan(...)
- mavlink_msg_can_frame_send(...)
- mavlink_msg_can_frame_send_buf(...)
Also applies to: 109-138, 154-180, 236-257, 281-302
lib/main/MAVLink/common/mavlink_msg_actuator_control_target.h (2)
23-33: Nit: consider ordering MESSAGE_INFO fields by ascending offset.Purely for readability/introspection consistency (no functional change). If you prefer to keep generator order, ignore.
Also applies to: 34-42
10-10: Nit: fix minor grammar in docs.Use “differentiate between instances” instead of “difference between instances”.
- Actuator group. The "_mlx" indicates this is a multi-instance message and a MAVLink parser should use this field to difference between instances. + Actuator group. The "_mlx" indicates this is a multi-instance message and a MAVLink parser should use this field to differentiate between instances.Also applies to: 51-52, 120-121, 192-193, 272-273
lib/main/MAVLink/common/mavlink_msg_battery_status.h (2)
100-119: Optional: NULL-pointer safety for array inputs.If callers ever pass NULL for voltages/voltages_ext, _mav_put_uint16_t_array will read invalid memory. Guard or document non-NULL preconditions. Low risk if callers always provide arrays.
- _mav_put_uint16_t_array(buf, 10, voltages, 10); - _mav_put_uint16_t_array(buf, 41, voltages_ext, 4); + if (voltages) _mav_put_uint16_t_array(buf, 10, voltages, 10); + if (voltages_ext) _mav_put_uint16_t_array(buf, 41, voltages_ext, 4);Also applies to: 239-256, 343-360, 405-421
1-76: Scope note: keep generator deltas minimal.These edits look aligned with upstream MAVLink v2 patterns. Please avoid further manual reshuffles to ease future regen/upstream sync, per prior maintainer guidance.
lib/main/MAVLink/common/mavlink_msg_attitude_target.h (4)
92-92: *Prefer sizeof(packet.q) over sizeof(float)4Removes a magic number and ties size to the field.
- mav_array_memcpy(packet.q, q, sizeof(float) * 4); + mav_array_memcpy(packet.q, q, sizeof(packet.q));Also applies to: 137-137, 186-186, 269-269
82-82: Use MAVLINK_ATTITUDE_TARGET_FIELD_Q_LEN instead of literal 4Keeps array length consistent with field definition.
- _mav_put_float_array(buf, 4, q, 4); + _mav_put_float_array(buf, 4, q, MAVLINK_ATTITUDE_TARGET_FIELD_Q_LEN);- return _MAV_RETURN_float_array(msg, q, 4, 4); + return _MAV_RETURN_float_array(msg, q, MAVLINK_ATTITUDE_TARGET_FIELD_Q_LEN, 4);Also applies to: 127-127, 176-176, 259-259, 306-306, 354-354
275-278: Doxygen param name: use ‘attitude_target’ instead of ‘struct’Match the actual parameter name.
- * @param struct The MAVLink struct to serialize + * @param attitude_target The MAVLink struct to serialize
104-106: Doc param name mismatch (_status)Keep docs in sync with the function signature.
- * @param status MAVLink status structure + * @param _status MAVLink status structure
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/main/MAVLink/common/mavlink_msg_actuator_control_target.h(7 hunks)lib/main/MAVLink/common/mavlink_msg_attitude_target.h(7 hunks)lib/main/MAVLink/common/mavlink_msg_battery_status.h(6 hunks)lib/main/MAVLink/common/mavlink_msg_camera_information.h(1 hunks)lib/main/MAVLink/common/mavlink_msg_can_frame.h(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ledvinap
PR: betaflight/betaflight#14578
File: src/main/telemetry/jetiexbus.c:547-556
Timestamp: 2025-08-12T18:26:17.016Z
Learning: For Betaflight Jeti ExBus backport PR #14578: ledvinap prefers to keep the PR focused on backporting fixes from iNavFlight rather than expanding into broader architectural improvements. Additional improvements to TX/RX state machine should be handled in separate PRs.
📚 Learning: 2025-08-12T18:26:17.016Z
Learnt from: ledvinap
PR: betaflight/betaflight#14578
File: src/main/telemetry/jetiexbus.c:547-556
Timestamp: 2025-08-12T18:26:17.016Z
Learning: For Betaflight Jeti ExBus backport PR #14578: ledvinap prefers to keep the PR focused on backporting fixes from iNavFlight rather than expanding into broader architectural improvements. Additional improvements to TX/RX state machine should be handled in separate PRs.
Applied to files:
lib/main/MAVLink/common/mavlink_msg_battery_status.h
📚 Learning: 2025-07-08T07:52:07.174Z
Learnt from: jianpingwu1
PR: betaflight/betaflight#14520
File: src/platform/GD32/pwm_output_dshot.c:166-171
Timestamp: 2025-07-08T07:52:07.174Z
Learning: GD32F4 timer channel DMA hardware requires the second data element in DShot DMA buffers to appear twice. This necessitates a memmove operation in pwmCompleteDshotMotorUpdate() to shift buffer elements and duplicate the second data element at positions [1] and [2].
Applied to files:
lib/main/MAVLink/common/mavlink_msg_attitude_target.h
🔇 Additional comments (5)
lib/main/MAVLink/common/mavlink_msg_camera_information.h (2)
26-33: Message size/CRC constants look consistent with MAVLink COMMON.LEN/MIN_LEN (237/235) align with two extension bytes; CRC 92 matches known CAMERA_INFORMATION values. No action.
141-145: No fallback required — macros present in protocol.hmav_array_assign_uint8_t and mav_array_assign_char are defined in lib/main/MAVLink/protocol.h (static inline / #define around lines ~188–197), so the generated headers are fine; no fallback needed.
lib/main/MAVLink/common/mavlink_msg_actuator_control_target.h (1)
65-69: Good: unified to mav_array_memcpy for controls array.Consistent use of mav_array_memcpy improves portability across generated trees. No action needed here.
Also applies to: 98-103, 135-139, 206-211, 245-249
lib/main/MAVLink/common/mavlink_msg_battery_status.h (1)
542-585: Good: length-guarded getters for extension fields.The msg->len checks on time_remaining, charge_state, voltages_ext, mode, and fault_bitmask prevent OOB reads on short (legacy) frames.
lib/main/MAVLink/common/mavlink_msg_attitude_target.h (1)
27-54: LGTM: Message info table and offsetsField offsets/lengths/CRC align with the packed layout; gating under MAVLINK_COMMAND_24BIT looks correct.
|
Probably MAVLink library devellopers have not Coderabbitai! :) |
Co-authored-by: Mark Haslinghuis <mark@numloq.nl>
Co-authored-by: Petr Ledvina <ledvinap@gmail.com>
Co-authored-by: Petr Ledvina <ledvinap@gmail.com>
Co-authored-by: Petr Ledvina <ledvinap@gmail.com>
Co-authored-by: Mark Haslinghuis <mark@numloq.nl>
|
The interested question is how to test its.
.... will it work at one uart? Probably, will get one issue: the rssi and Link Quality data is in RADIO_STATUS mavlink packet now. Will we get these data in CRFS channel like before? (inav get rssi and LQ in mavlink module). |
blckmn
left a comment
blckmn
left a comment
There was a problem hiding this comment.
Is it worth making this a submodule if the source is actively maintained elsewhere?
|
Thank's all! |
Definely, my explains was wrong. I've doubted what AI has written about, because it is unpossible to read two protocol data from one uart. But probably, the CFRS protocol is much faster and reliable than mavlink. Will think about other solution too. Probably, using of mavlink for RC data in BF is not good though. Maybe try to use second uart to send mavlink telemetry into elrs rx. Can elrs transmitter send RC by CFRS and read mavlink telemetry to share telemetry data by wify? |
|
It seems, this flow control PR is lessfull, because:
|
|
Yes, There are too many problem by mavlink. Definelly, the using of MissionPlanner programm as ready solution is wrong way. Probably the next way will better for my task:
|
@demvlad would be nice to implement this. |
Ok. Will try to do it. |
6 participants
I need flight data telemetry to Mission Planner ground PC program.
ExpressLRS has full bi-directional MAVLink support.
But ExpressELRS manual says:
"Betaflight contains an incomplete implementation of the MAVLink protocol standard (lacking RADIO_STATUS flow control). This causes an Betaflight aircraft to saturate the bandwidth of a telemetry link using soft flow control, and renders it unusable, ergo breaking support with ExpressLRS MAVLink."
This PR does:
The flow control reads RADIO_STATUS frame and locks telemetry transmit while TX buffer is about to be filled out. The same method is used in INAV firmware.
The mavlink_min_txbuff cli parameter set minimum of free TX buffer space percent to enable MAVLink telemetry transmit.
This value can prevent TX buffer overflow.
The default mavlink_min_txbuff value is 0, the flow control is off.
Some DEBUG parameters will be added next time.
Testing is required.
Summary by CodeRabbit
New Features
Enhancements
Protocol Changes
Removals