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

add Display::is_sleeping fixes #77 #78

Merged
merged 2 commits into from
Oct 23, 2023
Merged

add Display::is_sleeping fixes #77 #78

merged 2 commits into from
Oct 23, 2023

Conversation

almindor
Copy link
Owner

@almindor almindor commented Oct 6, 2023

Adds a preliminary is_sleeping method. I'm not too happy about the state machine logic here though. I think we can merge this for the next version but I want to refactor things so that we can have a workable state machine for things like this that can properly tie into the Model inits etc.

@almindor
Copy link
Owner Author

almindor commented Oct 6, 2023

@rfuest what do you think? I think we'll need to add some sort of state machine that can work well accross model and display itself.

In this case specifically I'm unhappy about how the sleeping state is not properly tied to init.

@rfuest
Copy link
Collaborator

rfuest commented Oct 7, 2023

In this case specifically I'm unhappy about how the sleeping state is not properly tied to init.

I'm not sure I understand what you mean. Would you prefer that the initial state can be set by Model::init instead of Display::init? For the sleeping state I don't think this is necessary, because Model::init is required to initialize the display into a non sleeping state. I also can't think of any other state that shouldn't be initialized to the same value by all Model::init implementations. Do you have a use case in mind?

mipidsi/src/lib.rs Outdated Show resolved Hide resolved
@almindor
Copy link
Owner Author

@rfuest I'm happy to merge this if you think it's good enough this way.

Copy link
Collaborator

@rfuest rfuest left a comment

Choose a reason for hiding this comment

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

LGTM

@almindor almindor merged commit 9fbaca7 into master Oct 23, 2023
6 checks passed
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