Implement newline handling for TerminalMode

[mjadczak]

What it says on the tin. See #78.

The reason the counter is an Option is to not rely on it before we initialise the mode and know that we are in the top left corner—I'm not wedded to this way of enforcing that, however.

[mjadczak]

This has been tested manually using a cheap ssd1306 chip from AliExpress over I2C.

[therealprof]

Interesting implementation. I don't have time to check it in depth right now but on a first glance this looks good, albeit a bit complicated. I take it there's no easy way to handle the newline character before trying to convert it to a bitmap?

[mjadczak]

The reason we can't handle the newline character more easily than introducing this new BitmapCharacter enum is because print_char does not take a char, it takes a T where TerminalMode<DI>: CharacterBitmap<T>, and so we can't just test that against \n, we need to add capability to CharacterBitmap to tell us that the character is a newline. (Incidentally, why is this done this way? So that the user can provide their own font? Would that require a newtype over char in order to avoid aliasing the trait impl?)

[jamwaffles]

@therealprof Do you have a moment to review this? TerminalMode is your area of expertise 🙂

[therealprof]

@jamwaffles I've reviewed #80 and provided some feedback. The gist is this: It works great but adds substantial overhead. I've written a little test application and captured the cargo bloat output on a STM32F429:

Before the change:

Bloat for example i2c_hal_ssd1306terminal
Compiling ...
Analyzing target/thumbv7em-none-eabihf/release/examples/i2c_hal_ssd1306terminal

File  .text   Size          Crate Name
0.0%   0.0%     0B                [0 Others]
0.7%  57.0% 2.1KiB      [Unknown] main
0.2%  14.7%   550B        ssd1306 ssd1306::command::Command::send
0.1%   9.7%   362B        ssd1306 <ssd1306::properties::DisplayProperties<DI>>::init_column_mode
0.0%   3.0%   112B    cortex_m_rt Reset
0.0%   2.7%   102B stm32f4xx_hal? <stm32f4xx_hal::i2c::I2c<I2C, PINS> as embedded_hal::blocking::i2c::Write>::write
0.0%   2.6%    96B        ssd1306 <ssd1306::properties::DisplayProperties<DI>>::draw
0.0%   2.5%    94B      [Unknown] __aeabi_memcpy
0.0%   2.5%    94B        ssd1306 <ssd1306::properties::DisplayProperties<DI>>::set_rotation
0.0%   2.2%    84B        ssd1306 <ssd1306::properties::DisplayProperties<DI>>::set_draw_area
0.0%   1.7%    64B            std <T as core::convert::Into<U>>::into
0.0%   0.5%    20B            std core::slice::<impl [T]>::copy_from_slice
0.0%   0.2%     6B            std core::result::unwrap_failed
0.0%   0.2%     6B            std core::panicking::panic
0.0%   0.2%     6B            std core::panicking::panic_fmt
0.0%   0.2%     6B    cortex_m_rt ResetTrampoline
0.0%   0.1%     2B    cortex_m_rt HardFault_
0.0%   0.1%     2B     panic_halt rust_begin_unwind
0.0%   0.1%     2B    cortex_m_rt DefaultPreInit
0.0%   0.1%     2B    cortex_m_rt DefaultHandler_
1.2% 100.0% 3.7KiB                .text section size, the file size is 296.5KiB

After the change:

Bloat for example i2c_hal_ssd1306terminal
Compiling ...
Analyzing target/thumbv7em-none-eabihf/release/examples/i2c_hal_ssd1306terminal

File  .text   Size          Crate Name
0.0%   0.0%     0B                [0 Others]
1.0%  69.5% 3.2KiB      [Unknown] main
0.2%  12.1%   572B        ssd1306 ssd1306::command::Command::send
0.0%   2.4%   112B    cortex_m_rt Reset
0.0%   2.1%   102B stm32f4xx_hal? <stm32f4xx_hal::i2c::I2c<I2C, PINS> as embedded_hal::blocking::i2c::Write>::write
0.0%   2.0%    94B      [Unknown] __aeabi_memcpy
0.0%   2.0%    94B        ssd1306 <ssd1306::properties::DisplayProperties<DI>>::draw
0.0%   2.0%    94B        ssd1306 <ssd1306::properties::DisplayProperties<DI>>::set_rotation
0.0%   1.9%    90B        ssd1306 <ssd1306::mode::terminal::TerminalMode<DI>>::advance_cursor
0.0%   1.7%    82B        ssd1306 <ssd1306::mode::terminal::TerminalMode<DI>>::reset_pos
0.0%   1.3%    64B            std <T as core::convert::Into<U>>::into
0.0%   1.0%    46B        ssd1306 <ssd1306::properties::DisplayProperties<DI>>::set_row
0.0%   1.0%    46B        ssd1306 <ssd1306::properties::DisplayProperties<DI>>::get_dimensions
0.0%   0.4%    20B            std core::slice::<impl [T]>::copy_from_slice
0.0%   0.1%     6B            std core::result::unwrap_failed
0.0%   0.1%     6B            std core::panicking::panic
0.0%   0.1%     6B            std core::panicking::panic_fmt
0.0%   0.1%     6B    cortex_m_rt ResetTrampoline
0.0%   0.0%     2B    cortex_m_rt HardFault_
0.0%   0.0%     2B     panic_halt rust_begin_unwind
0.0%   0.0%     2B    cortex_m_rt DefaultPreInit
0.0%   0.0%     2B    cortex_m_rt DefaultHandler_
1.5% 100.0% 4.6KiB                .text section size, the file size is 316.5KiB

It may not matter much in this particular case but it certainly hurts on a Cortex-M0...

Other than this it's a really great change.

[mjadczak]

TIL about cargo bloat, that seems really useful. If code size is an issue, what about my suggestion in #80?

If this is an issue perhaps this should be a separate mode, like CharacterMode, so that it can be compiled away if not used?

[therealprof]

@mjadczak I did a small experiment and got rid of the BitmapCharacter enum which immediately reduced the overhead to ~400B which is still not perfect but acceptable in my POV.

[therealprof]

@mjadczak You can find my experiment here: https://github.com/therealprof/ssd1306/blob/terminal-mode-cursor/src/mode/terminal.rs

[mjadczak]

Does that actually compile with no warnings? The key function now looks like this:

pub fn print_char<T>(&mut self, c: char) -> Result<(), ()>
    where
TerminalMode<DI>: CharacterBitmap<T>,

i.e. there is no more need for the T type, instead the char specialisation of the trait is selected in the self.properties.draw(&Self::to_bitmap(c))? call. This kind of obviates the need for making this a trait anyway, since implementing it does not give you anything.

Incidentally, I was going to do it this way initially, before I realised that the trait was there—I did raise this above:

Incidentally, why is this done this way? So that the user can provide their own font? Would that require a newtype over char in order to avoid aliasing the trait impl?

I still don't really get the abstraction provided by the trait, but assumed it was important when writing the code.

So, I think that either

• The trait is not required, we only support one font and therefore could get rid of the trait (just make it a function which takes char directly)
• We do want the trait, in which case your solution won't work if someone wants to use a different type which implements it.

[mjadczak]

Also, I should probably close in favour of #80 and continue discussion there.

[therealprof]

I don't remember the history of this but yes, the idea was to support custom fonts at some point.