Skip to content

Conversation

@beniroquai
Copy link
Collaborator

This PR focusses on a simplifcation of the readme (e.g. install via UV).

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR simplifies the README documentation with a focus on UV (a fast Python package installer) as the primary installation method. The changes also include various code improvements across multiple controller and manager files.

Changes:

  • Simplified README to emphasize UV-based installation and removed extensive platform-specific documentation
  • Fixed spelling errors throughout the codebase (corrected "availableIlliminations" to "availableIlluminations")
  • Refactored illumination parameter handling to use list-based approach instead of individual parameters
  • Added error handling for camera trigger source configuration
  • Improved objective position detection logic

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 14 comments.

Show a summary per file
File Description
README.md Streamlined installation instructions to focus on UV, removed outdated platform-specific guides, fixed typos
ESP32LEDMatrixManager.py Added channel_index attribute for LED matrix
hikcamera.py Added error handling for trigger mode/source setting, changed auto_trigger default, upgraded debug to info logging
experiment_performance_mode.py Refactored illumination parameters from dict to list, added synchronization delays with TODOs
experiment_normal_mode.py Conditionally add timing workflow step only when n_times > 1
experiment_mode_base.py Updated illumination parameter preparation to handle LED channel detection
TimelapseController.py Fixed spelling: availableIlliminations → availableIlluminations
ObjectiveController.py Added logic to detect current objective position, with position recalibration
MCTController.py / MCTController.py_OLD Fixed spelling: availableIlliminations → availableIlluminations
ExperimentController.py Refactored illumination API to use list instead of individual parameters

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 807 to 814
ret = self.camera.MV_CC_SetEnumValue("TriggerMode", MV_TRIGGER_MODE_ON)
if ret != 0:
self.__logger.error(f"Set TriggerMode failed! ret [0x{ret:x}]")
return False
ret = self.camera.MV_CC_SetEnumValue("TriggerSource", MV_TRIGGER_SOURCE_SOFTWARE)
if ret != 0:
self.__logger.error(f"Set TriggerSource failed! ret [0x{ret:x}]")
return False
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error handling has been added for setting TriggerMode and TriggerSource, but the function returns False on error while it doesn't have an explicit return type declaration. This creates an inconsistent return pattern where success has no explicit return (implicit None) while failure returns False. Consider either returning True on success or making the return type consistent throughout the function.

