rust: Concrete renderers #4137
Draft
rust: Concrete renderers #4137
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
code for given models wouldn't build without these
get_frame_buffer would indiscriminately hand out a &'static mut reference to the same memory. In practice these references are short-lived, but the function as written is not safe.
This also makes Mono8 display use the separate bumps instead of a single larger one, because the merge is introducing an unnecessary difference
|
note: switch on "hide whitespace" to review |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR reworks the way we invoke display renderers, replacing closure-based
run_with_bumpswith a guard-based locking mechanism, and replacingrender_on_displaywith a macro.This significantly reduces the amount of trickery needed to get Rust to accept the various lifetime bounds (not to zero, unfortunately :( ), and -- perhaps more importantly -- it allows us to create a
Displaytrait that is a parameter ofUIFeaturesCommon, cleanly binding each model to a particular Display implementation and providing an obvious location to change it without relying on exclusive features.(ideally this would also let us remove the
display_xxxxfeatures, because we can now build all the impls at the same time, and select the one to be used via UIFeatures. @cepetr do you think this is viable?)the drawback is that you can no longer freely build, say, model_mercury UI for the Trezor T hardware just by switching some feature flags, because ModelTTFeatures specify the NoFb implementation. But if this is desired, we can move the feature-flag selection into ModelTTFeatures (which is frankly a better place for it because model TT only works on color displays anyway)
Importantly,
Displaydoes not have lifetime parameters, which makes it very useful downstream -- e.g., it would be possible to keepLayoutObjobject-safe even if parametrized byDisplay. (which is the endgame here -- get rid of all "selectors" that import different modules based on which model is selected, and so be able to e.g. run all rust tests for all models from a single binary)additionally, f2baf82 fixes unsound API of
trezorhal::display::get_frame_buffer