Skip to content

Fix #34: Created EVS checkout test#53

Open
czogby-nasa wants to merge 3 commits into
devfrom
34-create-evs-checkout-test
Open

Fix #34: Created EVS checkout test#53
czogby-nasa wants to merge 3 commits into
devfrom
34-create-evs-checkout-test

Conversation

@czogby-nasa
Copy link
Copy Markdown
Contributor

No description provided.

@czogby-nasa czogby-nasa added this to the v7.1.0 milestone May 8, 2026
@czogby-nasa czogby-nasa requested a review from ddstewar May 8, 2026 18:30
@czogby-nasa czogby-nasa self-assigned this May 8, 2026
@czogby-nasa czogby-nasa marked this pull request as draft May 8, 2026 18:31
@czogby-nasa czogby-nasa marked this pull request as ready for review May 8, 2026 18:42
Copy link
Copy Markdown
Contributor

@ddstewar ddstewar left a comment

Choose a reason for hiding this comment

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

I'll be holding approval until the 7.0.1 build has been moved to the main branch and that has been confirmed with a good RfR. This will need to remain unmerged until then.

Here are my questions/comments


# Check final command count has incremented by the number of commands sent
wait_check(f"<%= target_name %> CFE_EVS_HK COMMAND_COUNTER >= {cmd_count} + 21", 30)
wait_check(f"<%= target_name %> CFE_EVS_HK COMMAND_ERROR_COUNTER == 0", 1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It was not established that the error counter was 0 at the onset of the test. Either compare this to the initial of error counter or remove this line.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you keep it, change this to a check() call and not a wait_check()

set_line_delay(0.0)

# Check final command count has incremented by the number of commands sent
wait_check(f"<%= target_name %> CFE_EVS_HK COMMAND_COUNTER >= {cmd_count} + 21", 30)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is a wait of 30 seconds needed here? The reset wait is 10 seconds. What behavior prompted this to be 30 seconds?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because it's a lot of commands, and they can't all execute simultaneously. I saw they definitely take more than one telemetry cycle. I reduced it from the standard 100 seconds, and kept a safe margin assuming 6hz telemetry. But I can experiment to see how much further it can be reliably reduced.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No need. the 30 is better than 100 by far. With the command queueing that makes sense. Thanks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I set it to 12 seconds, then ran it 5 times in a row, and it succeeded every time. So that seems like a good baseline to use.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For this app you said the rate was 6Hz? That tracks for 2 packets wait time I advised on the Issue text then. Glad that was able to come down. thanks for chaecking.

@ddstewar
Copy link
Copy Markdown
Contributor

Note to self: This should be approved once we are able to merge to dev branch. Review complete

@czogby-nasa czogby-nasa linked an issue May 11, 2026 that may be closed by this pull request
1 task
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.

Create EVS checkout test

2 participants