Copilot uses AI. Check for mistakes.
data: Dictionary with completion information
"""
self._logger.info(f"Stagescan completion signal received: {data}")
time.sleep(0.5) # Small delay to ensure all processing is done # TODO:
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard-coded sleep delays (0.5 seconds) are added with TODO comments suggesting uncertainty about the approach. These arbitrary delays can impact performance and suggest potential race conditions or synchronization issues that should be addressed properly rather than with fixed delays. Consider using proper synchronization mechanisms like checking actual completion status or using event-driven approaches instead of fixed sleeps.

Copilot uses AI. Check for mistakes.
README.md Outdated
<a href="https://discord.gg/y4Su9PpEvX">
<img src="https://cdn.prod.website-files.com/6257adef93867e50d84d30e2/636e0a6918e57475a843f59f_icon_clyde_black_RGB.svg" alt="W3Schools.com" width="100">
</a>
It is a fork - that has been detached from the original ImSwitch fork: https://github.com/ImSwitch/ImSwitch
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The phrase "has been detached from the original ImSwitch fork" is confusing. If this is a fork, it cannot be "detached from" a fork. Consider rephrasing to something like "This is a fork of the original ImSwitch project" or "This is a fork that has diverged from the original ImSwitch repository".

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

README.md Outdated
<a href="https://discord.gg/y4Su9PpEvX">
<img src="https://cdn.prod.website-files.com/6257adef93867e50d84d30e2/636e0a6918e57475a843f59f_icon_clyde_black_RGB.svg" alt="W3Schools.com" width="100">
</a>
It is a fork - that has been detached from the original ImSwitch fork: https://github.com/ImSwitch/ImSwitch
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in the phrase "focusses on". The correct spelling is "focuses" (with one 's').

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

README.md Outdated

```
sudo apt-get install libxml2-dev libxslt-dev
For this, please reffer to the bash files in this repo: https://github.com/openUC2/ImSwitchDockerInstall
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in "reffer". The correct spelling is "refer".

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback


# Check if we received all expected frames
if self._frame_count >= expected_frames:
time.sleep(0.5) # Small delay to ensure all processing is done # TODO: We should rather count stored frames not camera triggers?
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another hard-coded 0.5 second sleep with a TODO suggesting it should count stored frames rather than camera triggers. This is the same performance and synchronization issue as line 717. The TODO provides a better alternative approach that should be implemented.

Copilot uses AI. Check for mistakes.
self.I_max = 255
self.intensity=0
self.enabled = False
self.channel_index = "led"
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The channel_index is being set to lowercase "led", but other parts of the codebase (e.g., ESP32LEDLaserManager.py line 96) check for uppercase "LED" using exact string comparison. This inconsistency could cause the LED channel to not be properly recognized. Consider either setting it to uppercase "LED" to match existing code patterns, or ensuring all comparisons are case-insensitive.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

README.md Outdated
<a href="https://discord.gg/y4Su9PpEvX">
<img src="https://cdn.prod.website-files.com/6257adef93867e50d84d30e2/636e0a6918e57475a843f59f_icon_clyde_black_RGB.svg" alt="W3Schools.com" width="100">
</a>
It is a fork - that has been detached from the original ImSwitch fork: https://github.com/ImSwitch/ImSwitch
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in "simplifcation". The correct spelling is "simplification".

Copilot uses AI. Check for mistakes.
README.md Outdated


### Option D: Install from Github (UC2 version)
This is an outdated version how you can install it through conda.
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grammatical error: "This is an outdated version how you can install it through conda" should be "This is an outdated version of how you can install it through conda" or "This describes an outdated method to install it through conda".

Copilot uses AI. Check for mistakes.
if abs(mPositionObjective - self._objectivePositions[self._currentObjective]) > 500:
self.calibrateObjective()
self._moveMotorToSlot(0)
self._currentObjective = 0 # TODO: We should compare the position with the configured positions and decided if in slot 0/1
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code sets self._currentObjective = 0 after all the logic to determine the actual objective position (lines 100-109). This hardcoded assignment at line 110 overwrites the carefully calculated position from the previous logic, making lines 100-109 effectively dead code. Either remove line 110 or remove the position calculation logic above it.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Copilot AI commented Jan 14, 2026

@beniroquai I've opened a new pull request, #210, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Contributor

Copilot AI commented Jan 14, 2026

@beniroquai I've opened a new pull request, #211, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Contributor

Copilot AI commented Jan 14, 2026

@beniroquai I've opened a new pull request, #212, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Contributor

Copilot AI commented Jan 14, 2026

@beniroquai I've opened a new pull request, #213, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI and others added 4 commits January 14, 2026 15:43
The review comment references a typo "focusses" → "focuses" in
README.md:5, but this word does not exist in any tracked files or git
history.

## Investigation Results

- Searched all markdown files, README.md (current and historical), and
entire codebase
- Checked commit dcee095 and all
ancestors
- The word "focusses" only appears in PR #209's description text: "This
PR focusses on a simplifcation..."

## Analysis

The review bot likely flagged the PR description rather than file
content. PR descriptions cannot be modified through code changes.

**Awaiting clarification:** Should I address other typos from the review
(e.g., "reffer" → "refer" on line 130), or is there a different
file/location for the "focusses" typo?

<!-- START COPILOT CODING AGENT TIPS -->
---

💡 You can make Copilot smarter by setting up custom instructions,
customizing its development environment and configuring Model Context
Protocol (MCP) servers. Learn more [Copilot coding agent
tips](https://gh.io/copilot-coding-agent-tips) in the docs.

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
The `ESP32LEDMatrixManager` was setting `channel_index = "led"`
(lowercase), causing it to fail case-sensitive comparisons for `"LED"`
in `ESP32LEDLaserManager.py` and breaking LED channel recognition.

## Changes

- Changed `channel_index` from `"led"` to `"LED"` in
`ESP32LEDMatrixManager.__init__()` to match:
- Case-sensitive equality checks in `ESP32LEDLaserManager.setEnabled()`
(line 96) and `setValue()` (line 114)
- Uppercase `"LED"` values in configuration files
(example_histo_daheng.json, FRAME2b.json, example_uc2.json)

```python
# Before
self.channel_index = "led"  # Would not match == "LED" checks

# After  
self.channel_index = "LED"  # Matches case-sensitive comparisons
```

<!-- START COPILOT CODING AGENT TIPS -->
---

💡 You can make Copilot smarter by setting up custom instructions,
customizing its development environment and configuring Model Context
Protocol (MCP) servers. Learn more [Copilot coding agent
tips](https://gh.io/copilot-coding-agent-tips) in the docs.

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: beniroquai <4345528+beniroquai@users.noreply.github.com>
Corrects spelling error in the "Installation of drivers" section of
README.md.

**Changes:**
- Line 130: Changed "reffer" to "refer"

<!-- START COPILOT CODING AGENT TIPS -->
---

💬 We'd love your input! Share your thoughts on Copilot coding agent in
our [2 minute survey](https://gh.io/copilot-coding-agent-survey).

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: beniroquai <4345528+beniroquai@users.noreply.github.com>
Addresses review feedback on contradictory phrasing in README.md line 5.

## Changes
- Rephrased "It is a fork - that has been detached from the original
ImSwitch fork" to "This is a fork that has diverged from the original
ImSwitch repository"

The original text was logically inconsistent—something cannot be
"detached from a fork." The new phrasing clearly states the relationship
to the upstream project.

<!-- START COPILOT CODING AGENT TIPS -->
---

💡 You can make Copilot smarter by setting up custom instructions,
customizing its development environment and configuring Model Context
Protocol (MCP) servers. Learn more [Copilot coding agent
tips](https://gh.io/copilot-coding-agent-tips) in the docs.

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: beniroquai <4345528+beniroquai@users.noreply.github.com>
@beniroquai beniroquai enabled auto-merge January 14, 2026 14:47
ethanjli added a commit that referenced this pull request Jan 14, 2026
@ethanjli
Copy link
Collaborator

ethanjli commented Jan 14, 2026

While you were working and I was waiting for Docker container images to download on my RPi, I also did a simplification of README.md in #208. I've now merged your changes into that PR, which I will be merging into the master branch soon. So you can go ahead and close/delete this PR; or you can rename it to something else if you don't want to throw away all the non-README.md changes you've made in this PR.

github-merge-queue bot pushed a commit that referenced this pull request Jan 14, 2026
This PR merges https://github.com/openUC2/ImSwitch-ReactAPP into this
repository so that we aren't constantly crapping React app build outputs
into this repo upon every new commit for the React app. This merger also
makes it simpler and less error-prone to commit API changes: a single
pull request in this repo would include all changes in both the React
app and the backend.

This PR also resolves #205 (incorporating changes made in #209).
@beniroquai beniroquai closed this Jan 14, 2026
auto-merge was automatically disabled January 14, 2026 22:28

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants