Terminal mode cursor

[mjadczak]

This supersedes #79 by tracking the full vertical and horizontal position ourselves within TerminalMode, and implementing line-wrapping manually. This means we can use the Page addressing mode, so we can write characters to arbitrary points on the screen, while still remaining bufferless. As a bonus, we can handle \r properly.

There may be places that this could be cleaned up—I especially don't like the potential confusion between "pages" and "rows" and the magic * 8 which needs to be in place (it probably needs a few wrapper types). It does work though—tested manually over I2C on a cheap AliExpress SSD1306 board.

Closes #78

[therealprof]

Very nice. For some reason \r does not work for me and unfortunately this adds substantial bloat to the binary size...

[mjadczak]

Strange—does set_position itself work for you?
I haven't looked at how this affects the binary size, I don't know why it would do so massively—if anything, there is a little more runtime work but I wouldn't expect a ton more code to be generated. 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]

As mentioned in #79 I've tested this PR and it generally works (including '\r' and '\n', not sure what the problem with my previous tests were). If we can reduce the size a bit (again, see #79 for a possible change) I'd be very happy to take it.

[mjadczak]

I've pushed two more commits—the first one removes the extra enum, and gets rid of the trait entirely (since it was public, this could technically mean we'd need to version this as 0.3, unlikely as it is that anyone is using it) and the second one is a small change which could be an optimisation or not—basically recursing to the "print a character" case for inserting spaces when printing a newline.

Then I realised that actually, a newline probably should not clear the rest of the line, so I made a third commit which makes it behave like an actual terminal in that respect. The only issue there is that because we need the result of the logical cursor change in order to know what to send to the device, there could be a case where the logical cursor is changed but the device cursor is not, resulting in inconsistent state which is what I tried to avoid elsewhere by placing the infallible logical cursor change after the physical data is sent. If you're ok with this, perhaps we could make it a documented property that after any Err result you have to reset the mode before using it again.

[therealprof]

Hm, I tried this and either I stumbled right into your described problem or something else is off in the calculations now, i.e. it worked fine in the previous version: If I output a string with a newline character at the end, the linebreak position ends up in some other horizontal position on the screen so when I output some longer runs of text it'll make a diagonal jump somewhere where it's not supposed to do that.

[mjadczak]

Sorry, I banged that code out and had not tested it. Should get some time to try to reproduce tomorrow—it's probably some logic bug I introduced.

[mjadczak]

So I'm not sure I understand your bug - here are the results I'm getting:

disp.write_str("123\n456\nfoo\rbar");

gives me

123
456
bar

as expected.

disp.write_str("This string is clearly far too long to fit on one line\n456\nfoo\rbar");

gives me

This string is c
learly far too l
ong to fit on on
e line
456
bar

which again seems correct.

Can you tell me what the string you tried was, and how it misbehaved?

[therealprof]

I don't have a fixed sequence. I wrote a small application which takes serial input and renders it on the screen. Since my terminal doesn't seem to produce useful character sequences for \n and \r I mapped a to yield return\n. Then I'm just (slowly) typing in characters.
a...a... for instance just produced:

return
...return
.
 ..

[therealprof]

@mjadczak Okay, I just tried it running it directly with a fixed string and not surprisingly this does the trick as well:

        let mut disp: TerminalMode<_> = Builder::new().with_i2c_addr(0x3c).connect_i2c(i2c).into();
        let _ = disp.init();
        let _ = disp.clear();
        let _ = disp.write_str("return\n...return\n...");

[therealprof]

diff --git a/src/mode/terminal.rs b/src/mode/terminal.rs
index 9bc07b0..6d4b9fa 100644
--- a/src/mode/terminal.rs
+++ b/src/mode/terminal.rs
@@ -169,6 +169,7 @@ where
                 let CursorWrapEvent(new_line) = self.ensure_cursor()?.advance_line();
                 self.properties.set_column(0)?;
                 self.properties.set_row(new_line * 8)?;
+                self.ensure_cursor()?.set_position(0, new_line);
             }
             '\r' => {
                 self.properties.set_column(0)?;

fixes it for me; not quite sure why. The whole calculation seems a bit convoluted to me and could use some simplification...

[mjadczak]

Ah yes, you're right. What's happening here is that when you go to a new line, the physical cursor is set to the beginning of the new line, but in the logical cursor, only the row is changed, and the current column is not. So, when we have written enough characters such that we would have wrapped (had we not explicitly gone to a new line), we end up just moving down a row—but the assumption there is that we have automatically wrapped around to the first column on the physical screen, and so we don't explicitly tell it to do this, ending up with the other two dots just floating on the line below.

The actual fix then is the one I've just pushed, to actually remember in the logical cursor that when we advance to the next line, we go to the first column.

I agree that the synchronisation between the logical and physical cursor is a bit convoluted, but I'm not sure how to make it better. Suggestions?

[therealprof]

I agree that the synchronisation between the logical and physical cursor is a bit convoluted, but I'm not sure how to make it better. Suggestions?

Not at the moment but I think we can leave improvements to another PR. ;)

[therealprof]

LGTM, thanks.

[therealprof]

@jamwaffles #[deny(warnings)] at its best here. 😅

[jamwaffles]

I had to disable #[deny(warnings)] in embedded_graphics for this reason. I'm not opposed to doing it here, although it'll produce noise until the embedded-hal v2 traits migration path is sorted out which will probably bug some users.

