Generate devid.json and devid.json5 from code, use in decode_devid.py - #34396
Conversation
|
Will stop ArduPilot/MissionPlanner#3772 and mavlink/qgroundcontrol#15047 from being required. Hopefully. |
Previous review (2026-09-14)Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Reviewed at head Verdict: REQUEST CHANGES. The generated tables are correct:
The problem is a downstream copy of the script. Bug
Notes
Checked:
CI was still running when checked. |
acb1b35 to
3ddfcff
Compare
|
@amilcarlucas - what are your thoughts on all of this?
|
| devtype | Category | Before | After |
|---|---|---|---|
| 0x04 | compass | AK8963 (trailing space) |
AK8963 |
| 0x05 | compass | BMM150 (trailing space) |
BMM150 |
| 0x11 | compass | RM3100_OLD |
RM3100 |
| 0x12 | compass | RM3100 |
RM3100_2 |
| 0x13 | compass | MMC5883 |
MMC5983 |
| 0x31 | imu | ADIS1647x |
ADIS1647X |
| 0x18 | imu | UNKNOWN |
ACC_LSM9DS1 |
| 0x41 | imu | UNKNOWN |
ZEROONE_FPGA_SCH16T |
| 0x44 | imu | UNKNOWN |
ICM56686 |
These names change in the tables but look the same after clean_devtype(): DEVTYPE_INS_SERIAL→DEVTYPE_SERIAL and DEVTYPE_INS_BMI270→DEVTYPE_BMI270.
AMC's current test (clean_devtype("DEVTYPE_INS_ICM42688")) is not affected.
5. Optional: show device-type warnings
Some device types have a note in the firmware header. For example, compass 0x12 (DEVTYPE_RM3100_2) is a mistaken ID that only master firmware used, between 2020-03-26 and 2020-05-23. The command-line tool prints these notes as warnings. Library calls never print anything; to show a note in AMC's UI, use the new function:
from ardupilot_methodic_configurator.decode_devid import get_device_type_description
note = get_device_type_description(devtype, "compass") # None for most device typesOther tools that decode device IDs
MAVProxy, WebTools and Mission Planner have their own decoders and are not affected. If you'd like to stop maintaining a separate table, you can use ArduPilot's generated files instead:
- Generate them yourself: run
Tools/scripts/decode_devid.py --dump-json devid.json(or--dump-json5 devid.json5) in an ArduPilot checkout. - Download them: the autotest server's documentation build publishes
devid.jsonanddevid.json5next to the per-vehicle LogMessages files. Once Generate devid.json and devid.json5 from code, use in decode_devid.py #34396 is deployed, they are expected athttps://autotest.ardupilot.org/LogMessages/devid.jsonand.../devid.json5.
The file layout:
{
format_version: 1, // file structure version; readers should reject unknown values
data_version: "sha256:…", // hash of the tables; changes whenever any entry changes
bus_types: [ {value: 0x01, name: "I2C"}, … ],
device_types: {
compass: [ {value: 0x12, name: "DEVTYPE_RM3100_2", description: "unused, past mistake; …"}, … ],
imu: [ … ],
baro: [ … ],
airspeed: [ … ],
mavlink: [ … ],
},
}A device ID packs its fields into bits:
- bits 0–2: bus type
- bits 3–7: bus number
- bits 8–15: address
- bits 16–23: devtype
For DroneCAN (bus type 3), the devtype byte holds the sensor ID plus one for compasses, and is 0 for other sensors.
Questions go on #34396.
Previous review (2026-09-14)Automated review note — AI-generated (Claude), independently cross-checked by a second model and re-verified against the live diff. Please sanity-check before acting. Reviewed again at head The generated tables are correct, but the script now fails when run through a symlink. Verdict: APPROVE → COMMENT. Everything from the earlier rounds is still resolved. Issue
Notes
Checked independently by both reviewers: all 111 enumerators match the headers; old and new decoding differ only in the renames you list, over 1662 CLI cases and 30,720 IDs; |
3ddfcff to
21030ab
Compare
Previous review (2026-09-15)Automated review note — AI-generated (Claude), independently cross-checked by a second model and re-verified against the live diff. Please sanity-check before acting. Re-reviewed at head This push is a rebase. The symlink issue from last round is still open. Verdict: COMMENT (unchanged). The only change to your patch is also dropping Still open
CI: 101 passing, 1 pending. |
Rather than maintaining copies of the bus and device type tables, decode_devid.py now parses the enums in AP_HAL/Device.h, AP_SerialManager.h and the compass, IMU, baro and airspeed backend headers using logger_metadata/enum_parse.py. A small rename map keeps established display names (DRONECAN, AK0991x), and the retired LIS2MDL ID, which is in no enum, is listed explicitly. Comments on enum entries are shown as warnings when decoding. --dump-json and --dump-json5 write the tables, with a format_version and a content-hash data_version, for tools without a source tree; --json reads such a file back. A copy of the script outside an ArduPilot tree (as synced by MethodicConfigurator) reads devid.json from its own directory. The files are published alongside LogMessages.* by build_log_message_documentation.sh, and CI dumps them in the logger_metadata step so that header changes which break parsing are caught. The tables had drifted from the headers: ACC_LSM9DS1, INS_ZEROONE_FPGA_SCH16T and INS_ICM56686 were missing, MMC5883 is now MMC5983, AK8963/BMM150 had trailing spaces, and several names now follow the headers.
21030ab to
a1f71a5
Compare
…matches The hex and "1U << n" patterns were not anchored, so "0x18 + 1" parsed as 0x18, "(1U<<4) | 8" as 16, and a second entry on the same line was lost; these now fail to match rather than giving wrong values. The forms used in the tree (parentheses, U/UL/ULL suffixes) are accepted. A comment containing "};" no longer ends an enumeration early, and comments after a closing parenthesis or without a trailing comma, previously dropped, are now captured; this adds descriptions to the LogMessages output but leaves enumeration names and values unchanged. enum_parse_unittests.py covers this and runs in the logger_metadata CI step. No header in the tree uses the forms which were parsed wrongly, but this parser is used to generate device ID metadata in ArduPilot#34396, where a header written in one of them would give wrong device names.
|
Automated review note — AI-generated (Claude), independently cross-checked by a second model and re-verified against the live diff. Please sanity-check before acting. Re-reviewed at head All four open items from the previous round are resolved. Verdict stays COMMENT on two cheap, non-blocking notes below. This push is not a rebase — merge-base with master is Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_16_1511/devcall_pr_reviews.html#pr34396 Resolved
Notes (neither blocks merge)
Smaller still, not worth acting on separately: no test asserts the generated output (the CI hook checks only the exit status), and the legacy tables are now rebuilt on each attribute access, so Verified cleanI established table correctness three independent ways rather than taking the generator's word for it: an CI at this head: 5 passing, 87 pending, 0 failing, 0 cancelled (snapshotted once, not waited on). Both an independent Codex pass and this review were run on this head and reconciled. The cold pass argued for REQUEST CHANGES on the parser limitation; I did not carry that, because the current tables are provably correct and you deferred the hardening deliberately. |
|
AMC 4.4.4 is already using this. |
Summary
Tools/scripts/decode_devid.pynow reads bus and device types from the C++ enums instead of keeping its own copies of the tables, which had drifted. It can also write the tables asdevid.json/devid.json5(with format and data versions). Those files are published with the LogMessages documentation, and CI checks that the headers still parse.Classification & Testing (check all that apply and add your own)
Testing:
decode_devid.py.-C/-I/-B/-A/-M, including invalid, negative, out-of-range and DroneCAN IDs). The only differences are the intended name fixes and newly decoded types listed below.--dump-json, read back with--json, is identical to live header parsing in all cases.devid.jsonis valid JSON.devid.json5parses with the Pythonjson5package to the same data asdevid.json.enum_parsecan't evaluate, which would otherwise make it silently drop the whole enum0x19compass IDload_devid_jsonrejects files with a wrongformat_version, a missingdata_version, a missing category or duplicate values.build_log_message_documentation.shin full withBUILDLOGSpointing to a scratch directory. It exited 0, and the publisheddevid.json/devid.json5match direct dumps.logger_metadata/parse.pystill runs with the extendedAP_Compass_Backend.hcomment.devid.jsonbeside it; without one, the import still succeeds and only the calls fail, with an error naming both places it looked. mypy with MethodicConfigurator's own settings at Python 3.10 reports no issues.--jsonfile (a top-level array, a missing file, truncated JSON) and an unwritable--dump-json/--dump-json5path each print an error and exit 1 rather than a traceback.Description
The device-type tables in
decode_devid.pywere hand-maintained copies of enums in the firmware, and they had drifted:DEVTYPE_ACC_LSM9DS1(0x18),DEVTYPE_INS_ZEROONE_FPGA_SCH16T(0x41) andDEVTYPE_INS_ICM56686(0x44) were missing.MMC5883, although the header was changed toMMC5983in 4c756fd.AK8963andBMM150had trailing spaces.Live parsing. The script now parses the enums with the existing
Tools/autotest/logger_metadata/enum_parse.py:AP_HAL::Device::BusTypeAP_Compass_Backend::DevTypesAP_InertialSensor_Backend::DevTypesAP_Baro_Backend::DevTypesAP_Airspeed_Backend::DevTypeAP_SerialManager::DeviceTypeBeyond the enums themselves, the script has:
BUS_TYPE_, airspeed names getDEVTYPE_AIRSPEED_.UAVCAN→DRONECAN,CANBUS→CAN, andDEVTYPE_AK09916→DEVTYPE_AK0991x(from ab3b68c). A rename that no longer matches an enum entry is an error.0x19. Colliding with an enum value is an error.Warnings from enum comments. A comment on an enum entry becomes that entry's
descriptionand is printed as a warning when the ID is decoded. TheDEVTYPE_RM3100_2comment now records that 0x12 was only used by master firmware from 2020-03-26 (dd4cf6c) to 2020-05-23 (a0cf4e1):JSON output for other tools. For tools without an ArduPilot source tree (GCSs, log analysers):
--dump-json FILEwritesdevid.json.--dump-json5 FILEwritesdevid.json5: the same data with0xNNvalues and unquoted keys.--json FILEmakes the script use such a file instead of the headers.devid.jsonfrom its own directory, which is how MethodicConfigurator's synced copy will work. Inside a tree the headers always win.Both files contain:
format_version(currently1), the version of the file structuredata_version, asha256:hash of the tables, which changes exactly when the data changesbus_typesanddevice_types.{compass,imu,baro,airspeed,mavlink}as lists of{value, name[, description]}Build server and CI.
build_log_message_documentation.shpublishesdevid.jsonanddevid.json5inLogMessages/, next to the per-vehicleLogMessages.*.logger_metadataCI step dumps both files to/dev/null, so a header change that breaks parsing fails CI.Compatibility. Code that imports the module still works:
BUSTYPES,COMPASS_TYPES,IMU_TYPES,BARO_TYPES,AIRSPEED_TYPESandMAVLINK_TYPESare still provided, now built when first used, and__all__keeps them available throughfrom decode_devid import *.__getattr__checks the attribute name before loading anything, so importing works outside an ArduPilot tree even with nodevid.jsonpresent; only the calls then fail.decode_device_id,get_device_type_nameandformat_device_infoare unchanged.Output changes from before (all now match the headers):
DEVTYPE_RM3100_OLD/DEVTYPE_RM3100→DEVTYPE_RM3100/DEVTYPE_RM3100_2DEVTYPE_MMC5883→DEVTYPE_MMC5983DEVTYPE_INS_SERIAL→DEVTYPE_SERIALDEVTYPE_INS_BMI270→DEVTYPE_BMI270DEVTYPE_INS_ADIS1647x→DEVTYPE_INS_ADIS1647XDEVTYPE_AK8963andDEVTYPE_BMM150Behaviour changes.
devid.jsonbeside it, or a file passed with--json.0x00now decodes asDEVTYPE_MAVLINK_UNKNOWNrather than the literalUNKNOWN.Follow-up (not needed here).
enum_parse.pyhas some long-standing quirks: unanchored patterns mean0x18 + 1parses as0x18, a comment containing};ends an enumeration early, and one/* */pattern mis-parses hexadecimal entries. No header in the tree uses those forms, and the tables generated here are identical either way, so hardening it (with unit tests, and converting the one header that uses/* */comments in enumerations) is a separate branch.claude helped!