fix(bioreactor-v1): default impeller_pwm to 100, not full speed - #61
Conversation
impeller_pwm defaulted to 255 — the top of its own range. Every other actuation default in the file is inert or gentle (both enables false, both dose rate caps 0, both startup delays 60 s), which makes 255 read as deliberate when it is more likely just the maximum. impeller_enable correctly defaults to false, so nothing moves on entering AUTO. But enabling the impeller is the single obvious next action, and the runbook says to enable conservatively without mentioning a PWM — so a first-user following it literally gets a full-speed start on their first autonomous run. On this machine that is the largest current transient it produces, on a shared and undersized 12 V rail. 100 was run on the bench and visibly stirred the vessel. Raising it should be a deliberate act. Also documents that parameters are not persisted. They revert to these defaults on every runtime restart, including across an upgrade, silently and with no log line — so the defaults are not merely a starting point, they are what the machine does every time it comes back. That is the half of this hazard a better default does not fix: an operator who tunes the impeller down mid-run gets full speed again after any restart. Tracked separately against the runtime. Applied to the automation and full profiles in both config/ and config-release/, and to the parameter table in docs/runbook.md. Closes #60
…ange From review. Both are claims I asserted without checking, in text whose whole job is to be trusted. The runbook note said the revert happens "silently and with no log line". That is false and it points operators away from the evidence: the behaviour tree logs every parameter it reads on every tick (core/automation/bt_nodes.cpp:402), so at tick_rate_hz: 2 the live value is in the journal twice a second. An operator who believed the note would skip the one command that shows them, in a second, that they are back at the default after an upgrade. Now says what is true — nothing announces the revert — and gives the grep. The YAML comment asserted that a full-speed start is the largest current transient this machine makes. There is no measurement of that anywhere: no brownout, no throttled event, no inrush record. Worse, the impeller failure it leans on happened while commanded at PWM 100 — the value this change makes the default — so the comment implied a reduction in exposure that this change does not deliver. Replaced with what is actually established: impeller_pwm is a continuous-duty setpoint gated only by impeller_enable, unlike the dose pumps which also carry a rate cap of 0, and 100 ran continuously through a full culture. The default change itself is unaffected and still right.
Review gate: returned DO NOT MERGE, both findings validThe config change itself came back clean and well-evidenced. What did not was the text I wrote around it — twice, asserting things I had not checked. Fixed in 2f04118. 1. The safety note was factually wrong, in the direction that hurtsI wrote that a parameter revert happens "silently and with no log line". The behaviour tree logs every parameter it reads on every tick: At A safety note that trains people away from the available evidence is worse than no note. It now says what is true (nothing announces the revert) and gives the command. 2. I asserted an electrical cause I never measured — and it points the wrong wayThe YAML comment claimed "a full-speed start is the largest current transient this machine makes, on a shared 12 V rail." There is no measurement of that anywhere: no brownout, no Worse: the impeller failure I was leaning on happened while commanded at PWM 100 — the value this PR makes the default. So the comment implied a reduction in exposure that this change does not deliver, and four config files would have carried that as settled fact. Replaced with what is actually established: What the review confirmed
Filed separatelyanolishq/anolis-workbench#305 — |
1 participant
Closes #60.
Problem
impeller_pwmdefaulted to 255 — the top of its own range. Every other actuation default in the file is inert or gentle:impeller_enablefalsedose1_enable/dose2_enablefalsedose1_max_pulses_per_hour/dose2_max_pulses_per_hour0dose1_startup_delay_s/dose2_startup_delay_s60impeller_pwmwhich makes 255 read as deliberate when it is more likely just the maximum.
impeller_enablecorrectly defaults tofalse, so nothing moves on entering AUTO. But enabling the impeller is the single obvious next action after activating automation, and the runbook says to enable conservatively without mentioning a PWM — so a first-user following it literally gets a full-speed start on their first autonomous run. On this machine that is the largest current transient it produces, on a shared and undersized 12 V rail.We set 100 by hand on the bench specifically to avoid this. That step existed nowhere in the procedure.
Change
impeller_pwmdefault 255 → 100, in theautomationandfullprofiles, in bothconfig/andconfig-release/. 100 visibly stirred the vessel on the bench; raising it should be a deliberate act.docs/runbook.mdupdated to match.On the persistence half
A better default reduces the blast radius but does not fix the other half of this hazard, which the issue picked up in a comment: automation parameters revert to config defaults on every runtime restart, including across an upgrade, silently and with no log line. So an operator who tunes the impeller down mid-run gets the default back after any restart — and the direction of that silent change is toward more actuation.
Filed against the runtime as anolishq/anolis#273, including the detail that
parameter_manager.hppdocuments "optional persistence back to YAML (disabled by default)" while no implementation exists — which is how a reader concludes it is switched off rather than absent.This matters immediately: the next planned action on the rig is an upgrade to 0.1.41, which is a restart.
Verification
Config-only change; this repo has no build. All ten
bioreactor-v1YAML files parse,machine-profile.yamlis untouched and still parses, and noimpeller_pwmdefault of 255 remains anywhere in the project.The behaviour tree reads the parameter through
GetParameterand passes it asmotor2_pwmtodcmt0(behaviors/bioreactor_stir_dual_dosing.xml:16,43), so the value flows unchanged — only its starting point moves.