Feature/BCON lite - Beam Control without deflection#88
Conversation
…/EBEAM_dashboard into feature/beams-off-button
…from Cathode Heating
…from Cathode Heating
…/EBEAM_dashboard into feature/beams-off-button
|
Result of Asking Codex in VSCode the question "I want your assessment if this code is good to go. Tell me if the code is ready to merge into the develop branch. Is there anything critical that needs to change before merging? Will it break develop in any way? What do the changes in the new code do and are they good changes? Point out any critical errors, in particular logic errors or anything that might break the dashboard. Make sure you do a diff of this branch relative to develop.": It said that this won't break develop. It pointed out two issues, the second one we were aware of: Findings (ordered by severity) High: Beam arming can report success even when no BCON hardware/connection exists.
Risk: operator sees system armed while hardware is absent/disconnected, which is a safety/UX mismatch. Medium: Full test suite passes but still emits thread warnings in unrelated interlock test code.
|
In response to the first finding, is it necessary for the Arm Beams toggle to have a hardware connection. I initially considered this a software interlock however we should recheck its current behavior with the bcon_driver |
@ColeMov and I discussed, we are going to keep arm beams software-only as it was originally intended. |
|
Arm beams is now software only and toggle still works in frontend. |
|
Most recent version was largely successful and tested as documented in BCON-BTEST-010: Firmware and Dashboard Integration Test https://docs.google.com/document/d/11xRWqmwaAlp3FNkDllFCPtVAfsml8pKOYc3Xju00kMQ/edit?usp=sharing |
Resolved conflicts in: - main.py: kept both 'KnobBox' and 'BeamPulse' in SUBSYSTEMS list - dashboard.py: kept ports_after_id from #105 + toggle images and BeamPulse subsystem init from BCON-lite - subsystem/cathode_heating/cathode_heating.py: kept HEAD's LUT-based logic (from #90) plus BCON-lite's turn_off_all_beams and set_target_current additions Co-authored-by: sbasu107 <40325803+sbasu107@users.noreply.github.com>
|
Tomorrow, I can take a look at the merge conflicts from cathode heating created by merging PR #90. This should be resolved quickly. |
![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: 23dc141d7b
ℹ️ 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".
| def disarm_beams(self) -> bool: | ||
| self.beams_armed_status = False | ||
| self.set_all_beams_status(False) | ||
| if self.bcon_driver: | ||
| self.bcon_driver.stop_all() |
There was a problem hiding this comment.
Disarming beams does not stop an in-flight CSV sequence thread, so if an operator disarms (or triggers E-STOP, which calls disarm_beams) during a run, _sequence_worker can continue issuing sync_start commands on later steps and re-drive channels after the stop action. This is a safety regression because software disarm is expected to halt further pulse commands immediately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I would consider this fine for now and should not block the merge. We do not currently have plans to operate using a CSV sequence at this time. We should note this as a future issue and either target the thread to stop when calling disarm_beams or remove the feature entirely.
| self._connected = ok | ||
| self._poll_errors = 0 | ||
| self._reset_cached_state() | ||
|
|
||
| if ok: |
There was a problem hiding this comment.
When the post-open validation read fails (e.g., device not ready after settle), connect() sets ok=False and reports failure, but it never closes self._serial on that path. That leaves the COM port open while the driver is marked disconnected, which can block external recovery tools and creates inconsistent connection state until a later reconnect/disconnect happens to close it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 0c8a972. On post connection validation failure the COM port will be closed to match connection state.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c8a9729b5
ℹ️ 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.subsystems['Beam Pulse'] = beam_pulse_subsystem | ||
| else: | ||
| # placeholder if module not importable | ||
| container = self.frames.get('Beam Steering/Pulse', self.frames['Process Monitor']) |
There was a problem hiding this comment.
When BeamPulseSubsystem cannot be imported, this fallback writes the placeholder label into Process Monitor instead of the Beam Pulse pane, because it looks up 'Beam Steering/Pulse' and then falls back to self.frames['Process Monitor']. In that failure mode, users get a misleading Process Monitor panel while the Beam Pulse frame stays empty; the fallback should target the Beam Pulse frame directly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not addressing at the moment, but saved as issue #111
| frame = ttk.Frame(self.com_port_menu) | ||
| frame.pack(fill=tk.X, padx=5, pady=2) | ||
| ttk.Label(frame, text="Beam Pulse:").pack(side=tk.LEFT) | ||
| port_var = tk.StringVar(value=self.com_ports.get('Beam Pulse', '')) |
There was a problem hiding this comment.
The new COM-port field is initialized from self.com_ports.get('Beam Pulse', ''), but this commit registers the subsystem key as BeamPulse in main.py (SUBSYSTEMS). As a result, the Beam Pulse dropdown appears blank even when a port is already configured, and pressing Apply can overwrite the in-memory config with an empty value. Read both aliases (or the canonical key) when initializing this control.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not fixing at the moment, but tracked as issue #112
bwalkerMIR
left a comment
bwalkerMIR
left a comment
There was a problem hiding this comment.
This is a big PR for introducing BCON to the dashboard, was well tested by the team, and looks good. Codex found some bugs that were fixed over the last day, and others were low priority so made to issues. Approved.
8 participants
PR Review Checklist
@codex reviewfrom PR.gitignorefiles were not committedGitHub Copilot Generated Summary
This pull request introduces the new Beam Pulse (BCON) subsystem to the project, providing both hardware driver and GUI control for beam pulsing via RS-485 serial communication. It includes comprehensive documentation for both the backend driver (
instrumentctl/BCON) and the frontend GUI subsystem (subsystem/beam_pulse), adds a full test suite for cathode heating beam-off logic, and updates the dashboard's main module to include the new subsystem.Major new features and documentation:
BCON driver and documentation:
instrumentctl/BCON/README.mdwith detailed documentation for the BCON (Beam Controller) driver, including hardware interface, command set, telemetry format, API reference, error handling, and usage examples.instrumentctl/BCON/__init__.pyto expose the BCON driver, enums, utility functions, and hardware register constants for external use.Beam Pulse GUI subsystem and documentation:
subsystem/beam_pulse/README.mddescribing the newBeamPulseSubsystem, its GUI features, hardware communication, API, integration, and troubleshooting. This subsystem provides a Tkinter-based interface for controlling beam pulse hardware via the BCON driver.Testing and workflow changes:
citestfiles/CathodeHeating/beams_off_test.py, a comprehensive unittest suite for the cathode heating subsystem's beam-off logic, covering success, failure, exception handling, idempotency, and UI update scenarios.Dashboard integration:
BeamPulsesubsystem in the dashboard's main subsystem list inmain.pyto enable its use in the application.