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

TerminalMode #27

Merged

Conversation

therealprof
Copy link
Collaborator

@jamwaffles This is a crude first version of the character mode. I've verified that it works (most surprisingly with the same initialisation code of yours) but it could use a cleanup and get rid of the code duplication and add a little more sophistication to it.

I've tested it with a 128x64 I2C version on my Nucleo-F042 board with this:

        /* Setup I2C1 */
        let mut i2c = I2c::i2c1(p.I2C1, (scl, sda), 400.khz());

        use ssd1306::displayrotation::DisplayRotation;
        let mut disp = Builder::new()
            .with_i2c_addr(0x3c)
            .connect_i2c(i2c)
            .into_char();
        disp.set_rotation(DisplayRotation::Rotate180);
        disp.init().unwrap();
        disp.clear();
        for i in 1..20 {
            let _ = disp.print_chars(b"foooooo");
        }

Just dumping it here because I have to catch a plane and get some feedback whether this approach would work for you.

NB: Rotation acts really funny for 90 and 270 degrees, it basically mirrors the characters and changes from l-t-r to r-t-l mode with character mode.

@jamwaffles jamwaffles mentioned this pull request Mar 29, 2018
@jamwaffles
Copy link
Collaborator

Looks good! Is there anything we can do to dedupe common methods like set_rotation(), reset(), etc? I think this is why I originally went down the impl Drawing for SSD1306 route for egfx.

Also, the big lump of hex for the font is a bit C-ish for my liking. Could you turn it into an include_bytes!() macro from a source image? I used ImageMagick in egfx to get a 1BPP representation of a font image. It would make supplying custom fonts an easier feature to add in the future.

@jamwaffles jamwaffles added the do not merge Incomplete or exploratory work label Mar 29, 2018
@jamwaffles jamwaffles changed the title [Don't merge] Character mode Character mode Mar 29, 2018
@jamwaffles
Copy link
Collaborator

I'm liking this more the more code I read 😄. What do you think about implementing reset(), configure_rotation(), get_dimensions(), set_rotation() (and maybe clear() if we can support it in all modes) on NoMode? It would DRY up the code quite a bit.

Also, can we be more specific with the mode names? I suggest GraphicsMode and CharacterMode; Egfx is a bit obtuse. I'm aiming for a "pit of success" for users 😃.

@therealprof
Copy link
Collaborator Author

Looks good! Is there anything we can do to dedupe common methods like set_rotation(), reset(), etc? I think this is why I originally went down the impl Drawing for SSD1306 route for egfx.

Yeah, needs work. I was thinking of adding another type, similar to what we're doing with the GPIO HAL implementations which allow switching modes on-the-fly (without requiring impls inside each mode) and also common functions shared between all implementations. I just wanted to see stuff working before making it super-fancy since that has the tendency to cause the whole thing fall flat on its face with Rust.

Also, the big lump of hex for the font is a bit C-ish for my liking. Could you turn it into an include_bytes!() macro from a source image? I used ImageMagick in egfx to get a 1BPP representation of a font image. It would make supplying custom fonts an easier feature to add in the future.

I must admit I kinda like the C-ish thing. Also the content MUST match the mapping code, i.e. you cannot simply swap out the font, we might be able to do some magic with one file per glyph but I'm not sure we want to go that way.

I'm liking this more the more code I read 😄. What do you think about implementing reset(), configure_rotation(), get_dimensions(), set_rotation() (and maybe clear() if we can support it in all modes) on NoMode? It would DRY up the code quite a bit.

The problem with that is that we could only change the rotation and get the dimensions before changing mode which we certainly don't want?

Also, can we be more specific with the mode names? I suggest GraphicsMode and CharacterMode; Egfx is a bit obtuse. I'm aiming for a "pit of success" for users 😃.

Well, I used Egfx for "embedded graphics" and I think we might have more "graphics" modes in the future which might require a different setup and not necessarily a full buffer, e.g. a scanline renderer might be nice. But hey, I'm not too attached to those names. 😉

@therealprof
Copy link
Collaborator Author

Before I forget: have you tried it?

@jamwaffles
Copy link
Collaborator

Before I forget: have you tried it?

Just had a play with it now, I see what you mean about the rotation screwing everything up. I'll fix that asap.

I just wanted to see stuff working before making it super-fancy since that has the tendency to cause the whole thing fall flat on its face with Rust.

Fair 😂. Are you planning on doing some dedupe in this PR?

I must admit I kinda like the C-ish thing. Also the content MUST match the mapping code, i.e. you cannot simply swap out the font, we might be able to do some magic with one file per glyph but I'm not sure we want to go that way.

Alright, let's leave it for now :). I'd like to experiment with a 6x8 (wxh) font in character mode in the future which doesn't look too hard from what I've seen in your code, just send 6 bytes instead of 8. That'll be the time to figure out a nice way of using different fonts. I guess one could always just use egfx for fancier font rendering.

The problem with that is that we could only change the rotation and get the dimensions before changing mode which we certainly don't want?

Good point, ignore me.

Well, I used Egfx for "embedded graphics" and I think we might have more "graphics" modes in the future which might require a different setup and not necessarily a full buffer, e.g. a scanline renderer might be nice.

Can you think of other modes that would clash with the (admittedly generic) graphics name? In your specific example, you'd just call into_scanline() which is a clear name I think. I'm thinking of newcomers and Arduino converts that want to "just draw some stuff" which usually comes from the word "graphics", so it fits nicely with assumptions as a sort of default goto for the driver.

@jamwaffles
Copy link
Collaborator

Just been having a look at the rotation stuff and it seems fine for 0 and 180 degree rotations so I think #28 might be a red herring. Can you confirm? It is however broken for 90 and 270 degree rotations, but that's an artifact of the chip I think; the page address pointer moves vertically when set up right, but the 8 bit blocks are now pointing horizontally so the text turns into noise. You'll have to do some bit twizzling to fix that I think which is a shame.

@therealprof
Copy link
Collaborator Author

Fair 😂. Are you planning on doing some dedupe in this PR?

I was hoping to do it step by step. First change the structure to allow for modes, (i.e. the other PR one we agree on a naming), then introduce the character mode, then do the deduplication and then have another go at simplifying the mode changes.

Alright, let's leave it for now :). I'd like to experiment with a 6x8 (wxh) font in character mode in the future which doesn't look too hard from what I've seen in your code, just send 6 bytes instead of 8.

Problem is you really can only update 8 bits at a time on an 8 bits boundary, so for 6x8 you'd really only change the spacing (i.e. 2 horizontally and 0 vertically) but not the number of characters per line. It should be possible to use any arbitrary (but constant) height but I haven't tried that yet.

That'll be the time to figure out a nice way of using different fonts.

Yeah, ultimately I'd like to have pluggable (i.e. user providable) fonts. With that it would also be possible to do e.g. a rogue like game with character based pseudo graphics.

I guess one could always just use egfx for fancier font rendering.

Yeah, ultimately you definitely would want to allow font rendering via egfx, too.

Can you think of other modes that would clash with the (admittedly generic) graphics name?

A scanline approach is a graphics approach as well, the only difference being that the graphics is rendered line by line while egfx represents a framebuffer. I think we somehow should take care of the (optional) external dependency or better yet (from my POV) make it a hard dependency. IMHO crates should do useful stuff out-of-the-box without requiring unwieldy features switches so if we decide against providing a useful simplified pixel twiddling graphics interface (as previously suggested) then embedded-graphics should be all in.

@jamwaffles
Copy link
Collaborator

Hmm, that font stuff is a bit frustrating. Charmode should be pretty simple right, so we can just leave the more complex font/character rendering to embedded_graphics. I like the idea of using custom 8x8 fonts for tiled graphics, but perhaps work for a future PR? :)

