-
Notifications
You must be signed in to change notification settings - Fork 4.9k
devices.py --list now shows DDS devices; fix error #13703
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
Conversation
|
Since @AviaAv is working on the same code that we can add D555 to LibCI best that we do a cross review :) |
unit-tests/py/rspy/devices.py
Outdated
| _context = rs.context( { 'dds': False } ) | ||
| settings = {} | ||
| if disable_dds: | ||
| settings['dds'] = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be better to do: settings['dds'] = { 'enabled' : False}
Both options work for disabling DDS, but when we use settings['dds'] = True we still won't see DDS devices, so best to be consistent about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a shortcut -- if dds is not an object, then everything inside the missing object gets the default value and enabled becomes False.
But OK, will do.
|
|
||
|
|
||
| def query( monitor_changes=True, hub_reset=False, recycle_ports=True ): | ||
| def query( monitor_changes=True, hub_reset=False, recycle_ports=True, disable_dds=True ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't we want to show dds devices by default? disable_dds=False?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to change existing behavior
| if action == 'list': | ||
| query( monitor_changes=False, recycle_ports=False, hub_reset=False ) | ||
| query( monitor_changes=False, recycle_ports=False, hub_reset=False, disable_dds=False ) | ||
| map_unknown_ports() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but can you please explain why map_unknown_ports() is needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because otherwise I wouldn't know which port contains the ethernet device. :)
|
LGTM, some small comments |
Kept the old behavior, where DDS devices are not queried by default from
run-unit-tests...But now, at least from the command-line (which I use), you can do
devices --listand you'll see all devices including eth and it'll show you their ports.Also fixed a bug with eth devices port lookup.