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

fix PR #12378 missing recordable< recommended-pbs > #12420

Merged
merged 3 commits into from Nov 17, 2023

Conversation

maloel
Copy link
Collaborator

@maloel maloel commented Nov 17, 2023

PR #12378 removed the recommended_processing_blocks_base derivation but I forgot to add back the recordable< recommended_processing_blocks_interface > derivation.

This was not caught anywhere, isn't tested, and cannot be caught by compilation since the dependency is only using a dynamic_cast which then failed. The result was that the recommended processing blocks were not recorded, so playback didn't have them.

I tried adding a test using a software-device but got stuck because there's no public API to add recommended processing blocks to a sensor... I manually tested and made sure it now happens.

Tracked on [LRS-954]

@maloel maloel requested a review from Nir-Az November 17, 2023 08:53
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.

We do have record and playback UT.
i wonder what is missing for them to catch it

@maloel
Copy link
Collaborator Author

maloel commented Nov 17, 2023

We do have record and playback UT.

You're right, of course. I failed because I wanted it in a software-device. For a live device, we don't need to set the blocks first.
Added checks in the existing test. They fail on dev, and now succeed.

@maloel maloel merged commit 64ff9a6 into IntelRealSense:development Nov 17, 2023
16 of 17 checks passed
@maloel maloel deleted the pb-factory branch November 17, 2023 13:26
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.

None yet

2 participants