Type-level display size

[bugadani]

Hi! Thank you for helping out with SSD1306 development! Please:

• Check that you've added documentation to any new methods
• Rebase from master if you're not already up to date
• Add or modify an example if there are changes to the public API
• Add a CHANGELOG.md entry in the Unreleased section under the appropriate heading (Added, Fixed, Changed, etc)
• Run rustfmt on the project with cargo fmt --all - CI will not pass without this step
• Check that your branch is up to date with master and that CI is passing once the PR is opened

PR description

This is just a rough draft of how I imagine type-level display sizes. This has several added benefits:

• allows the user to specify display sizes
• can be extended via GenericArray to support flexible display buffer size accidentally implemented
• hopefully allows generating much nicer machine code

The downside is the amount of code changed and the API it broke...

Closes #20
Closes #122

[bugadani]

I wish there was a way to skip CI on drafts... this really is a waste of energy at this point

[bugadani]

Haven't measured the perf difference yet, but setting no_inline and inspecting the generated assembly, at least there are no integer multiplications generated and only a single jump table instead of two (I guess display rotation). Which is nice IMO :)

[jamwaffles]

This looks like a really good start while we wait for const generics to bake. I left some comments about the use of type vs const.

Which is nice IMO :)

That is nice!

The downside is the amount of code changed and the API it broke...

Don't worry about it. There are a bunch of other breaking changes for the next release anyway.

[bugadani]

Indeed, the associated constants are nice. I guess I went trigger-happy with the typenum crate

[bugadani]

Now that's pedantic, failing build because of an extra empty line 😂

[bugadani]

Currently, changing the rotation is only possible via a complete reconfiguration. I'm not sure if I want to reintroduce it, though, since the type would change and the interface errors should also be returned, so a function like that would have a horrible signature.

