[feat] omicronxx: optimize speed when changing the light intensity#3302
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request optimizes the omicronxx driver to significantly reduce latency when changing light intensity by only sending commands to channels that have actually changed, rather than updating all channels regardless of whether they changed.
Key changes:
- Implements state tracking via
_current_powerto remember previous power values for each channel - Modifies
_updateIntensities()to compare current vs target power per channel and skip unchanged channels - Optimizes master device control to only toggle when transitioning between all-off and not-all-off states
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe driver now maintains per-instance per-device power state in Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test / Client
participant Driver as GenericxX Driver
participant USB as USBAccesser
participant Device as Omicron Device (per-channel)
Note over Driver,USB: Driver holds _current_power and receives target intensities
Test->>Driver: request intensity change (targets)
Driver->>Driver: compute current_all_off, per-device cp, tp
alt tp > cp and cp == 0 (turning on)
Driver->>USB: send "turn on device" command
USB->>Device: activate device
Driver->>USB: send "set power" command (clipped)
USB->>Device: set intensity
else tp == 0 and cp > 0 (turning off)
Driver->>USB: send "deactivate device" command
USB->>Device: deactivate and set 0
else cp != tp (power change while on)
Driver->>USB: send "set power" command (clipped)
USB->>Device: set intensity
else cp == tp
Note over Driver: skip device (no command)
end
alt master transition from all-off -> non-all-off
Driver->>USB: send "turn on master" command
else master transition to all-off
Driver->>USB: send "turn off master" command
end
Driver->>Driver: update self._current_power with new states
Driver->>Test: respond (completion)
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/odemis/driver/omicronxx.py (1)
649-649: Redundant initialization of_current_power.This assignment is immediately overwritten at line 665 before being used. Consider removing this line or adding a comment explaining the defensive initialization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/odemis/driver/omicronxx.py(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linting
src/odemis/driver/omicronxx.py
[error] 1-1: PNG metadata check detected forbidden chunks in file. Contains forbidden metadata chunks.
🪛 Ruff (0.14.8)
src/odemis/driver/omicronxx.py
705-705: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (python)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (3)
src/odemis/driver/omicronxx.py (3)
663-666: LGTM!The "trick" to simulate all devices being on before turning them off is a clean approach to ensure proper initialization commands are sent. The comment clearly explains the rationale.
691-730: LGTM - Optimization logic is correct.The per-channel comparison and selective command sending achieves the PR objective of avoiding unnecessary overhead. The logic correctly handles:
- Skipping unchanged channels
- Proper sequencing for on transitions (LightOn → setLightPower → activate)
- Proper sequencing for off transitions (deactivate → LightOff → setLightPower)
- Master device state transitions only when crossing the all-off boundary
705-722: Consider addingstrict=Truetozip()for defensive programming, but verify Python 3.10+ support first.The
strict=Trueparameter would raise aValueErrorifself._devices,self._current_power, andpowerhave different lengths, catching potential bugs early. However, this parameter requires Python 3.10+, and the project's minimum Python version could not be determined from the codebase. Verify the minimum supported Python version before applying this refactor.Note: The current code is safe by design—these three iterables are guaranteed to have matching lengths because
self._power.valueis initialized withlen(self._devices)elements, and bothself._current_powerand thepowerparameter are always synchronized to this length.
tmoerkerken
left a comment
tmoerkerken
left a comment
There was a problem hiding this comment.
A few unit tests would be nice to prove it works :)
The most typical change is to change just one channel. However, until now, the driver would always update all channels. Changing a channel can take ~50ms. It's not so long, but on systems with 4 channels, when turning on and off one channel quickly, that would end up costing around 300ms overhead. => Only send commands for the channel that changed
pieleric
force-pushed
the
feat-omicronxx-optimize-speed-when-changing-the-light-intensity
branch
from
6ad6503 to
df27742
Compare
It's just an optimisation, so the behaviour should stay identical. That makes it a little harder to test, but with the help of copilot, I've added a wrapper to count the number of commands passed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/odemis/driver/omicronxx.py (1)
663-666: Document the initialization trick more clearly.The comment describes the trick but doesn't explain why setting
_current_powerto max power (while hardware is actually off) is necessary. Consider adding a brief explanation that this ensures all channels get proper initialization commands (turn off + set to 0) on the first call to_updateIntensities.src/odemis/driver/test/omicronxx_test.py (1)
21-23: Consider removing unused import.The
Listtype from the typing module is imported but doesn't appear to be used in the test file. Consider removing it to keep imports clean.-from typing import Any, List +from typing import Any
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/odemis/driver/omicronxx.py(3 hunks)src/odemis/driver/test/omicronxx_test.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/odemis/driver/omicronxx.py (1)
src/odemis/model/_vattributes.py (5)
value(381-382)value(385-388)value(781-786)value(789-790)min(924-925)
src/odemis/driver/test/omicronxx_test.py (1)
src/odemis/driver/omicronxx.py (2)
USBAccesser(60-178)sendCommand(124-151)
🪛 Ruff (0.14.8)
src/odemis/driver/omicronxx.py
705-705: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (4)
src/odemis/driver/omicronxx.py (3)
649-649: Verify initialization synchronization with power.value.The initialization of
_current_powerfromself.power.valueassumes thatpower.valuereturns a consistent snapshot. Since power is a ListContinuous VA that can be modified externally, ensure that this initialization happens before any subscribers can modify the value, or that concurrent modifications are handled safely.
692-693: No action needed—timing values are consistent.The comment states "each command takes ~15 ms" while the PR description mentions "changing a single channel can take ~50 ms". These values are not inconsistent: a channel change involves multiple commands (typically 3 per operation:
LightOn(),setLightPower(), andactivate()). Therefore, 3 commands × ~15 ms ≈ 45–50 ms per channel, which aligns perfectly with the PR description. The two measurements refer to different granularities of the same operation, not contradictory claims.
697-697: Master state transitions are correctly guarded by state change conditions; however, manual master modifications won't be actively recovered.The initial state (line 665) correctly triggers master turn-off on first call:
_current_poweris set topower.range[1], then_updateIntensities()is called with actual power (0), making the condition at line 727 (all_off and self._master and not current_all_off) evaluate true.The turn-on condition (line 697) and turn-off condition (line 727) properly ensure master state changes only during device power transitions. However, if the master device is manually turned off/on externally (via direct device control), the logic won't detect or recover from the mismatch—the master state will remain inconsistent until the next power transition occurs (
all_offornot all_offcondition is triggered). This is acceptable only if external master modifications are not expected; otherwise, active state validation should be added.src/odemis/driver/test/omicronxx_test.py (1)
42-61: LGTM! CountingUSBAccesser implementation is clean.The wrapper correctly intercepts
sendCommandcalls while delegating all other attribute access to the wrapped instance. The type hints improve code clarity.
The most typical change is to change just one channel. However, until
now, the driver would always update all channels. Changing a channel can
take ~50ms. It's not so long, but on systems with 4 channels, when
turning on and off one channel quickly, that would end up costing around
300ms overhead.
=> Only send commands for the channel that changed