IMHO crates should do useful stuff out-of-the-box without requiring unwieldy features switches so if we decide against providing a useful simplified pixel twiddling graphics interface (as previously suggested) then embedded-graphics should be all in.

#31 😄. I can see that it's confusing for consumers of the driver to have to enable a feature that is a big part of the feature set. Also #26 (comment). Embedded graphics should take care of pixel twiddling (with as-yet unwritten features) I think.

@therealprof
Copy link
Collaborator Author

Hmm, that font stuff is a bit frustrating. Charmode should be pretty simple right, so we can just leave the more complex font/character rendering to embedded_graphics. I like the idea of using custom 8x8 fonts for tiled graphics, but perhaps work for a future PR? :)

Yes, yes and yes. ;)

Character mode can be used in a number of ways:

  1. Streaming text like a console
  2. Positioned text
  3. Tiled graphics

At the moment 1) and 2) are highest priority for me and currently supported with the built-in font but there's certainly room for further extension in there, especially with swappable fonts. Potentially we can also enable other tile sizes but I need to look into that; it would certainly be nice to also allow for a scaled mode/larger fonts.

@therealprof therealprof force-pushed the features/charmode branch 5 times, most recently from 2f69575 to 1478508 Compare April 4, 2018 22:11
@therealprof
Copy link
Collaborator Author

@jamwaffles Rebased existing code again to incorporate DisplayModeTrait and added example. Now I can finally focus on getting this cleaned up for inclusion. 😉

@jamwaffles
Copy link
Collaborator

jamwaffles commented Apr 5, 2018 via email

@therealprof therealprof force-pushed the features/charmode branch 2 times, most recently from 5c5b80b to 1d73dab Compare April 7, 2018 21:47
This was referenced Apr 8, 2018
Signed-off-by: Daniel Egger <daniel@eggers-club.de>
Signed-off-by: Daniel Egger <daniel@eggers-club.de>
Signed-off-by: Daniel Egger <daniel@eggers-club.de>
Signed-off-by: Daniel Egger <daniel@eggers-club.de>
Signed-off-by: Daniel Egger <daniel@eggers-club.de>
Signed-off-by: Daniel Egger <daniel@eggers-club.de>
…nt it

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