[therealprof]

@jamwaffles I agree that the embedded-hal warnings are a huge annoyance but it's fully independent of what we do here.

[jamwaffles]

I haven't checked this repo in a while. I noticed this PR was approved, but there are some conflicts and the CI is failing. The fallible v2 traits in #83 should mean that the deny_warnings can get switched back on.

@mjadczak can I ask you to rebase this PR and fix anything required in the CI? Then this PR can get pushed over the line if people are still happy with it 👌

[mjadczak]

I've rebased this locally but it seems I need to do a bit of work to understand the new way of handling errors, as I have a few functions where I have to materialise an Err value, instead of just passing it up from the communication layer (and DI::Error is a type with no further bounds).

[jamwaffles]

I have a few functions where I have to materialise an Err value, instead of just passing it up from the communication layer

You could either map the comms layer error to (), make a new enum or panic!() if the correct mode isn't set. There are tradeoffs to all solutions. I think I'd choose panic!() with a message explaining the mode isn't set right but you or others might have a better proposal.

I've rebased this locally

Can you push your rebase (even if it fails to compile)? I was investigating your branch and had to rebase it myself. It would be good to save this duplicated effort :)

[mjadczak]

I've pushed my non-compiling rebase, and I'm taking a look now. I think the biggest issue I had was that DisplayInterface does not specify what its errors can be:

/// A method of communicating with SSD1306
pub trait DisplayInterface {
    /// Interface error type
    type Error;
    /// Send a batch of up to 8 commands to display.
    fn send_commands(&mut self, cmd: &[u8]) -> Result<(), Self::Error>;
    /// Send data to display.
    fn send_data(&mut self, buf: &[u8]) -> Result<(), Self::Error>;
}

So when I'm writing code which is generic over any particular DisplayInterface, there is no way for me to synthesise a DisplayInterface::Error - it's not just a question of adding a new error to the enum.
One way around this is to just panic!() as you suggested, or else for the functions in DisplayInterface to actually return a Result<(), DisplayError<Self::Error>> where DisplayError<T> is some new trait which contains both "protocol errors" which are invariant of the interface (which I want to return) as well as errors of type T which would be interface-specific.
Perhaps panicking is ok in this case though, as I think that the errors would indicate programmer error rather than some unpredictable error condition with the device.

(also note that I wrote the above before revisiting the code thoroughly, so I may be misremembering / missing something)

[mjadczak]

I've just pushed changes which now build on my machine - I have made the lower-level operations inside Properties simply panic if used in an incorrect mode, but I have introduced a new error enum for the TerminalMode itself to represent the other cases I was using Err(()) in. I haven't tested that this code works just as well as before yet.

EDIT: I noticed that the tests are failing because one of the examples unwraps the result of initialising the TerminalMode, but the new error enum does not implement Debug. Unfortunately this cannot be derived as DisplayInterface::Error is not bound to implement it (and this continues further down).
Should we 1) require all of the relevant errors to implement Error (therefore getting Display and Debug) - is this not done in embedded libraries for a reason? - or 2) just remove the unwrap call in the example?

[jamwaffles]

Sorry it's taken me a while to get back to this. Can you change the error type back to just ()? It's nice to be able to differentiate between logic errors and hardware errors, but I can't seem to get the right bounds in the right places to make this work. I can't find the Error trait in core either, so that won't work in embedded contexts. It should certainly be possible to .unwrap() in user code - the only other solution would be to panic.

[mjadczak]

If we're ok with hiding the exact underlying error from the user (by just using ()), we can make the result unwrap-able by implementing Debug ourselves and simply reporting the InterfaceError as "an error has occurred in the underlying interface" in the message (as far as I can tell, both interfaces use () for this error type anyway, so that's all the user will be able to find out in any case).

[jamwaffles]

I'm fine with that approach personally.

@therealprof do you have any comments/objections? Otherwise I can merge this PR once the branch is rebased and the build passes.

[therealprof]

Works for me. I'm more concerned with the bloat than with error handling to be honest. And panics, especially with custom messages, do add a lot of that. Since we do have control over the implementations (and I don't think there will be custom implementations of the used traits in this crate) it would make sense to me just remove the associated Error type and just provide an enum with error causes. That would make handling much easier but of course would be a breaking change.

[jamwaffles]

just remove the associated Error type and just provide an enum with error causes ... would be a breaking change

0.3.0 will be breaking due to changes in Embedded Graphics anyway, so we can change this interface for that release. I like the idea of removing the associated type - it will simplify the code quite a bit. Let's explore that in a separate PR so this one can get merged.

[jamwaffles]

Hey @mjadczak, can you rebase this branch and fix the build when you have a moment? It would be great to get this PR merged!

[mjadczak]

Sure! I'll try to take a look this week.

Has master now changed to a single error enum? FWIW I have been using a variant of this where I bound DisplayInterface::Error to implement core::fmt (which in practice both variants do) and it's been working well.

[mjadczak]

I've rebased on master - for now with the explicit impl of Debug for the TerminalModeErrror (in order for the error to be unwrapable)

[jamwaffles]

Thanks! I'll test this on a physical device as soon as I can.

[jamwaffles]

Tested on a real device and it works great. I think the doc comments could use a bit of cleanup, but that can happen in another PR for the whole crate. Thanks for your time and hard work!

[jamwaffles]

Released in 0.3.0-alpha.2. Thanks!