[bugadani]

	push	{r4, r5, r6, r7, lr}
	add	r7, sp, #12
	str	r8, [sp, #-4]!
	and	r12, r2, #248
	mov.w	lr, #0
	lsl.w	r12, r12, #4
	uxtab	r12, r12, r1
	cmp.w	lr, r12, lsr #10 ; this is definitely the index check, compares to 1024
	bne	.LBB35_2
	ldrb.w	lr, [r0, #1044]
	uxtb	r1, r1
	ldrb.w	r5, [r0, #1046]
	uxtb	r4, r2
	ldrb.w	r8, [r0, #1045]
	cmp	lr, r1
	ldrb.w	r6, [r0, #1047]
	it	hi
	movhi	lr, r1
	cmp	r5, r4
	add	r12, r0
	strb.w	lr, [r0, #1044]
	it	hi
	movhi	r5, r2
	cmp	r8, r1
	strb.w	r5, [r0, #1046]
	it	hi
	movhi	r1, r8
	add.w	r12, r12, #20
	strb.w	r1, [r0, #1045]
	cmp	r6, r4
	it	ls
	movls	r6, r2
	strb.w	r6, [r0, #1047]
	and	r0, r2, #7
	ldrb.w	r1, [r12]
	lsl.w	r2, r3, r0
	movs	r3, #1
	lsl.w	r0, r3, r0
	bic.w	r0, r1, r0
	orrs	r0, r2
	strb.w	r0, [r12]
.LBB35_2:
	ldr	r8, [sp], #4
	pop	{r4, r5, r6, r7, pc}

Not bad. The 5 conditionals are (well, should be, haven't really checked that close) the index check and the 4 min/max calls. Funny that half of the assembly is related to the min/max tracking :)

[therealprof]

Currently, changing the rotation is only possible via a complete reconfiguration. I'm not sure if I want to reintroduce it, though, since the type would change and the interface errors should also be returned, so a function like that would have a horrible signature.

Shouldn't that be rather easy to implement? I mean, it's basically just a function telling the display to interpret the pixels differently and then change the typestate into the new rotational typestate. The HAL impls are doing the very same all the time for e.g. the GPIO pins.

[bugadani]

Yes and no. Handling failure can get messy, because the func must return the old object and the error. In fact, if one if the two commands fail for some reason, there may not be a clear recovery path. I may be overcomplicating things but I don't necessarily want a fire and forget configuration.

I certainly would be shocked to see a return value with a type similar to

Result<DisplayInterface<<I>, DisplaySize128x64, Rotate90>,
(DisplayInterface<<I>, DisplaySize128x64, Rotate0>, DisplayError)>

🤯

It's possible to restore the original functionality by adding a MutableRotation type and make it the default, and implement set_rotation for that type only.

[therealprof]

If your I2C communication fails during configuration it's game over anyway and you can only start over. There's no way of telling where it failed... Thinking about it some more it probably doesn't make too much sense to offer live rotation anyway since it's way too complex too handle, just changing the display setup is not enough, you'd actually have to repaint the whole screen for which you need new pixel data in case you're rotating by 90° or 270° since the aspect ratio most likely changes.

[bugadani]

I really don't think trying to figure out error handling is in the scope of a display driver. However, I believe consuming a display interface in case of an error is impolite, panicing is simply wrong and eating the error is also incorrect.

I mostly agree with "If your I2C communication fails during configuration it's game over anyway", but theoretically it's not always a communication failure. What if someone implements a virtual driver that can fail to take the peripherial and returns that as an error? Then there is the possiblity to retry later.

I'm also worried if easy change of typestate rotation would possibly cause binary bloat, since every draw operation would be duplicated for the rotation types (the whole point of this PR is an optimized draw operation support), and I'm fairly certain rustc isn't smart enough to deduplicate.

There's also the ease of use aspect: if I just want to rotate my screen, do I really need to add an awful lot of boilerplate code to support plugging in a rotation value into my display driver?

I've implemented dynamic rotation to allow a tradeoff between convenience and performance. If someone needs performance over convenience, they can consume and reinitialize.

There are still several open topics and to do points in this PR:

• Names (I could certainly use some suggestions here)
• What should be the default rotation (I see arguments for both fixed 0 and dynamic)
• Formatting
• Documentation
• Getting rid of * imports
• Examples

[therealprof]

I mostly agree with "If your I2C communication fails during configuration it's game over anyway", but theoretically it's not always a communication failure. What if someone implements a virtual driver that can fail to take the peripherial and returns that as an error? Then there is the possiblity to retry later.

Maybe, maybe not. The problem is that there's a sequence of steps which need to be followed to change the display configuration and if any error occurs the display really could be in any state (including lock-up) and neither the user nor the driver know which one that is. The only sane way to handle this IMHO would be to give the resources back and let the application decide what to do with them.

[therealprof]

NB: Those comments are just some food for thought. I don't care either way whether and how we support screen rotation at runtime because I'm certain it's not easy to cover every of the many angles and I'm also not conviced there's great benefit to supporting that. It'd be much easier to allow for proper display destruction and have an application needing that feature reinitialize the display, that way the behaviour is clear.

[bugadani]

NB: Those comments are just some food for thought. I don't care either way whether and how we support screen rotation at runtime because I'm certain it's not easy to cover every of the many angles and I'm also not conviced there's great benefit to supporting that. It'd be much easier to allow for proper display destruction and have an application needing that feature reinitialize the display, that way the behaviour is clear.

I'd like to add that we're in no rush to get this merged, if it ever will be :) I'm open to suggestions if the current solution is not satisfactory for any reason. In fact, I'm quite aware that having 3 (and with #124, 4) template parameters are rather overwhelming to work with, but my main goal was to get this working first and clean up the specifics later if necessary.

[bugadani]

Well, that looks a lot nicer, unit-like tuple structs are quite useful.

Do you have opinions about changing rotation to orientation? I believe it's a bit more straightforward, since display orientation is actually a display property, while image rotation is kinda related to the render part?
I find myself often confused by what a 90° image rotation means, on an intuitive level it feels backwards for me. "I rotate the image 90° clockwise, which means I need to rotate my device 90° CCW".

[therealprof]

I don't have any opinion on the naming. Either seems fine to me.

[jamwaffles]

The type-level display size makes sense to me as it can never change, but why type-level rotation? Is it for optimisation reasons? The distinction between fixed and non-fixed rotations is odd as well.

I find myself often confused by what a 90° image rotation means, on an intuitive level it feels backwards for me. "I rotate the image 90° clockwise, which means I need to rotate my device 90° CCW".

For this example, imagine a device where the display is screwed into the case rotated 90deg CCW. In this case, the image would need to be rotate 90deg CW to show in the correct orientation to the user. We could reduce confusion by renaming DisplayRotation to ImageRotation. I'd prefer to stick with "rotation" over "orientation" unless there are objections.

[bugadani]

Yes, the rotation is there for optimization. Fixed produces the simplest machine code, but takes away set_rotation, which is problematic to implement with changing typestate, and can also be done by reinitializing the display anyway. The non-fixed rotation adds the set_rotation method, and it is used by default to keep compatibility. I'm sure the wording can be improved somehow.

Renaming to ImageRotation seems like a good compromise to ease my confusion :)

[bugadani]

For now I've removed the rotation related changes from this PR.

The current PR contains the following:

• remove the old DisplaySize enum
• add new unit-like display size types
• adds a type parameter to *Mode and DisplayProperties, the default is the 128x64 display
• source compatibility for anyone who used the default display size
• only the necessary size framebuffer is allocated
• Commands are made public, in order to allow custom display sizes
• displaysize module is public, but reexported via prelude (umm... yeah, this is probably not nice)
• added displayproperties to prelude

I've also incorporated a fix for #122

[jamwaffles]

Sorry it's taken a while to get round to this. Overall it looks good. I like the encoding of hardware properties in the type system. Some small extra comments, after which I think this is good to merge.

[bugadani]

Sorry it's taken a while to get round to this.

No worries. I've done the requested modifications. Thanks for the feedback :)

[jamwaffles]

Looks good!

@therealprof Anything in here you don't agree with? Otherwise I'll get this merged.

[jamwaffles]

Thanks!