-
Notifications
You must be signed in to change notification settings - Fork 46
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
Builder pattern refactor #32
Conversation
@xgroleau could you please check if things still work fine for you with this PR? I think this should now finally enable proper orientation with some needed code simplification. |
Looks good! Though when I use the the new drivers, nothing is written to the screen. Maybe my init sequence is not valid? Though it seems fine. Here is the code that I used Before let mut delay = Delay;
let di = SPIInterface::new(spi, dc, cs);
let display_cfg = DisplayOptions {
orientation: mipidsi::Orientation::PortraitInverted(false),
..Default::default()
};
let mut display = mipidsi::Display::st7789_240x240_b240x320(di, Some(reset), display_cfg);
display.init(&mut delay).unwrap(); After let mut delay = Delay;
let di = SPIInterface::new(spi, dc, cs);
let display = mipidsi::Builder::st7789(di)
.with_display_size(240, 240)
.with_framebuffer_size(240, 320)
.with_orientation(mipidsi::Orientation::PortraitInverted(false))
.init(&mut delay, Some(reset))
.unwrap(); |
Huh, that's odd. I have a 240x320/240x240 display (waveshare) too and things work here. Things work if you don't specify the orientation? |
I just tested your exact init sequence and it works ok for me o.O You get absolutely nothing on the display this way? |
I think it's an issue on my communication bus, looking up what is going on. It's probably not the lib actually |
Ah, one thing that comes to mind, do you use the CS pin? Make sure it's set right if you use it "manually" (e.g. outside the SPI struct), it's possible it has been flipped from last time and is now always "high" making the display not listen. Something like this happened to me once, was a nice head scratcher. |
It does draw properly but then the device is cleared immediately after rendering, not sure why though, still investigating. Maybe an issue with my reset pin. Not sure why it works with the previous version and not this one though :/ |
Hmm I did change the reset pin logic a bit. It shouldn't cause any functional changes but now the pin is no longer owned for the duration of the Display's existence, only during init. Is it possible you're accessing the same pin afterwards? Could you try making sure the pin is not used completely? Reset should work if you pass |
That's what i thought, probably that the pin goes out of scope, is dropped and dropping it causes a reset. |
Yep that was it. Just so you know, it is still required to give a type to the None, which causes some awkward typing like this to make the compiler happy. Though that is just a general issue when using generics
To avoid, I personally create a dummy implementation that cannot be instantiated. See here for the |
After fixing the pin issue, all orientations works fine! |
Good point, I'll think about how to solve it. Question on the pin though, how come dropping it caused a reset? is this part of the HAL implementation for GPIO in your case? I find it odd that dropping a pin should change its function or set a value on it. |
When dropped, embassy, the hal that I use, try to optimise for power consumption. Thus dropping a pin deconfigures and when deconfigured, it's in an unknown state. |
Ok that changes things quite a bit. I will have to re-introduce the HW pin into the I'm also considering just not allowing hw reset pin and depending on sw reset only as the other alternative, that way if someone does use the HW reset pin they will know it needs to be handled properly. Not sure which way to go tbf. |
Is there anything that the software reset cannot do that the hardware reset can? If not, software only may be a good option if you want to avoid generics. I personally don't mind the added generic if there is a gain, but some may prefer avoiding it. |
AFAICS the spec says sw reset is a must. The only thing I can think of is if somehow the device stops responding to commands, a hardware reset could bring it back, but then so can a power cycle then. I'll probably add the param back to make it safer, it's not such a huge deal. |
I think I have found a solution to this that doesn't necessitate either. Can you please retry the "drop the pin" case again? It should work ok now. |
After testing the fix doesn't work. After checking the drop implementation, the pin is actually disconnected, so a floating pin. And after some testing, I actually DO need to initialize the pin and keep it in scope for the whole duration of the program (as in just passing None and never initializing the pins does not work), thus probably ownership would be required? Maybe this is caused by a weak pull resistor on my board though, I am using a OEM board so I don't have the schematics. |
Interesting, so essentially you either need to not connect the HW reset pin to prevent flutter from the MCU, or make sure that it's initialized and locked in properly for the whole duration. I'll reintroduce the pin ownership then, it won't prevent cases where people send in |
Perfect thanks a lot. And yes, electrical issue due to a floating pin is definitely a user error, as long as there is a small line in the doc for that, it should be fine. |
I just checked the spec. The RESX pin needs to be kept high for normal display operation and lowering it for over 5us will cause a reset. The display will NOT work if the RESX is kept low however, so technically it's a requirement. I will keep it optional on the basic level for those cases where the reset pin is hardwired to VIN. |
@xgroleau sorry for the spam today :) could I bother you with one last test run please? The pin is now owned again (if supplied). |
I can confirm that it works properly! No worries, I'm more than happy to help :) |
This PR tries to address a few things with a major refactor:
set_orientation
broken if done before init #31 by forcing init before use via typestatesDisplay
via the builder pattern and unifies all options into one placeframebuffer > display size
default offset handling logicRST
reset pin fromDisplay
itself into the builder'sinit
function*Together these 3 changes should make things more straight forward while removing a set of possible bugs if users called things like
set_orientation
before callinginit
. It also simplifies theModel
by removing theModelOptions
ownership .init
call. This means that someone could change that pin to other state, or send a reset signal in the middle of the display doing something. I consider this an extreme edge case not worth the "consume" logic that'd be needed here and additional generic param propagation +release
logic.