Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive hardware reset stress test for RealSense D400 and D500 devices. The test repeatedly resets devices and verifies they reconnect successfully, with configurable iteration counts based on test context (nightly vs weekly) and connection type (USB/GMSL vs DDS).
Changes:
- Added new stress test file with context-aware iteration counts (10-100 iterations depending on nightly/weekly context and connection type)
- Updated testing documentation to explain how to run nightly-only tests using
--contextflag - Added copyright header convention to agent instructions emphasizing use of current year
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| unit-tests/live/hw-reset/test-stress.py | New stress test that performs repeated hardware resets with proper timeout handling, device matching via serial numbers, and failure tracking across iterations |
| .github/skills/testing.md | Added documentation section explaining how to run nightly-only tests with --context flag and added --debug option documentation |
| .github/agent-instructions.md | Added explicit copyright header convention requiring current year for new files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@AviaAv forgot to add a nightly run to make sure it works. |
|
|
||
| # test:device each(D400*) | ||
| # test:device each(D500*) | ||
| # test:donotrun:!nightly |
There was a problem hiding this comment.
in nightly run, do we use both 'nightly' and 'weekly' contexts? if not, this test will not run on weekly
There was a problem hiding this comment.
Good question, checking
There was a problem hiding this comment.
Changed so that it will run nightly too now, will monitor it
There was a problem hiding this comment.
Also removed D585S as it fails and we are running an old version, will reopen after we update the LibCI FW
| # test:device each(D500*) | ||
| # test:donotrun:!nightly | ||
| # test:timeout 360 | ||
| # test:timeout:weekly 3600 |
There was a problem hiding this comment.
locally this syntax # test:timeout:weekly 3600 didn't work for me, let's please also test weekly CI run to make sure this test passes
| if pl == "D400": | ||
| return MAX_ENUM_TIME_D400 | ||
| if pl == "D500": | ||
| is_dds = ( d.supports( rs.camera_info.connection_type ) |
There was a problem hiding this comment.
this is already done before this function call, let's remove this and add global is_dds
There was a problem hiding this comment.
Correct, used the global one.
According to co-pilot, reading from global is permitted w/o the need of declaring the global usage
| new_dev_handle = None | ||
|
|
||
| log.d( f"[{i}/{iterations}] Sending HW-reset" ) | ||
| dev_for_reset.hardware_reset() |
There was a problem hiding this comment.
at the end of the loop we do dev_for_reset = new_dev_handle, but if added_sn == tested_sn is false, we will call hardware_reset on None
There was a problem hiding this comment.
Please review new code
| time.sleep( 1 ) # let the device settle before the first reset | ||
|
|
||
| failed_removal = [] | ||
| failed_reconnect = [] |
There was a problem hiding this comment.
Since we break in case of failure during the test loop, we have two mutually exclusive arrays, which hold up to one variable
This is fine if intended to for readability, but could be simplified
There was a problem hiding this comment.
Intentional — separate arrays give a clearer failure message distinguishing removal failures from reconnect failures.
| dev_for_reset = None # always the latest live handle — used for hardware_reset() calls | ||
| device_removed = False | ||
| device_added = False | ||
| new_dev_handle = None # updated by callback so each iteration gets the fresh handle |
There was a problem hiding this comment.
do we really need 3 device handles for the same device? Using only dev should be enough IMO
There was a problem hiding this comment.
We have 2 and we need it , the old and the new.
With 1 it sometimes work and sometimes not when the new device got a different address
I would normally only run at weekly context, but since it checks a bug we have missed, it seems OK to run a shorter version on nightly and a long one on weekly. |
| # Fails on D585S and D555 | ||
|
|
||
| # test:device each(D400*) | ||
| # test:device each(D500*) !D585S !D555S |
No description provided.