Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add A module (disparity modulation) set and get test #12466

Merged
merged 8 commits into from Dec 4, 2023

Conversation

Tamir91
Copy link
Contributor

@Tamir91 Tamir91 commented Dec 3, 2023

Tracked on [LRS-971]

@Tamir91 Tamir91 requested a review from Nir-Az December 3, 2023 09:28
def test_amp_factor(am_device, new_a_factor: float):
"""
This function set new A Factor value to advance mode device
:am_am_device: advance mode device
Copy link
Collaborator

Choose a reason for hiding this comment

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

am_am please rename

"""
This function set new A Factor value to advance mode device
:am_am_device: advance mode device
:a_factor: new A Factor value
Copy link
Collaborator

Choose a reason for hiding this comment

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

new_a_factor?

:am_am_device: advance mode device
:a_factor: new A Factor value
"""
factor = am_device.get_amp_factor()
Copy link
Collaborator

Choose a reason for hiding this comment

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

rename to previous_amp_factor

am_device.set_amp_factor(factor)

log.d('Checking A factor: ' + str(new_a_factor))
test.check(am_device.get_amp_factor().a_factor - new_a_factor < 0.01)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Try using this API test.check_float_lists

factor.a_factor = new_a_factor
am_device.set_amp_factor(factor)

log.d('Checking A factor: ' + str(new_a_factor))
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for this log

depth_profile_depth = next(p for p in depth_sensor.profiles if p.stream_type() == rs.stream.depth)
depth_profile_infrared = next(p for p in depth_sensor.profiles if p.stream_type() == rs.stream.infrared)

test.start('Check that Disparity modulation receive values:')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please wrap all the logic with test.start
If you don't get into the if you are not in the test?
And you do finish it in the end?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can wrap with with test.closure( 'verify set/get of Disparity modulation' ):
and remove the test.finish.
Like here:

with test.closure( "No librs syncer; direct from server" ):

if depth_sensor and advance_mode_device:

depth_profile_depth = next(p for p in depth_sensor.profiles if p.stream_type() == rs.stream.depth)
depth_profile_infrared = next(p for p in depth_sensor.profiles if p.stream_type() == rs.stream.infrared)
Copy link
Collaborator

Choose a reason for hiding this comment

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

WHat are you doing with lines 41 + 42?

test_amp_factor(advance_mode_device, 0.1)
test_amp_factor(advance_mode_device, 0.15)
test_amp_factor(advance_mode_device, 0.2)
test_amp_factor(advance_mode_device, 0.0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

1/2 set is enough

@Tamir91 Tamir91 added the python label Dec 4, 2023
@Tamir91
Copy link
Contributor Author

Tamir91 commented Dec 4, 2023

Here is the commit with the changes.

@Tamir91 Tamir91 requested a review from Nir-Az December 4, 2023 09:52


device = test.find_first_device_or_exit()
depth_sensor = device.first_depth_sensor()
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Why do I need depth_sensor?
  2. Why do I need to close depth sensor if I didn't ask start streaming?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I thought I would use it and leave it in the code. Removed


with test.closure('Verify set/get of Disparity modulation'):
if depth_sensor and advance_mode_device:
a_factor_values = [0.05, 0.0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's check 0.01 & 0.05
0 is a special case which I don't know if is valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I found that 0 is a default value, so thought to return the default value at the end of the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked values out of scope [0, 0.2] and it gave me error.

@Tamir91 Tamir91 requested a review from Nir-Az December 4, 2023 13:54
Copy link
Collaborator

@Nir-Az Nir-Az left a comment

Choose a reason for hiding this comment

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

LGTM

@Nir-Az Nir-Az merged commit 56309b2 into IntelRealSense:development Dec 4, 2023
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants