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

Home use testing: screen refresh alert and offline analysis updates #283

Merged
merged 5 commits into from
Apr 28, 2023

Conversation

tab-cmd
Copy link
Contributor

@tab-cmd tab-cmd commented Apr 27, 2023

Overview

Tested release candidate in a variety of home use scenarios. One issue observed in using a secondary usb-c monitor was if a disconnect occurs or the battery runs out, the primary monitor refresh rate may change to default settings. This can be configured per display arrangement to some degree in Windows; however, to prevent issues in the future and add some awareness of the monitor requirements, I've added an alert for low monitor refresh rates. In the future, we might also consider a consistent frame rate check using PsychoPy to determine monitor stability.

Ticket

https://www.pivotaltracker.com/story/show/184589205

Contributions

  • system_utils.py: update the screen properties method to return the refresh rate, add is_screen_refresh_ok check.
  • validate.py: update validate_bcipy_session to include a check for screen refresh
  • offset_anaysis.py: update to script with documentation and to support future factoring into codebase

Test

  • Make test-all; Added tests for system_util alerts
  • Photodiode testing
  • Configured the monitor with different refresh rates ensuring < 120 would be flagged

Documentation

  • Are documentation updates required? In-line, README, or documentation? Yes

Changelog

  • Is the CHANGELOG.md updated with your detailed changes? Yes!

@tab-cmd tab-cmd requested a review from lawhead April 27, 2023 18:01
@tab-cmd tab-cmd changed the title Home use testing Home use testing: screen refresh alert and offline analysis updates Apr 27, 2023
@tab-cmd tab-cmd requested a review from danielpklee April 28, 2023 01:54
Copy link
Collaborator

@lawhead lawhead left a comment

Choose a reason for hiding this comment

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

Looks like a nice addition.

True if the refresh rate is less than 120 Hz; otherwise False.
"""
if refresh is None:
refresh = get_screen_info()[-1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be more readable if get_screen_info returned a NamedTuple.

from typing import NamedTuple
class ScreenInfo(NamedTuple):
    width: int
    height: int
    rate: float

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't taken to using NamedTuples often, but I like the suggestion- I'll do it before merging!

@tab-cmd tab-cmd merged commit 7fd0107 into 2.0.0rc3 Apr 28, 2023
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.

2 participants