Add calculated depth variable - #2738
Conversation
ES-Alexander
left a comment
ES-Alexander
left a comment
There was a problem hiding this comment.
Approving because code generally looks ok, and the feature does seem to work.
Some comments/suggestions/questions :-)
| // Substitutes `NaN` for any data-lake variable that has not been populated yet, | ||
| // so expressions referencing missing dependencies evaluate to `NaN` instead of | ||
| // throwing a SyntaxError. Once the missing variable's data arrives, the listener | ||
| // fires and the expression is re-evaluated with the real value. | ||
| const expressionReplaceFunction = (match: string): string => { | ||
| const variableId = match.replace('{{', '').replace('}}', '').trim() | ||
| if (!variableId) return 'NaN' | ||
| const variableData = getDataLakeVariableData(variableId) | ||
| if (variableData === undefined) return 'NaN' | ||
| return variableData.toString() | ||
| } |
There was a problem hiding this comment.
Will this cause problems for variables that were expected to be strings? NaN makes sense for numerical data, but maybe undefined is more general?
If necessary we could have a very rough proxy where NaN is used if the computed output is expected to be numerical, but that assumes no numerical output is determined using string inputs (which would happen if you were mapping a set of string options to some meaningful numbers, for example).
Alternatively we could extend the specifier syntax to include an optional "value if undefined", but that adds complexity that ideally we would be able to avoid users needing to deal with 🤷♂️
| const currentDelay = nextDelayToEvaluateFaillingTransformingFunction[func.id] || 10 | ||
| const lastTimeTried = lastTimeTriedToEvaluateFaillingTransformingFunction[func.id] || 0 |
There was a problem hiding this comment.
| const currentDelay = nextDelayToEvaluateFaillingTransformingFunction[func.id] || 10 | |
| const lastTimeTried = lastTimeTriedToEvaluateFaillingTransformingFunction[func.id] || 0 | |
| const currentDelay = nextDelayToEvaluateFailingTransformingFunction[func.id] || 10 | |
| const lastTimeTried = lastTimeTriedToEvaluateFailingTransformingFunction[func.id] || 0 |
There's an extra l in "Failing".
This is continued usage of an existing variable, so not critical to fix in this PR, though it does add more instances that need fixing if it's just used as-is.
There was a problem hiding this comment.
Fixed — renamed to Failing on the backoff maps.
| /** | ||
| * Optional MAVLink path suffixes (e.g. `SCALED_PRESSURE2/press_abs`) that | ||
| * should be migrated to this option when found in persisted widget settings. | ||
| * Used to retire legacy paths in favor of the compound variables defined in | ||
| * `predefined-resources.ts`. | ||
| */ |
There was a problem hiding this comment.
This is likely excessive commentary for a migration helper. Also if we merge this PR without merging #2725 we can skip the migration entirely.
There was a problem hiding this comment.
Shortened the JSDoc. Kept the field itself: #2725 already landed without shipping SCALED_PRESSURE* as a preset on master, so this only helps anyone who persisted settings from an earlier draft of this work. Cheap enough to keep.
| * @param {number} paramValue - Parameter value | ||
| */ | ||
| private setParameterInDataLake(paramName: string, paramValue: number): void { | ||
| const dataLakeId = `/vehicle/${this.currentSystemId}/parameters/${paramName}` |
There was a problem hiding this comment.
| const dataLakeId = `/vehicle/${this.currentSystemId}/parameters/${paramName}` | |
| const dataLakeId = `vehicle/${this.currentSystemId}/parameters/${paramName}` |
I believe we're generally avoiding root slashes (with MAVLink kept as a legacy exception), because they don't really contribute anything.
More generally, it may be better to expose params through the existing MAVLink path (with param_id as the instancing field), and maybe just make the name nice? Technically the parameter protocol is not limited to vehicles, and can be from any MAVLink component (e.g. a robot arm and autopilot could be independent components in the same system that each have their own parameters).
That being said, I believe Cockpit only requests parameters from the autopilot at this stage, so if we're assuming that won't change then the current approach may be sufficient.
There was a problem hiding this comment.
Dropped the leading slash — path is now vehicle/{systemId}/parameters/{paramName}, and the compound-variable expressions match. Keeping the dedicated vehicle-parameter namespace for now since Cockpit only requests params from the autopilot.
| createDataLakeVariable({ | ||
| id: dataLakeId, | ||
| name: `Parameter ${paramName} (MAVLink / System: ${this.currentSystemId})`, | ||
| type: 'number', |
There was a problem hiding this comment.
It's likely worth explicitly including allowUserToChangeValue: false,, possibly with a TODO comment about eventually supporting parameter setting (for relevant params) as well.
There was a problem hiding this comment.
Done — set to false, with a TODO for eventually writing selected parameters back.
| id: dataLakeId, | ||
| name: `Parameter ${paramName} (MAVLink / System: ${this.currentSystemId})`, | ||
| type: 'number', | ||
| }) |
There was a problem hiding this comment.
Ideally we would include metadata like the per-firmware parameter descriptions, a breakdown of bitmasks and booleans vs regular numbers, and longer-term the units too. None are required for this PR, since it's already a net improvement as-is.
There was a problem hiding this comment.
Agreed. I believe a more broadly work on that front is the needed solution.
|
|
||
| return input.toString().replace(dataLakeInputRegex, (match) => replaceFunctionToUse(match)) | ||
| let current = input.toString() | ||
| for (let pass = 0; pass < MAX_DATA_LAKE_RESOLUTION_PASSES; pass++) { |
There was a problem hiding this comment.
Does this happen every time a compound or subtitle string variable gets evaluated?
From a performance standpoint it seems valuable to cache the outermost IDs, and set up a second set of listeners for the nested ones so re-resolving only happens when one of the dependencies has actually changed.
There was a problem hiding this comment.
Multi-pass only runs when a listened dependency changes (or when discovering IDs for subscription), capped at 10 and typically 1–2 passes on these short expressions. syncDependencies already listens to both the inner IDs (autopilotSystemId) and the outer resolved paths.
But let me think about this one performance-wise.
a81f379 to
1e9fc44
Compare
Each `PARAM_VALUE` received from the autopilot is now stored under
`/vehicle/{systemId}/parameters/{paramName}`, exposing parameters to
widgets and transforming functions reactively (the readable aspect of
bluerobotics#1848).
Registers `baro2.pressure_alt` and `baro3.pressure_alt` as transforming
functions in the predefined data-lake resources, alongside the camera
zoom/focus ones. Each computes altitude in meters from
`SCALED_PRESSURE*.press_abs` and the autopilot
`BARO*/BARO_SPEC_GRAV/BARO_ALT_OFFSET` parameters using ArduSub's
underwater depth formula, with the active autopilot resolved through
the nested `{{autopilotSystemId}}` template.
Two related changes to the transforming-function evaluator so the
predefined compound variables behave well across a connect:
- Expression evaluation now substitutes `NaN` for any missing
dependency instead of leaving the `{{...}}` placeholder literal. The
expression evaluates to `NaN` (which is what consumers expect for a
variable that "isn't ready yet") instead of throwing a SyntaxError
that burns through the exponential backoff retry budget.
- Listener setup re-discovers dependencies after every evaluation so
the resolved variable set follows changes to inner-template values.
When `autopilotSystemId` flips (a vehicle connects, or its system ID
is changed), expressions that template on it now seamlessly switch
to listening on the newly-resolved `/vehicle/{N}/...` paths.
The data-lake substitution engine was single-pass, so a placeholder
whose value itself contained `{{...}}` was left half-resolved. That
blocks the `baro2.pressure_alt` / `baro3.pressure_alt` compound
variables (and any future one) from referencing per-vehicle paths via
nested `{{autopilotSystemId}}` substitution, e.g.
`{{/vehicle/{{autopilotSystemId}}/parameters/BARO_SPEC_GRAV}}`.
`replaceDataLakeInputsInString` and `findDataLakeVariablesIdsInString`
now loop the substitution / discovery until the result is stable
(capped at 10 passes). Inputs that don't nest behave exactly as before
since the first pass already produces the final result; nested inputs
fully resolve in one call and their inner-resolved variable IDs are
included in the discovery output.
… variables Adds `baro2.pressure_alt` and `baro3.pressure_alt` to the altitude source options that depth/altitude widgets expose in their config menus. Their `value` points at the predefined compound variable IDs (so the widget subscribes directly to the resolved altitude in meters and `toMeters` is the identity), and the new `legacyMatchSuffixes` field migrates persisted widget settings that still reference the raw `SCALED_PRESSURE*/press_abs` paths.
1e9fc44 to
3e26fba
Compare
Compound-variable altitude sources (e.g. `baro2.pressure_alt`) return `NaN` until every dependency has been received. Without this guard the widgets display "NaN m" and DepthHUD's history buffer gets poisoned. Treat non-finite values as missing so consumers keep their existing "--" / blank fallbacks.
3e26fba to
799a015
Compare
There was a problem hiding this comment.
- 🛑 values don't update
- I suspect the templating isn't properly accounting for internal variables when determining which variables should trigger updates of a compound variable, so because the system ID doesn't tend to update the value only gets evaluated once and then stays constant
- If I edit the
baro2.pressure_altdefinition to use hardcoded 1s for the system ID then it updates fine- The preferred solution here is obviously to fix the triggering issue, but if desired this PR could change to hardcoded system IDs and the templating stuff can get fixed in a later PR
Side notes:
- Searching the data-lake seems to cause low-performance spikes, though this apparently also happens in master
- I don't remember how data-lake search indexing works, but nothing I search seems to help me find the pressure_alt variables - typing "baro2" or "pressure_alt" results in no filtering
- Possibly we need to be splitting IDs by slashes and dots as part of the searching process?
- In the compound variable definition, only the internal variables get the hover treatment
- The outer variable (using the inner one as part of its ID) has no hover helper to show the name and value
- Various incoming data lake variables show up (in the table) as "User defined", including CPU temperature, as well as apparently MAVLink Camera Tilt and Network Latency
|
@ES-Alexander my bad I'm still updating it. Will ping when done. |
I was hopping #2872 made it unnecessary. What do you guys think? Pinging @Williangalvani as well on the matter. |
|
I do think it is unnecessary, I'm also adding warnings on Sub when home gets set in not-great frames |
|
I'd want confirmation from the people running into the issue that it's actually resolved before we close down this approach. I suspect #2873 and/or #2874 is needed, to avoid the issue triggering during mission planning, with the latter also preventing users from accidentally triggering it by manually setting the home position from the map. Independently, commits 1, 3, 4, and 6 are for incidental fixes and improvements that are useful in general, regardless of whether we want the dedicated depth variable. 1 in particular would be nice to get merged somewhere, since it's useful for a lot of other applications. 4 would allow us to use dynamic MAVLink System IDs, but IIRC that functionality wasn't working at last review. As a side note, 1 has an issue linked in the commit message, which will cause notification spam if merged. |
I do plan to bring #2873 back. Was rebasing it last Friday. Will finish today and put again for review.
Totally agree. Merging this PR or not my plan is to move those dedicated commits to other PRs for review.
Good catch. Will remove it. |
Any news on this one? |
|
As I understand it we know that planning a mission still triggers it - I don't think it's worth asking for confirmation that it doesn't occur anymore until one of those fix PRs has been merged, right? It seems #2873 has already been approved - is there something holding it up? |
Just time to properly rebase it 😅 As I understand with #2873 merged you plan to ask people to test if they still see the bug. That's it? |
3 participants
Summary
Adds depth/altitude readings derived from
SCALED_PRESSURE2/3as predefined data-lake variables, with the underlying machinery needed to make them work cleanly across vehicles. Widgets that currently read raw pressure (DepthHUD,DepthIndicator,RelativeAltitudeIndicator) can now subscribe directly to the resolved altitude in meters.What changed
PARAM_VALUEreceived from the autopilot is mirrored at/vehicle/{systemId}/parameters/{paramName}, making parameters reactive to widgets and transforming functions (addresses the readable half of data-lake: provide access to autopilot parameters #1848).baro2.pressure_alt/baro3.pressure_alt. Registered as transforming functions that compute altitude in meters fromSCALED_PRESSURE*.press_absand the autopilot'sBARO*/BARO_SPEC_GRAV/BARO_ALT_OFFSETparameters using ArduSub's underwater depth formula, with the active vehicle resolved through the nested{{autopilotSystemId}}template.replaceDataLakeInputsInString/findDataLakeVariablesIdsInStringnow loop until stable (cap 10 passes), so a placeholder whose value itself contains{{...}}(e.g.{{/vehicle/{{autopilotSystemId}}/parameters/BARO_SPEC_GRAV}}) fully resolves. Non-nested inputs behave exactly as before.NaNinto the expression instead of leaving the literal{{...}}placeholder (which threwSyntaxErrorand burned the exponential-backoff budget). Listeners are re-discovered after every evaluation so expressions follow inner-template changes — e.g. whenautopilotSystemIdflips on connect, dependencies switch to the new/vehicle/{N}/...paths automatically.baro2.pressure_altandbaro3.pressure_altas altitude sources whosevaluepoints directly at the compound-variable IDs (toMetersbecomes the identity). A newlegacyMatchSuffixesfield migrates persisted settings that still reference the rawSCALED_PRESSURE*/press_abspaths.NaNguards on altitude consumers.DepthHUD,DepthIndicator, andRelativeAltitudeIndicatornow treat non-finite altitude as missing, so the brief window where a compound variable is still resolving doesn't show "NaN m" or poison the depth-history canvas.Test plan
baro2.pressure_alt/baro3.pressure_altappear in the data lake and resolve to sensible values once parameters arrive.DepthHUD,DepthIndicator, andRelativeAltitudeIndicatoroption menus, select the new altitude sources and confirm readings match the previous raw-pressure path.SCALED_PRESSURE*/press_abssource still load and switch to the new compound variable vialegacyMatchSuffixes.--/ blank instead ofNaN m./vehicle/{N}/...paths without a refresh.To be merged after #2725.
Closes #2736.