Skip to content

Conversation

@therealprof
Copy link
Collaborator

Signed-off-by: Daniel Egger daniel@eggers-club.de

Signed-off-by: Daniel Egger <daniel@eggers-club.de>
@therealprof
Copy link
Collaborator Author

Moving some common code from graphics mode over to properties.rs for deduplication purposes and to clean up the mode. The whole chunk of init_column_mode() should really be broken up into more sizeable and reusable pieces so we don't need to have another chunk for a slight variation of this. Also the rest of the Command should be moved out into nice functions and send_data() on the borrowed interface be properly encapsulated in properties.rs.

Copy link
Collaborator

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

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

Cleanup ❤. Just a couple of doc comments and we're good to go.

Small nit: I'm starting to think properties isn't the right name for this stuff. I'm absolutely fine not bikeshedding a variable name, but do you have a better one in mind we could use?

/// Initialize display in column mode
pub fn init_column_mode(&mut self) -> Result<(), DI::Error> {
// TODO: Break up into nice bits so display modes can pick whathever they need
// Display is set up in column mode, i.e. a byte walks down a column of 8 pixels from
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you move this to the function docblock? Would be helpful to have in the rendered docs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, that's why I was asking for naming ideas before. DisplaySetup would work, DisplayConfiguration also.

}
}

// Display is set up in column mode, i.e. a byte walks down a column of 8 pixels from column 0 on the left, to column _n_ on the right
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be useful to have in the rendered docs too.

}

/// Reset display
// TODO: Move to a more appropriate place
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, yes this is a strange one. It's only applicable to the SPI interface which makes placing it in the architecture of this crate pretty weird. Do you have any thoughts where this could go?

Signed-off-by: Daniel Egger <daniel@eggers-club.de>
@jamwaffles
Copy link
Collaborator

Thanks for the doc changes. I can't think of a good name now so let's just get this change merged :)

@jamwaffles jamwaffles merged commit aa79ac2 into rust-embedded-community:master Apr 2, 2018
@therealprof therealprof deleted the features/morededup branch April 4, 2018 06:49
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