Repository navigation
perf(smartfuel): keep the configuration and compare the restart values instead of rebuilding both every wake - #789
Conversation
…re by value Smart.wakeup computes once a second, and every such wake built a new configuration table through three fresh closures and a parsed API version, then joined seven tostring results into a string to decide whether to restart the estimate. Both only change when a setting or the battery configuration does. The three helpers are file-level functions now. The configuration is kept and rebuilt only when one of its input values differs from the last wake; the inputs are compared by value, not by table identity, because the SmartFuel and battery pages write into the session's tables in place. The restart test compares its seven values field by field. A computing wake with unchanged inputs allocates nothing instead of 945 bytes (desktop Lua 5.3), and runs fewer instructions.
The first version compared the inputs through one helper call each and picked every value a second time when it rebuilt, so a wake that had to rebuild cost about 200 instructions more than before. The thirteen inputs are now read into locals once, compared in one test, and the table is built from those locals; the store of the last values is sized when the module loads.
…rison changes, and what moves the restart values The restart values move with the active battery profile and the pack's cell count as well as with the settings, and the one difference against the string comparison covers any value that changes only between an integer and a float of the same size, not only the cell count.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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 |
heliflieger
left a comment
heliflieger
left a comment
There was a problem hiding this comment.
The cache is keyed on the thirteen raw inputs, compared by value, and Smart.reset() drops it -- so an in-place write from the SmartFuel or Battery page is still seen, which the identity-keyed copy that fails to restart is exactly the right control for. The single deliberate difference (an int and a float of the same size no longer restarting the estimate) is called out in the body and in the release note, and it is an improvement, since the string comparison restarted the estimate for a value that had not moved. I verified that getSmartConfig has exactly one caller (:620) and that the returned table is only read there, so handing out one instance cannot leak a mutation into the next wake. Approving.
| and last.sagGain == sagGain and last.voltageFallLimit == voltageFallLimit | ||
| and last.fuelDropRate == fuelDropRate and last.stabilizeDelay == stabilizeDelay | ||
| and last.stableWindow == stableWindow then | ||
| return smartConfig |
There was a problem hiding this comment.
The cached table is now returned by reference. Its only caller is Smart.wakeup (:620), which reads source, firmwareSource, stabilizeDelaySeconds and stableWindowVolts and writes none of them, so the cache cannot be polluted today. If a later caller ever writes into cfg, this is the line where that becomes a stale-configuration bug.
There was a problem hiding this comment.
Agreed. 35f641b puts that rule into the comment above the cache: the kept table is handed out itself, not a copy, so a caller must not write into it.
Its only caller reads it today; the comment names the rule a later caller has to keep.
heliflieger
left a comment
heliflieger
left a comment
There was a problem hiding this comment.
Re-reviewed on the new head 35f641bd. The added commit is the documentation half of the point I raised on the previous head -- that the cached table is handed out by reference, so a caller must not write into it -- and it says exactly that in the comment above configInputs. No code changed, so the earlier review of the behaviour still stands on this head: the cache is keyed on the thirteen raw inputs by value, Smart.reset() drops it, and the identity-keyed control is what shows an in-place write from the pages is still seen. Approving.
| -- -- while the configuration is read on every wake that computes. So the values it was last built | ||
| -- from are kept here, and the table is rebuilt only when one of them differs. They are compared by | ||
| -- value, not by the identity of the tables they come from: the SmartFuel and battery pages write | ||
| -- into the session's tables in place. The kept table is returned itself, not a copy: a caller |
There was a problem hiding this comment.
This sentence is the one that was missing: the kept table is returned itself, so the rule for every future caller is now written down where the cache is defined rather than only in the pull request. Smart.wakeup (:620) obeys it today.
2 participants
What and why
Closes #768
Smart.wakeupintasks/events/telemetry_bg/smart.luacomputes once a second in every Lua state that runs the event runtime (the dashboard widget, the service widget, and the tool). On every such wake it calledgetSmartConfig, which created three closures (readLegacyLocalSource,getFirmwareSource,pick), parsed the API version, compared it against a new{ 12, 0, 9 }table and returned a new configuration table; and it joined seventostringresults into a string to decide whether to restart the estimate. The configuration only changes when a setting does, and the restart values only with the settings, the battery configuration, the active battery profile or the pack's cell count.This change:
cfgandbatteryPrefsare passed in) and makes the minimum API version a constant;session.apiVersion,smartfuel_config.smartfuel_mode,battery_config.smartfuelRemoteSource, the model preferencessmartfuel_source,calc_localandsmartfuel_publish, and the seven tuning values aspickresolves them. They are compared by value, not by the identity of the tables they come from, because the SmartFuel and battery pages write into the session's tables in place.Smart.reset()drops the kept configuration as well, so the first wake after the SmartFuel or Battery page's save (both call it) rebuilds it;What it costs, master
1b8d789bfagainst this branchThe reproduction from #768 on desktop Lua 5.3.6 (64-bit), 1 000 calls with the collector stopped, extended with an instruction count (
debug.sethook(..., "", 1)), a second fixture on the local voltage source, and rows for a wake that has to rebuild. Both trees in the same run, in both orders, with the string table pinned:Smart.reset()The branch's bytes for an unchanged wake are a fixed amount per measured loop rather than per call: 1.5 bytes per call over 1 000 calls and 0.15 over 10 000, and the same on a wake that returns at the interval gate and runs none of the changed code. So an unchanged wake stops producing about 57 kB of garbage a minute in each Lua state that runs it. A wake that has to rebuild costs about 25 to 55 instructions more than before, for the comparison; that happens once per settings change.
lua5.3 bin/accounting/measure.lua --check, master against this branch: 142 rows, 0 failures on both. Rows that moved:unit.events.wakeuptheme.@aerctheme.@aerc-ntheme.@rt-rctheme.@rt-rc-ntheme.@srb-rcpass.tuning.primepass.serviceunit.telemetry.drainunit.telemetry.handoffpass.serviceis the service widget's dearest pass, the one where SmartFuel computes for the first time and builds its configuration; it rises by 50 and stays well under its target of 7850. The two telemetry rows run without a connection, whereSmart.reset()runs on every wake and now clears one value more.Nothing was run on a radio for these figures; a radio's Lua is 32-bit, so the objects are smaller there, and their number is the same.
What does not change
The configuration and every restart decision, with the one exception at the end of this section. Master and this branch were loaded side by side, sharing one session, through 201 wakes in which the settings and the battery configuration change one value at a time, mostly by writing into the existing tables as the pages do: each tuning value, a value removed from the flight controller's SmartFuel configuration while the model preferences hold one, the cell count, the capacity, the minimum and full cell voltages, the reserve, a battery profile switch on firmware with per-profile cells, the local source, sensor publication, the firmware source, the API version, a reset, a disconnect and reconnect, and the tables replaced by equal and by different copies. Compared per wake: the eight configuration fields, every log line (including
smart reset), every published sensor value, the hand-over torfsuite.session.smartfueland the estimator's state. Master and this branch agree on all 201 wakes. Copies of this branch that key the configuration on table identity, that ignore any one of the thirteen inputs, or that drop any one of the seven restart values each disagree, so the sequence sees every input.The exception, by design: the restart values are now compared as numbers, so a value that changes only between an integer and a float of the same size no longer restarts the estimate -- a cell count reported as
6.0where it was6, or a capacity of2000.0where it was2000. The string comparison restarted it, because Lua 5.3 (the version EdgeTX ships) prints the two differently. A further run of the same sequence with one more step,Cel#reporting6.0after6, shows it: master restarts at that step and this branch does not, and the two agree on every wake before it. Whether a radio actually produces such a flip was not checked; EdgeTX hands a sensor without decimals to Lua as an integer.On the EdgeTX simulator
EdgeTX 2.12 simulator (TX16S MK3 screen, the dashboard widget with Urban) connected to a Rotorflight 4.6.0 SITL flight controller. The flight controller's cell count was set to 0 (auto-detect) and its SmartFuel source to off, so the radio computes the estimate itself, from the current. The same scripted sequence ran on three cards: master, this branch, and a copy of this branch that keys the configuration on table identity (broken on purpose, to show the sequence can tell). The in-place write in the last step was made by a few lines added to the card's copy of the suite, the same on all three cards.
smart reset source=1smart reset source=1The figures are the value SmartFuel hands to the dashboard, read every two seconds; the dashboard's fuel gauge showed 75% and 23% on master and on this branch. No disconnect in any run. The allocation figures above were not read off the simulator.
Related: #409 (the issue this is the remainder of) and #423 (the pull request that closed it). #788, the pull request for #763, removes an allocation of the same kind from the dashboard's logic tick. This branch shares no file with any open pull request.
Checklist
See CONTRIBUTING.md for each rule.
release-notes/, or a line below says why not.docs/is updated, or a line below says why no page changes.Documentation: no page -- nothing a pilot can observe changes; SmartFuel computes the same figure and restarts on the same changes, and the SmartFuel pages under
docs/pages/describe the sources and the cell count rule, not how often the configuration is read.