feat(dronecan): GPS node-ID and battery-ID filter fields#2673
Draft
daijoubu wants to merge 48 commits into
Draft
feat(dronecan): GPS node-ID and battery-ID filter fields#2673daijoubu wants to merge 48 commits into
daijoubu wants to merge 48 commits into
Conversation
- Add MSP2_INAV_DRONECAN_NODES (0x2042) and MSP2_INAV_DRONECAN_NODE_INFO (0x2043) to MSPCodes.js - Parse both responses in MSPHelper.js; add FC.DRONECAN_NODES and FC.DRONECAN_NODE_INFO state slots in fc.js - Add tabs/dronecan.html and tabs/dronecan.js with node table, health badges, 2s auto-refresh, and per-node detail on row click - Register dronecan tab in gui.js allowedTabs and configurator_main.js - Add i18n strings for tab and column headers - Fix nav-toggle-all link being caught by tab-switch handler (showed spurious firmware upgrade warning) CSS health badge styling still pending.
- Fix MSPHelper.js: use 'data' not 'dataView' in DRONECAN_NODES/NODE_INFO parse handlers - Fix dronecan.js: set GUI.active_tab = this (not string) so cleanup() is callable on tab switch - Fix dronecan.js: interval.remove() not interval.kill() (correct API) - Fix dronecan.js: call GUI.content_ready(callback) to remove loading overlay; bare callback() left content permanently hidden - Add case 'dronecan' to tab switch in configurator_main.js - Add CSS health badge colour coding (src/css/tabs/dronecan.css) - Remove commented-out duplicate accordion block from configurator_main.js
Replace bespoke tab-pane/tab-header/tab-content markup with the standard tab-[name]/content_wrapper/tab_title/gui_box pattern used by all other tabs.
…ests Query MSP2_INAV_DRONECAN_NODE_INFO once per new nodeID and cache the name. Subsequent refreshes use the cache with no extra MSP requests.
… table - Show last_seen in node detail panel (now meaningful after firmware fix) - Remove uptime column from node table (not in NODES response; available in node detail via NODE_INFO)
…nvention Use green/amber/red scheme matching ArduPilot Mission Planner and QGroundControl: OK=green, WARNING=amber, ERROR=red, CRITICAL=red. Removes purple CRITICAL which had no precedent in DroneCAN tooling. Also removes duplicate accordion/disable_3d_acceleration handlers introduced by fdd4fcb that caused double-firing on nav group clicks.
Adds CAN baud rate and FC node ID controls to the DroneCAN tab using the generic settings API (MSPV2_SETTING / MSPV2_SET_SETTING). Save & Reboot button follows the standard content_toolbar pattern. Settings are loaded sequentially on tab init to avoid MSPV2_SETTING response race condition.
Three nodes on a bus line, grey/white variants for inactive/active states.
Remove stray NNER property and duplicate ANALOG/ARMING_CONFIG/FC_CONFIG/MISC declarations introduced by a bad hunk placement in the DroneCAN WIP commit.
… cleanup Add DRONECAN_NODES and DRONECAN_NODE_INFO to FC.resetState() so stale node data from a previous connection is cleared before reconnecting. Clear nameCache in dronecanTab.cleanup() so node names from a previous session are not shown when reconnecting to a different FC that reuses the same node IDs.
…NavFlight#7 iNavFlight#8 iNavFlight#10 - MSPHelper: add bounds check before getUint32 in DRONECAN_NODES parser to prevent RangeError on truncated packets (iNavFlight#3) - dronecan.js: replace parallel NODE_INFO requests with sequential fetch to avoid MSP dedup collapsing multiple node lookups to one (iNavFlight#4) - dronecan.js: guard NODE_INFO callback on info.nodeID === nodeId to prevent stale slot from a racing showDetail/render request (iNavFlight#8) - dronecan.js: use textContent for node name in both render and showDetail instead of unescaped innerHTML interpolation (iNavFlight#7) - dronecan.js: fill MODE_LABELS[4-6] with UNKNOWN_N and fall back to MODE_N for out-of-range values instead of silently showing OPERATIONAL (iNavFlight#10)
Fix getGpsProtocols() which was missing CRSF (firmware index 2) and had FAKE at the wrong index. Full list now matches gpsProvider_e enum: 0=UBLOX, 1=MSP, 2=CRSF, 3=FAKE, 4=DRONECAN. When DroneCAN is selected, hide the serial port and baud dropdowns (not applicable for CAN bus) and reset port to NONE so saving does not incorrectly assign a serial port to GPS. Show an info note directing the user to the DroneCAN tab for bus configuration.
Protocol now sits at the top of the GPS config section so port and baud collapsing when DroneCAN is selected doesn't shift it.
Blue background and border made the GPS enable toggle appear to turn blue when port/baud fields collapsed. Match preset_info box style instead.
Applying background to the outer div coloured sibling elements that fell within its visual bounds. Target the span so only the text gets the grey background.
Background box styling visually enclosed adjacent elements. Plain italic text at 0.9em conveys the note without affecting surrounding layout.
Note now sits within the protocol row above its divider, visually grouped with the dropdown that triggered it rather than floating between protocol and SBAS as a separate element.
Documents the Phase 5 wire format and verifies the parser correctly handles 46-byte (base) and 71-byte (with version data) payloads. Currently fails on version fields — passes after Phase 6 extends the MSPHelper.js decoder to read bytes 46-70.
…tail panel MSPHelper.js: extend MSP2_INAV_DRONECAN_NODE_INFO decoder to parse bytes 46-70 (sw_major, sw_minor, sw_optional_field_flags, sw_vcs_commit, hw_major, hw_minor, hw_unique_id) when payload is >= 71 bytes. dronecan.js: add SW Version, HW Version, and Unique ID rows to showDetail() panel, conditionally rendered when version data is present.
Replace synchronous NODE_INFO fetch with dronecanAsyncPoll() helper that works with the new firmware ASYNC_REQUEST/ASYNC_RESULT MSP commands. Seq generation counter guards against stale results across MSP ports. Changes: - MSPCodes: replace NODE_INFO (0x2043) with ASYNC_REQUEST/ASYNC_RESULT - MSPHelper: add ASYNC_REQUEST and ASYNC_RESULT decoders; ASYNC_RESULT handles variable-length GetNodeInfo and GetSet responses including BigInt for int64 values outside Number.MAX_SAFE_INTEGER - fc.js: replace DRONECAN_NODE_INFO with DRONECAN_ASYNC_REQUEST/RESULT - dronecan.js: add dronecanAsyncPoll() helper; update node name fetch and detail panel to use async pattern; add parameter list/edit UI - main.css: add .tab-dronecan to positioned tab list (fixes save button) - dronecan.css: style params table, inputs, and write buttons - Replace defunct NODE_INFO decoder test with ASYNC_RESULT decoder test covering all value types, BigInt edge case, and seq preservation
…e node handling - Add Save to EEPROM button (ExecuteOpcode service 10) and Restart Node button (RestartNode service 5) to the parameter panel - Decode ExecuteOpcode and RestartNode responses in MSPHelper (simple ok-only result, no name prefix) - Wire both services through dronecanAsyncPoll with correct payloads - Live-update Health, Mode, Last Seen, Uptime, Vendor Status in the detail panel on every 2s refresh without re-fetching GetNodeInfo - Close detail panel automatically when the displayed node disappears from the node list (e.g. after a node ID change and restart)
- Add stale callback guard in showDetail (discards response if node changed) - Add cancellation guard in fetchParam chain (same mechanism) - Scope .param-actions selector to #dronecan-params to avoid leakage - Fix font-size 12px → 1em on param buttons (scales with display density) - Fix service_id comment order in MSPHelper (5=RESTART_NODE, 10=EXECUTE_OPCODE) - Add poll window comment (34 × 75 ms ≈ 2.5 s)
…nt, health colour - Add esc() helper; escape all wire-sourced strings (node name, param name, param value) before innerHTML interpolation - Add comment in MSPHelper noting offset is not advanced past value in the PARAM_GETSET switch — harmless today but must be revisited if firmware extends the response with min/max fields - Distinguish health-critical from health-error with dark red (#7b0000) - Add comment on nameCache clear explaining why (stale names across reconnects)
…tants - Add try-catch around MSPHelper READY decoder to handle truncated responses - Copy hw_unique_id with .slice() to avoid holding a live view into MSP buffer - Clamp health index in showDetail initial render (matches render() path) - Add service_id + node_id to poll staleness check (sequence alone can collide) - Abort fetchParam chain before issuing request when node changes (not just in callback) - Extract POLL_INTERVAL_MS / POLL_MAX_ATTEMPTS named constants - const writeValue (was let, never reassigned) - CSS: fix .health-badge 6-space indent; 40px bottom padding → 2.5em
- Extract getModeLabel() helper; replace 3 identical inline expressions - esc() applied to all firmware-sourced innerHTML fields (nodeID, vendor status, sw/hw version numbers) for consistency with XSS defence - fetchParam: abort at index > 254 (DroneCAN param index upper bound) - parseInt radix: add base 10 to saveConfig node ID parse - MSPHelper: delete forward-reference comment about future min/max fields
Decode min/max from async result MSP payload, show Range column in the params table, and block out-of-range writes with a red input outline and inline error on the Write button. Save to EEPROM button gets min-width to prevent layout shift on text change.
Add dronecan* locale keys for all param tab UI strings and wire them up in dronecan.js. Reuses the existing 'OK' key for write success.
…d, BigInt comparison - MSPHelper.js: define PARAM_TYPE_* constants; guard decodeNumeric() calls to INT/FLOAT params only (BOOL/STRING have no numeric range); replace magic numbers inside decodeNumeric with named constants - dronecan.js: use Number(param.min/max) in range comparison to keep validation in Number domain consistently with payload.value; fix TYPE_LABELS unknown-type fallback to use esc(String(...))
…s, BigInt comparison - dronecan.js: define PARAM_TYPE_INT/FLOAT/BOOL/STRING constants; replace all magic 1/2/3/4 literals throughout; fix BigInt range comparison using typeof guard so out-of-safe-integer bounds are compared in BigInt domain; i18n param table column headers; remove esc() from bundled i18n strings - locale/en/messages.json: add dronecanParamCol* keys adjacent to existing dronecan block
Non-numeric text in an INT/FLOAT param field produces NaN, which passes the range check silently then throws inside BigInt() in dronecanAsyncPoll, leaving the button permanently frozen. Add an isNaN guard that shows the existing out-of-range feedback instead.
…ope, NaN tooltip, CSS cleanup - dronecan.js: add DRONECAN_SERVICE_* constants; replace all bare service ID literals at call sites and in dronecanAsyncPoll guards; fix NaN tooltip to show dronecanParamOutOfRange instead of empty string - MSPHelper.js: hoist PARAM_TYPE_* constants to IIFE top scope; remove duplicate nested declarations from service_id===11 block - dronecan.css: scope health badge classes under #dronecan-node-table to avoid global name collisions; remove 2-space outer indentation
Mirror the service ID constants already defined in dronecan.js into MSPHelper's IIFE so the decode switch uses named constants throughout rather than bare literals with inline comments.
- Add DRONECAN_ASYNC_STATE_READY/ERROR and DRONECAN_ASYNC_REQUEST_STATUS_OK/BUSY named constants; replace bare 0/1/2/3 state literals in dronecanAsyncPoll - Add DRONECAN_EXECUTE_OPCODE_SAVE constant; replace bare 0 in Save to EEPROM handler - Fix param index safety cap: 254→8191 to match UAVCAN v0 uint13 spec; correct comment - Fix INT write precision: use BigInt(writeValue) directly instead of Number() to avoid silent loss of precision for values outside Number.MAX_SAFE_INTEGER - Update INT range validation to compare in BigInt domain throughout - Update BigInt encode to handle pre-BigInt payload.value without Math.trunc() - i18n: replace 's ago' string literals with dronecanSecondsAgo message key - i18n: replace hardcoded 'Error' in Name cell with dronecanNameError message key - Add dronecanSecondsAgo and dronecanNameError keys to locale/en/messages.json
…ht#2671 Fix 3 BUG-severity accessibility issues (missing <label> associations, missing table header), refactor two CRITICAL-complexity functions (cognitive complexity 23 and 32, both over the 15 threshold) by extracting helper functions, flatten function nesting that exceeded 4 levels, remove nested ternaries and a btn-aliasing pattern, and apply optional chaining in place of manual null checks. Also fix WCAG contrast failures on two button classes (~2.7:1 -> ~6:1) and address MINOR style findings (Number.parseInt/parseFloat/isNaN, String#replaceAll, String.fromCodePoint, redundant block, trailing .0 literals).
…ue helpers paramValueToString deduplicated a repeated pattern but kept the nested ternary it was meant to eliminate; convertParamValue's catch still returned the bare global NaN instead of Number.NaN. Both were newly flagged by SonarCloud after the previous commit. Verified with eslint-plugin-sonarjs (cognitive-complexity) locally: no violations.
Reject NaN, 0, and values above 127 silently rather than passing an invalid value to setSetting (node ID 0 is anonymous/broadcast and disables CAN unicast on the FC).
…diately The FC writes the async result slot after receiving the CAN response, so the first 75 ms poll can read a stale result from the previous request. Retry up to POLL_MAX_ATTEMPTS before giving up, matching the same timeout window used for in-progress states.
#dronecan-detail-table th matched nothing; HTML uses .dronecan-detail. dronecanColUptime had no matching data-i18n attribute in the template.
…ected Adds a dronecan_gps_node_id number input (0–125) to the GPS tab that appears when DroneCAN is selected as the GPS provider and is hidden for all serial protocols. 0 means accept any node; set non-zero to lock to a specific node ID. Pairs with firmware fix/dronecan-gps-health-guard which adds the dronecan_gps_node_id setting.
SBAS mode, optimization preset, update rate, constellation toggles (Galileo/BeiDou/GLONASS), and Assistnow buttons are all serial u-blox specific — INAV cannot configure a DroneCAN GPS node remotely so these controls have no effect and are now hidden when DroneCAN is selected. Timezone and DST settings remain visible as they are INAV OSD settings independent of GPS protocol.
… tab - Show dronecan_battery_id field in Battery Settings section when either voltage or current source is set to CAN; hide otherwise - Hide ADC-specific voltagescale, currentscale, and currentoffset fields when CAN is selected for the respective source - Skip saving voltagescale when vbat_meter_type is CAN; skip currentscale and currentoffset when current_meter_type is CAN - Add i18n keys for field label and help tooltip
Firmware's setting.yaml defines dronecan_gps_node_id as min:0 max:127, but the HTML input and tooltip both said 125 - mixed up with DRONECAN_DNA_MAX_NODE_ID (the unrelated DNA auto-allocation ceiling). This is a static filter setting, not DNA allocation, so a node statically configured to 126/127 should still be selectable. The runtime max was already self-correcting via Settings.configureInputs() reading the firmware-reported setting metadata, but the static HTML value and tooltip text were wrong regardless and would permanently mislead users about the valid range.
…o-show
Settings.configureInputs() (js/settings.js) calls parent.show() on a
[data-setting] input's parent as soon as that individual field's own
MSP fetch resolves - which happens per-field, asynchronously, well
before the tab's full settingsPromise chain completes. Both the
battery-ID row and the GPS node-ID row rely on their own tab-specific
toggle logic (updateBatterySourceFields() / GPS provider handling)
that only runs after that full chain resolves, so there was a window
(confirmed ~0.6-1.3s via timing instrumentation) where the row was
visible even in its supposed-to-be-hidden default state.
Wrapped both rows in an additional <div class="setting-container">
- a mechanism configureInputs() already checks for via
input.parents('.setting-container:first') before falling back to
input.parent(), with zero CSS rules defined anywhere so it's visually
inert. This redirects the generic auto-show to the harmless outer
wrapper, leaving each row's own inline display:none default untouched
until its tab's specific conditional logic decides otherwise.
Verified via fine-grained visibility sampling (8 timestamps from t=0
to t=2s) that both rows now stay hidden throughout in their default
state, with no regression in the shown-when-relevant case. The GPS
row turned out to have the identical latent race, not caught in
earlier coarser-grained testing - now covered by the same fix.
- configuration.css: the .setting-container wrapper (added to fix the visibility race) incidentally made #dronecan-battery-id-row match a :last-child selector, dropping its bottom border/spacing when shown even though it isn't actually the last field in the section. Added a targeted override to restore the normal separator. - messages.json: label simplified from "DroneCAN Battery ID (0 = any)" to "DroneCAN Battery ID", matching the GPS node-ID field's pattern of a plain label with details in the tooltip - the existing help text already explains the 0=any behavior. - configuration.js: extracted the 'CAN' string literal (used at 4 call sites for CAN-source detection) into a named CAN_SOURCE_LABEL constant with a comment explaining why text comparison is used instead of a hardcoded ordinal - vbat_meter_type and current_meter_type use different firmware tables (voltage_sensor / current_sensor) with different CAN indices, so a single hardcoded number couldn't work for both anyway, and text comparison is robust to firmware table reordering. Verified live via SITL: CSS spacing now matches other rows when the battery-ID row is shown, label/tooltip read correctly, and the full ADC/CAN toggle matrix (7 cycles) still shows/hides all four rows correctly with the refactored constant - no regression, zero console errors.
Add missing border/spacing override for #gps_dronecan_node_id_row (same :last-child regression already fixed for the battery-ID row), and document the battery ID's valid range in its tooltip for consistency with the GPS node ID tooltip.
4 tasks
SonarCloud (Web:S6853) flagged both new labels as lacking accessible text since the <span data-i18n> elements were empty until i18next replaces their content at runtime. Match the existing convention used elsewhere in these files (e.g. gpsPresetMode, gpsUpdateRate) by giving each span the same English text already defined for its key in locale/en/messages.json.
|
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
dronecan.jsto retry on stale results instead of failing immediatelyStacked on
feature/dronecan-configurator-tab(UI base for GetNodeInfo/param GetSet) — PR to follow.Firmware branch:
fix/dronecan-gps-health-guard(iNavFlight/inav#11698) adds the correspondingdronecan_gps_node_id/dronecan_battery_idsettings this UI reads and writes.Test plan
:last-childCSS regressions on conditionally-shown rows, verified empirically against the real cascade in headless Chromium)