@jamwaffles So I've been playing around with the IMHO only remaining outstanding item to get this in: custom font support. So I've been creating a trait for that but I'm not too happy with the results.

Base line size for an example based on the current state of this branch:

# arm-none-eabi-size target/thumbv6m-none-eabi/release/examples/i2c_hal_ssd1306alphabeter*
   text	   data	    bss	    dec	    hex	filename
   3580	      0	      4	   3584	    e00	target/thumbv6m-none-eabi/release/examples/i2c_hal_ssd1306alphabeter

After moving the very same code to it's own impl of a new trait (used unconditionally):

# arm-none-eabi-size target/thumbv6m-none-eabi/release/examples/i2c_hal_ssd1306alphabeter*
   text	   data	    bss	    dec	    hex	filename
   3600	      0	      4	   3604	    e14	target/thumbv6m-none-eabi/release/examples/i2c_hal_ssd1306alphabeter

But as soon as I make the const array a bit more rusty using a match (seems like a similar problem to the command bloat):

# arm-none-eabi-size target/thumbv6m-none-eabi/release/examples/i2c_hal_ssd1306alphabeter*
   text	   data	    bss	    dec	    hex	filename
   4136	      0	      4	   4140	   102c	target/thumbv6m-none-eabi/release/examples/i2c_hal_ssd1306alphabeter

And it only gets worse from there. 🙁

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

@jamwaffles So I spent two evening figuring out to make things work like I would have them. I hit a number of snags and no way to work around them (like not being able to set the draw position without affecting the display area and vice versa). So I'd like to rename this mode true to it's original purpose: TerminalMode and keep it at that.

A tile based CharacterMode is certainly possible but it will require to manually position each tile before drawing it so it'll require a different interface (or plenty of voodoo to record the current GDRAM position and weave in the right commands to modify the position as required).

Thoughts?

@jamwaffles
Copy link
Collaborator

TerminalMode sounds good. I was envisaging a "spew characters out to the screen" mode for this anyway; moving the draw position feels like feature creep but would be useful to have in the future. GraphicsMode can already print positioned chars, so let's keep the ball rolling on this PR and keep the scope simple :).

I've started writing the announce blog post (draft!) for this driver and it would be great to demonstrate multiple modes; right now telling people to jump through the into() hoop when there's only one mode feels strange :). Getting a solid TerminalMode integrated for release would be great.

@therealprof
Copy link
Collaborator Author

GraphicsMode can already print positioned chars, so let's keep the ball rolling on this PR and keep the scope simple :).

GraphicsMode is too expensive for some usecases due to full framebuffer and full refreshes.

Getting a solid TerminalMode integrated for release would be great.

I'll rename it later today and finish it up.

@jamwaffles
Copy link
Collaborator

GraphicsMode is too expensive for some usecases due to full framebuffer and full refreshes.

Very true!

I'll rename it later today and finish it up.

Awesome!

Signed-off-by: Daniel Egger <daniel@eggers-club.de>
@therealprof therealprof changed the title Character mode TerminalMode Apr 12, 2018
@therealprof therealprof removed the do not merge Incomplete or exploratory work label Apr 12, 2018
@therealprof
Copy link
Collaborator Author

@jamwaffles Okay, this is it. The examples also contain my speed test but I'd be happy to provide a good ol' Hello World for your post, too.

@therealprof
Copy link
Collaborator Author

I considered reverting the CharacterBitmap back to the previous implementation but that didn't improve code size and maybe we can use that in the future to provide swappable or custom fonts so I left it in.

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.

Couple of nits and we're good to go. I'd really like to add output scrolling to this mode in the future. Maybe a separate ScrollingTerminalMode? I think the driver chip can do scrolling itself so I don't imagine it'd be that hard to set up.

@@ -0,0 +1,58 @@
//! Endlessly fill the screen with characters from the alphabet
//!
//! Run on a Blue Pill with `xargo run --example character_i2c
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use cargo now! :D

DI: DisplayInterface,
{
/// Clear the display buffer. You need to call `disp.flush()` for any effect on the screen
pub fn clear(&mut self) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should return a Result<(), ()> from this method to be consistent with the rest of the API.

@therealprof
Copy link
Collaborator Author

The scrolling would be easy. But the usual problem applies: We have no idea when to do it. We'd need to keep track of where we are (and reset when the screen is cleared) to know when to wrap. Same applies to newline handling, if we wanted to do a line-break when someone sends a \n, we'd know to which column we're at and fill the rest of the line with blanks.

The real annoying thing about this display is that there's no information readout or advanced functionality that would allow us to operate statelessly. 🙁

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

Adressed the nits, but the clear() change will require a small change in the blog post, too. ;)

@jamwaffles
Copy link
Collaborator

Hmm. I had it in my head that scrolling would be an easy thing to do. Something to investigate :). Thanks for fixing the nits. I'll take care of the blog post.

@jamwaffles jamwaffles merged commit 3e85506 into rust-embedded-community:master Apr 13, 2018
@therealprof therealprof deleted the features/charmode branch April 13, 2018 08:22
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.

None yet

2 participants