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

Added row mask methods #16

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

Added row mask methods #16

wants to merge 13 commits into from

Conversation

nilscript
Copy link

No description provided.

Copy link
Owner

@jasonpeacock jasonpeacock left a comment

Choose a reason for hiding this comment

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

Overall this looks good, thank you! One typo (spelling) in a comment to fix.

I'm still a beginner in Rust, and a previous contributor made changes to ensure this library works with #![no_std], will your changes still work with #![no_std]?

src/types/display_data.rs Outdated Show resolved Hide resolved
src/types/display_data_address.rs Outdated Show resolved Hide resolved
src/types/led_location.rs Show resolved Hide resolved
src/types/display_data.rs Outdated Show resolved Hide resolved
@nilscript
Copy link
Author

I am positive that these changes are ![no_std] compatible. Library num_enum says it's no_std. I can't however guarantee that they are as I don't know how to build for a no_std environment.

@nilscript nilscript marked this pull request as draft November 12, 2020 10:47
/// Represents the desired values of the device, may not match
/// the current values if it has not been written recently.
/// Remember to use `.write_display_buffer()` after changing buffer.
pub buffer: [u8; ROWS_SIZE],
Copy link
Author

Choose a reason for hiding this comment

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

Rather than using custom a type, a plain byte is more flexible. It would make sense to restrict users from using u8 if there where invalid states a buffer could be in, which there are none.

Copy link
Author

Choose a reason for hiding this comment

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

Also marking buffer as public means that its way more easier for people to get the buffer in the state consumers want.
Any bit state in the buffer is a valid state, so it's no harm in marking buffer as public.

/// # Ok(())
/// # }
/// ```
pub fn update_row_mask(&mut self, row: usize, mask: u8) {
Copy link
Author

Choose a reason for hiding this comment

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

This function was written before the buffer was marked public. Should it be removed? Should the buffer remain private?

for row in self.buffer.iter_mut() {
*row = DisplayData::COMMON_NONE;
}
self.buffer = [0; ROWS_SIZE];
Copy link
Author

Choose a reason for hiding this comment

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

Trust the compiler. No need for iteration.

)?;

Ok(())
)
}
Copy link
Author

Choose a reason for hiding this comment

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

Removing the endings of similar functions is just a preference really, but makes no difference.

@@ -27,6 +30,55 @@ bitflags! {
}
Copy link
Author

Choose a reason for hiding this comment

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

I dont think it's a good idea to have bitflags when each flag marks a single bit AND all bits can be used to make any byte.
I think it's rather pointless, but it could stay for backwards compatibility.

const ROW_14 = 14;
/// Row 15
const ROW_15 = 15;
use DisplayDataAddress::*;
Copy link
Author

Choose a reason for hiding this comment

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

Changing from bitflags into enum as it makes more sense. Addresses should not need bitwise operations.

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