Feature: CCS and Beams Shutoffs from VTRX Pressure - #180
Conversation
(cherry picked from commit b660981)
…ontrol config (cherry picked from commit 9dface1)
(cherry picked from commit 6e31536)
(cherry picked from commit da1373f)
…oved remaining beam energy limit setting checks over to main control (cherry picked from commit 44e3921)
… outdated names (cherry picked from commit 9b15173)
…d cathode model (cherry picked from commit afccc55)
… ES440 temperature data (cherry picked from commit 60625ec)
…age on init (cherry picked from commit b83a2ce)
…ped polling, made the bcon and ccs shutoffs dependant on that setting
…arts unless CCS is outputting
… be set to 'None' rather than '0.0'. Emission limit checks for a finite emission prediction value >= 0 and treats 'None' as invalid
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0a06e2fc8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pass | ||
| finally: | ||
| self.flush_queued_logs() | ||
| self._update_main_control() |
There was a problem hiding this comment.
When the VTRX COM port fails to open at startup, start_serial_thread() is not called, so process_queue() is never scheduled; because the Main Control pressure callback is only invoked from this finally block, Main Control never receives the stale/unavailable-pressure updates that should start the CCS shutdown timer and beam-disable path. This affects systems booted with the VTRX unplugged or misconfigured while outputs are already active, which is exactly the stale-reading case the new guard is intended to handle.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Since the process queue can handle empty serial just fine, it can be started independently of the serial start.
Fixed in b9a46ad
In the situation where a VTRX COM port fails to open, and the process queue was independently started (like it is in b9a46ad, then Machine Status would be updated with invalid VTRX data and then would start the CCS timer and/or turn beams off
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9a46ad2d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cathode.vtrx_ccs_pressure_allows_output = ( | ||
| self._vtrx_ccs_pressure_output_status | ||
| ) |
There was a problem hiding this comment.
When Beam Pulse fails to initialize or is omitted, Dashboard never calls wire_beam_pulse() (it is only invoked after the BCON subsystem is constructed), so this block is the only place that gives Cathode Heating its VTRX pressure guard. In that context, if the BCON-disconnect guard is disabled, CathodeHeating.toggle_power_supply() sees vtrx_ccs_pressure_allows_output as None and skips the new high/stale-pressure block, allowing CCS output even though the VTRX CCS shutdown setting is enabled. Move this wiring to a Cathode/VTRX wiring path that runs whenever Cathode Heating is present.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a good catch. Very unlikely that the whole Beam Pulse subpanel does not initialize, but the fix is simple and easy, and the code being referenced should likely be moved anyway.
Fixed in ca4f73c
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca4f73c5be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Treat startup as already disabled until VTRX reports a safe pressure. | ||
| self._vtrx_pressure_beam_disable_latched = True |
There was a problem hiding this comment.
When the dashboard starts while BCON channels are already active, or the first VTRX updates are stale/high before any fresh safe sample arrives, this startup latch stays True and _handle_vtrx_bcon_pressure_update() returns without calling disable_all_beams() until a safe pressure clears it. In that scenario the new VTRX beam shutoff only blocks future beam starts and leaves currently active beams running under unsafe/unknown pressure; initialize this unlatched or explicitly disable active beams on the first unsafe update.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This was initially added so that Critical Logs, CCS and BCON shutdown were not triggered on dashboard startup. The failure scenario here is unrealistic in my opinion. Codex is neglecting the 1500ms BCON firmware watchdog.
The following situation has to happen in order for this issue to materialize:
T0: Beams are on and the dashboard crashes. The BCON firmware watchdog's last read is at
worst at 0ms at this point. The dashboard must crash for this scenario to work. Any normal
dashboard close down attempts a bcon disconnect.
T1: VTRX Pressure readings raise above 1e-5 mbar. This must happen after the dashboard
crashes, because if the pressure readings raised above that threshold while the dashboard
was active, it would attempt a BCON shutoff, and beams would not be on.
T2: The user manages to get to respond to the crash and immediately run py ./main.py and
click through the COM port config menu and relaunch the dashboard.
T3: The dashboard opens and the user clicks the manual BCON connect button. (The dashboard
normally waits 4.5s until init is complete before connecting to BCON
T4: The dashboard successfully connects to BCON before the 1500ms watchdog expires and
shuts beams off.
If everything through T4 completes, then yes, the dashboard would be running with beams on with vtrx pressure above 1e-5mbar.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bfd411adc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.disable_ccs_output_on_bcon_disconnect = True | ||
| self.disable_beams_on_vtrx_pressure_exceeded = True | ||
| # Treat startup as already disabled until VTRX reports a safe pressure. | ||
| self._vtrx_pressure_beam_disable_latched = True |
There was a problem hiding this comment.
With this initialized as already latched, _handle_vtrx_bcon_pressure_update() returns without calling disable_all_beams() for every unsafe or stale VTRX update until a fresh safe pressure is seen. If the dashboard restarts while BCON channels are already active, or if the first VTRX reading after startup is high/stale, the new pressure shutoff never sends the stop command and the channels can keep running in the unsafe state. Initialize unlatched or retry the shutoff once Beam Pulse is available instead of suppressing the first unsafe condition.
Useful? React with 👍 / 👎.
|
|
||
| def load_beams_estop_current_limit_ma(filepath=CONFIG_FILE, logger=None): | ||
| """Load the persisted +20kV Beams E-STOP current limit.""" | ||
| return _load_field(BEAMS_ESTOP_CURRENT_LIMIT_FIELD, filepath=filepath, logger=logger) |
There was a problem hiding this comment.
When upgrading an existing machine that has the old pos20kv.beams_estop_current_ma saved in usr/usr_data/beam_energy_warning_limits.json but no new Main Control config yet, this loader goes straight to _load_field() and writes the 0.7 mA default. The Beam Energy normalizer now drops the old key, so the operator's previously configured E-stop threshold is silently lost on first startup. Please read and migrate the legacy Beam Energy field before falling back to the new default.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Part of the melt pre-procedure will be to set and configure dashboard defaults. I don't think its worth supporting legacy JSON versions, especially because this setting is quite new and its probably only my development computer, the lab computer, and brandon's computer that have a saved json at all.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bfd411adc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.last_successful_read_time = time.time() | ||
| self.last_valid_pressure_value = pressure_value |
There was a problem hiding this comment.
When a VTRX frame has a valid pressure plus a warning/error segment (for example 972b ERR:16:UNSAFE FOR HV WARNING), line 379 sets self.error_state = True, so this new cache update is skipped. _update_main_control() then keeps publishing the previous last_valid_pressure_value as fresh until PRESSURE_READING_FRESH_SECONDS expires, so a transition from safe pressure to unsafe-with-warning can leave beam/CCS pressure guards seeing the old safe reading instead of tripping immediately; cache the parsed pressure/time before branching on the VTRX warning state or pass the parsed value directly to the callback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
VTRX now passes a firmware_error variable to Main Control. last_successful_read_time and last_valid_pressure_value functionality remained the same, but Main Control now responds to an explicit VTRX firmware error.
See e92c96d
… handle_serial_data() wrapper after I understood more about VTRX firmware errors
…m the last commit
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc0403ce63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Treat startup as already disabled until VTRX reports a safe pressure. | ||
| self._vtrx_pressure_beam_disable_latched = True |
There was a problem hiding this comment.
Initializing the beam pressure latch as already set means the first high-pressure, stale, or firmware-error VTRX update after dashboard startup hits the latched return in _handle_vtrx_bcon_pressure_update, so disable_all_beams() is not called. If BCON outputs are already active when the dashboard restarts or reconnects, this guard only blocks future starts and leaves the existing beams on until a safe reading arrives and a later unsafe transition occurs.
Useful? React with 👍 / 👎.
| allowed, error_message = self._vtrx_pressure_allows_output(action, log_failure) | ||
| if not allowed: | ||
| return False, error_message |
There was a problem hiding this comment.
The new pressure guard is only invoked from _emission_limit_allows_output, which covers mode/sync/sequence output commands; the Main Control CH A/B/C buttons call toggle_channel_enable() and that path writes set_channel_enable() directly without this helper. When the VTRX pressure is stale or above the limit, an operator can still enable a BCON channel even though the pressure guard is enabled, so the guard does not cover the channel-enable path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Previously, Main Control checks for enabling output were only done on a button click that would enable output. Emission Limit Allows Output should definitely not be called when enabling channels, since it relies using enabled channels to determine if it allows output of beams. (So that several cathodes can be heated to allow output if they are each individually below the limit, and beams can then only be turned on if the sum of enabled channels cathodes would be below the limit). So this behavior was essentially copied for the VTRX check. However, there is really no reason why an enable channel attempt can't check this.
I am going to add it in, since its a small change, and does not hurt anything.
See d63e9e9
|
@codex review |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
This PR has been thoroughly tested on the lab laptop. Items tested:
|
|
@bwalkerMIR This PR is ready for review and merge. This PR is quite long, and covers all automatic shutoffs, not just the new pressure shutoff. Around half of the added code is unit test code, it can be discarded if it gets in the way or is not necessary. bugfix/CCS-connection-status should be merged into develop before this PR is reviewed and merged. Once that branch is in, this PR should switch to target develop. |
2 participants
This PR expanded in scope from just adding pressure shutoffs. This PR now covers changes in all automatic shutoff features in the dashboard, as well as Predicted Emission LUT fallback with the Richardson-Dushman equation.
Main Areas of Change:
VTRX
Beam Energy
Main Control
Beam Pulse
Cathode Heating
This PR is based off of bugfix/CCS-connection-status (#173) This branch should only be reviewed and merged once that PR has been merged into develop.