Closed bugadani closed 4 years ago
I wish there was a way to skip CI on drafts... this really is a waste of energy at this point
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 :)
Indeed, the associated constants are nice. I guess I went trigger-happy with the typenum crate
Now that's pedantic, failing build because of an extra empty line 😂
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.
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 :)
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.
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.
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.
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:
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.
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.
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.
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".
I don't have any opinion on the naming. Either seems fine to me.
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.
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 :)
For now I've removed the rotation related changes from this PR.
The current PR contains the following:
I've also incorporated a fix for #122
Sorry it's taken a while to get round to this.
No worries. I've done the requested modifications. Thanks for the feedback :)
Thanks!
Hi! Thank you for helping out with SSD1306 development! Please:
master
if you're not already up to dateCHANGELOG.md
entry in the Unreleased section under the appropriate heading (Added, Fixed, Changed, etc)rustfmt
on the project withcargo fmt --all
- CI will not pass without this stepPR description
This is just a rough draft of how I imagine type-level display sizes. This has several added benefits:
The downside is the amount of code changed and the API it broke...
Closes #20 Closes #122