-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fix playback flush halt + add UT #10024
Fix playback flush halt + add UT #10024
Conversation
|
||
''' | ||
This class is used to wait and verify a requested playback status arrived whithin a requested timeout. | ||
It blocked the calling thread and sample the playback status using a playback status callback from LRS API\ |
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.
\
a the end
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.
blocked
-> blocks
sample
-> samples
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.
👍
|
||
Usage should look like this: | ||
psv = PlaybackStatusVerifier( device ); | ||
psv.wait_for_status( 5, rs.playback_status.playing ) |
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.
If that's the usage, why a class? Why not just simply a function call?
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 wanted to separate the callback registration from the blocking wait.
This way the users can set the callback before the start/stop call and than wait for the required status
More flexible.
It also has member variables that better to encapsulate within a class.
This class is used to wait and verify a requested playback status arrived whithin a requested timeout. | ||
It blocked the calling thread and sample the playback status using a playback status callback from LRS API\ | ||
|
||
Note: LRS set_status_changed_callback function is a 'register' and not a 'set' function |
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 think maybe this comment needs to go in the ctor?
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.
👍
|
||
#If the status changes too fast let say , Stopped -> Playing --> Stopped , | ||
# we want fail the test as our sleep is too long and we will not catch the required status | ||
test.check( status_changes_cnt + 1 == self._status_changes_cnt, |
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.
Not sure I understand...
Stopped -> Playing -> Paused -> Playing
If you're waiting for Playing, more than one count is not necessarily bad... am I misunderstanding?
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.
The issue is with Stopped -> Playing -> Stopped (because we sample it after a sleep)
On this case we can miss the Playing status.
It's more tailored for a single status check and not for a whole process.
(an expected transition check)
I can find a way to expend it for more usages if you think it's necessary, for the tests in the current usage I expect 1 change.
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 think the logic behind a check that can make a test fail needs to be good.
We can talk about it if you want.
Up to you.
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.
If you do choose to keep it, please note the limitation at the very least
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 change it to use a 10 [ms] sample rate (can be overridden by the user) and add comments about limitations and how to use this function.
This logic was chosen because LRS use register callback instead of set, otherwise I would have accepted the required status at the Ctor and verify its arrival at the callback.
This logic is good enough for our test considering this behavior.
1000/1000 iteration passed on a local test.
Tracked on [LRS-329]