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

RSDK-1724 RSDK-1714 RSDK-1713: Camera RC fixes #1773

Merged
merged 5 commits into from
Jan 19, 2023

Conversation

stevebriskin
Copy link
Member

@stevebriskin stevebriskin commented Jan 17, 2023

Some issues fixed in this PR, many of which are related:

  • RSDK-1713: inconsistent "live" vs. "Live"
  • RSDK-1714: likely caused by RSDK-1713
  • RSDK-1724: live and non-live images may appear at the same time
  • Toggling the camera off or switching to live mode did not stop the image refresh "thread" (GetImage requests were still being sent)
  • For non-live mode, the initial frame relied on a stale image from the live feed
  • Toggling the camera on/off reverted to a live stream without resetting the interval dropdown
camrc2.mov

CC @mcvella
CC @nataliajacobowitz

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Jan 17, 2023

const initStreamState = () => {
cameraStreamStates.set(props.cameraName, false);
};

const clearStreamContainer = (camName: string) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Copied from app.vue. Not happy that it's just modifying the DOM.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, looks fine for now but I'll make a ticket for removing all direct DOM manipulation from RDK.

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 18, 2023
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 18, 2023
@@ -87,14 +87,6 @@ const connectedFirstTime = new Promise<void>((resolve) => {
connectedFirstTimeResolve = resolve;
});

const selectedMap = {
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to camera.vue to avoid duplicating

if (time === 'Manual Refresh') {
viewFrame(cameraName);
} else {
viewFrame(cameraName);
Copy link
Member Author

Choose a reason for hiding this comment

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

logic change here: the first frame is fetched immediately. this fixes a bug where it would take 30 sec (or however many you have selected) for the first frame to come in. this is often not apparent because we start in Live mode, but if one switches to "30 sec" interval, toggle the cam off, toggle the cam on, then the screen would be blank for 30 sec.

resources: commonApi.ResourceName.AsObject[];
client: Client;
}

interface Emits {
(event: 'download-raw-data'): void
Copy link
Member Author

Choose a reason for hiding this comment

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

removed unused emits

@stevebriskin stevebriskin added the appimage Build AppImage of PR label Jan 18, 2023
@stevebriskin
Copy link
Member Author

QAed by @Fahmina

@nataliajacobowitz
Copy link

@micheal-parks I notice that I can edit the frequency drop down and type in the field. How can we fix that? Prime change?

@micheal-parks
Copy link
Member

@nataliajacobowitz Yep, I'll make a ticket for that.

@stevebriskin stevebriskin merged commit 0fc307b into viamrobotics:main Jan 19, 2023
randhid pushed a commit to randhid/rdk that referenced this pull request Feb 25, 2023
@stevebriskin stevebriskin deleted the fixcamcase branch April 25, 2023 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
appimage Build AppImage of PR safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants