Conversation
|
Thanks, I will take a look later today, can you update the branch? |
83092d5 to
b5449b9
Compare
|
Done! Those changes build and run fine on the simulator and my ssd1306 display. I just bought a ST7789, I'll be able to test with it as soon as it arrives. |
j-g00da
left a comment
There was a problem hiding this comment.
Please run fmt and clippy and then fix clippy warnings.
Also the framebuffer tests are panicking after changes.
I will take a deeper look later.
|
The git log is messy, but I wanted to do everything I wanted functionality wise before requesting review. I'll rebase and re-make the commits later, make other PRs if needed.
I will start to look into proc macros and derive macros, to see what's possible. |
|
When writing documentation, I stumbled upon https://docs.rs/embedded-graphics-core/latest/embedded_graphics_core/draw_target/trait.DrawTarget.html, and by reading the documentation, it is very familiar with what we are doing here, and I wanted to share that with you if you did not know about it! Something about implementing |
| C: PixelColor, | ||
| { | ||
| /// Callback fired after each buffer flush. | ||
| pub flush_callback: Box<dyn FnMut(&mut D)>, |
There was a problem hiding this comment.
Just a note: Removing flush_callback makes this a breaking change
mousefood/src/backend.rs
Outdated
| ) -> EmbeddedBackend<'display, D, C> { | ||
| display: &'display mut M, | ||
| config: EmbeddedBackendConfig, | ||
| ) -> Result<EmbeddedBackend<'display, D, C, M>> { |
There was a problem hiding this comment.
This is also a breaking change which must be documented
| /// | ||
| /// If the display driver requires additional operations, this is the place to make them | ||
| fn flush(&mut self) -> Result<()>; | ||
| } |
There was a problem hiding this comment.
One more thing, we should think about is making the integration crates able to define custom color conversions. E.g. Weact integration has it's own colors which ideally should be moved out of the main crate.
Let me know if you have some ideas for it.
There was a problem hiding this comment.
I think integration crates should be able to define custom color conversions (the Weact case confirms us that).
Is taking anything implementing embedded_graphics::PixelColor enough? Maybe it's time for another trait 😄
There was a problem hiding this comment.
Discovered DrawTargetExt from embedded_graphics, with a function to convert colors, would this interest you? https://docs.rs/embedded-graphics/latest/embedded_graphics/draw_target/trait.DrawTargetExt.html#tymethod.color_converted
There was a problem hiding this comment.
Sorry for late response, I'm not sure if this can be helpful here but you can experiment with this a bit.
But we are calling it directly on underlying display using |
Yup! Just figured that out now, sorry for the confusion (I think it confused myself) |
|
Understandable ;D |
|
For the 2 breaking changes, do you want one commit per breaking change, or having both changes in the same commit is fine? |
|
One is fine |
mousefood/src/error.rs
Outdated
| /// Flushing to the display failed. | ||
| #[error("flushing to DrawTarget failed")] | ||
| Flush, |
There was a problem hiding this comment.
I am debating whether this should be a standalone structure, because currently the only usage of Error::Flush is outside of mousefood, which is a bit weird imo.
I will make a new structure FlushError, export it, and have a From<FlushError> for Error to be able to use our Result type.
There was a problem hiding this comment.
What about doing Flush(String), so one can give more implementation specific details about what exactly went wrong?
There was a problem hiding this comment.
Yes, good idea! I added anything implementing core::error::Error, which includes strings!
There was a problem hiding this comment.
I'll revert to just taking a string, it will be simpler, and it's a good starting point.
f43e0b8 to
81c286b
Compare
81c286b to
f0a412a
Compare
|
Ready for review! The only thing missing is good documentation, if you have any ideas I'm all ears! |
mousefood/src/backend.rs
Outdated
| B: BufferedDisplay<D, C>, | ||
| D: DrawTarget<Color = C> + 'display, | ||
| C: PixelColor + 'display, | ||
| C: PixelColor + 'display + From<TermColor>, |
There was a problem hiding this comment.
nit: lifetimes after traits
| C: PixelColor + 'display + From<TermColor>, | |
| C: PixelColor + From<TermColor> + 'display, |
|
Thanks! Let's move out these two to new PRs and merge these first. |
52c4135 to
b40d7de
Compare
BREAKING CHANGE: removed `flush_callback` from struct `EmbeddedBackendConfig` BREAKING CHANGE: `EmbeddedBackend::init()` returns a Result
b40d7de to
ec01faf
Compare
|
I think we should implement crates for simulator and weact drivers in this PR already. |
|
What do you think about merging this PR onto a staging branch, and make PRs to implement drivers targeting that staging branch? If we implement drivers in this PR I'll become the bus factor, and I do not want to slow you down because of that. |
|
Let's use this branch as staging, just don't rebase anymore so we can safely work on this together. |
|
So I experimented a bit with what has to be done to be able to define custom color conversions without issues. fn convert_color(color: TermColor) -> C;And use it in let mut style_builder = MonoTextStyleBuilder::new()
.font(&self.font_regular)
.text_color(B::convert_color(TermColor(cell.fg, TermColorType::Foreground)))
.background_color(B::convert_color(TermColor(cell.bg, TermColorType::Background)));(this can be simplified, but that's the general idea) Finally we can implement conversions inside integration crates (example for epd-waveshare): impl<SPI, BUSY, DC, RST, DELAY> BufferedDisplay<Display4in2, epd_waveshare::color::Color>
for WaveshareEpd4in2<SPI, BUSY, DC, RST, DELAY>
where
SPI: SpiDevice,
BUSY: InputPin,
DC: OutputPin,
RST: OutputPin,
DELAY: DelayNs,
{
// (...)
fn convert_color(color: TermColor) -> epd_waveshare::color::Color {
// TermColor -> BinaryColor (implemented in mousefood)
let binary_color: BinaryColor = color.into();
// BinaryColor -> Color (from epd_waveshare)
binary_color.into()
}
}Let me know what you think @deadbaed |
|
Okay, let's use this branch. I've locked it through GitHub, so if I did everything well the only way to push code is by making PRs. I think that is super nice to have I am wondering what the code will be for display backends not requiring color correction, like the simulator. |
|
Just |
|
Awesome, just what I thought. I'd love to test your changes on my I did some shopping and bought a |
|
This PR has completely stalled, this summer I tried to get my This PR is also outdated with all the recent changes on main (it's awesome to see mousefood go under ratatui's organization!), and I think there were too many changes required to do what I had in mind in #66, and I lacked knowledge to properly implement this. I would like to take the time to tackle it again when 0.3 will be out, I'd like to know your opinion on #66 if it is worth the time and energy (I still think it is worth it in the long run). I'm happy to discuss about it on discord if you want, maybe let's create a mousefood channel? It might attract new contributors as well. Closing this, thank you for taking the time to reviews PRs! |
|
Yup, I'll take a look at the discussion in that issue whenever I can.
Currently there isn't much discussion happening around Mousefood but it might get more active after we create the new release. I think then we could create a new channel. Currently we could use the Could you maybe drop me a message there? |
This is the main pull request of #66, introducing a trait for display backends to implement to work with mousefood. This will also remove the
flush_callbackfunction, and removes the requirement of using a framebuffer.The
esp32-testwill be removed, it was just for the test. I'll also rebase from main