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

run-unit-tests on ykush devices #12561

Merged
merged 10 commits into from
Jan 29, 2024
Merged

Conversation

AviaAv
Copy link
Contributor

@AviaAv AviaAv commented Jan 8, 2024

Tracked on [LRS-991]



if __name__ == '__main__':
device = Acroname()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I say lets call it acroname (lower case) and not device as we call our cameras devices

@maloel
Copy link
Collaborator

maloel commented Jan 9, 2024

In general:

  • It seems both acroname and ykush are really implementations of some common 'hub' interface, except there isn't a common interface defined
    • Otherwise I don't see why we're going the object-oriented route: the modules themselves are objects, too
    • We're not envisioning a use-case where more than one acroname, or any combination of hubs exists, right?
  • The relay is just a forwarder - I'm not sure I see the need for the relay to duplicate all the functions of the actual hub implementation
    • E.g., disable_ports(): once you have an instance of a hub, why not call this function directly on it?

I think the direction is good, but maybe the 'relay' (I like 'hub' better) is your interface that acroname and ykush implement? You still need a create() function, but that's about it. Once you have a hub instance (that's your has_relay), you just need to use it...

unit-tests/py/rspy/ykush.py Outdated Show resolved Hide resolved
unit-tests/py/rspy/ykush.py Outdated Show resolved Hide resolved
unit-tests/py/rspy/ykush.py Outdated Show resolved Hide resolved
return None
except acroname.NoneFoundError():
return None
except:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not valid, I get:

TypeError: catching classes that do not inherit from BaseException is not allowed

It should be:

except BaseException:

Same in line 176.

return acroname.Acroname()
except ModuleNotFoundError:
return None
except acroname.NoneFoundError():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the ()

@@ -41,321 +41,328 @@ def usage():
log.w( 'No acroname library is available!' )
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is now less relevant now that there're alternatives - let's turn this into a log.d

Copy link
Collaborator

Choose a reason for hiding this comment

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

And same in ykush

Copy link
Collaborator

Choose a reason for hiding this comment

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

And make it lower-case and remove the ! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I'll do the same on ykush.py (log.w -> log.d)

Copy link
Collaborator

@maloel maloel left a comment

Choose a reason for hiding this comment

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

@Nir-Az this needs to be coordinated with changes to libCI, so I'm not merging yet

@maloel maloel merged commit ec0e15a into IntelRealSense:development Jan 29, 2024
16 of 17 checks passed
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