Skip to content
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

Automatically delete Obj and related on drop #144

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Conversation

nia-e
Copy link
Collaborator

@nia-e nia-e commented Jun 21, 2023

Yet more work towards #140, currently very broken and will require removing some quality-of-life functions i.e. new().

@nia-e nia-e added enhancement New feature or request breaking Breaks or changes the API labels Jun 21, 2023
@nia-e nia-e added the help wanted Extra attention is needed label Jun 21, 2023
@nia-e
Copy link
Collaborator Author

nia-e commented Jun 21, 2023

Considering changing removal of functions to be behind a feature gate i.e. static-obj-memory so that platforms where the overhead is "tolerated" don't need to worry about all of this and can still make everything 'static. Alternatively, we can just have this done anyways with e.g. Box<T>::leak(mut self) -> &'static mut T which is still in the code but commented out and expose .leak() on T: NativeObject types. Would like to hear some opinions

@cydergoth
Copy link
Contributor

cydergoth commented Jun 21, 2023 via email

@nia-e
Copy link
Collaborator Author

nia-e commented Jun 21, 2023

How is this handled in C?

@cydergoth
Copy link
Contributor

cydergoth commented Jun 21, 2023 via email

@nia-e
Copy link
Collaborator Author

nia-e commented Jun 21, 2023

Alright. I'll open an issue for it when I get back to working on this

@cydergoth
Copy link
Contributor

See an example here:
static LV_STYLE_CONST_INIT(focus_style, focus_style_props);

@cydergoth
Copy link
Contributor

Working from this branch, I get

 (cd ~/workspace/rust/lvgl-mm/ && RUST_LOG=debug RUST_BACKTRACE=1 DEP_LV_CONFIG_PATH=$(pwd) cargo build)
  5    Compiling lvgl-mm v0.1.0 (/home/jcrisp/workspace/rust/lvgl-mm)
  6 error[E0597]: `screen_style` does not live long enough
  7    --> src/main.rs:144:34
  8     |
  9 144 |     screen.add_style(Part::Main, &mut screen_style);
 10     |     -----------------------------^^^^^^^^^^^^^^^^^-
 11     |     |                            |
 12     |     |                            borrowed value does not live long enough
 13     |     argument requires that `screen_style` is borrowed for `'static`
 14 ...
 15 187 | }
 16     | - `screen_style` dropped here while still borrowed
 17
 18 error[E0597]: `screen` does not live long enough
 19    --> src/main.rs:151:45
 20     |
 21 151 |     let root_id = build_tree(None, Box::new(&mut screen), cfg, &mut widget_tree);
 22     |                   --------------------------^^^^^^^^^^^-------------------------                                                                                                        23     |                   |                         |                                                                                                                                           24     |                   |                         borrowed value does not live long enough
 25     |                   argument requires that `screen` is borrowed for `'static`                                                                                                             26 ...
 27 187 | }
 28     | - `screen` dropped here while still borrowed
 115 async fn display() -> Result<(), LvError> {
 116     const HOR_RES: u32 = 480;
 117     const VER_RES: u32 = 320;
 118
 119     let sim_display: SimulatorDisplay<Rgb565> =
 120         SimulatorDisplay::new(Size::new(HOR_RES, VER_RES));
 121     let output_settings = OutputSettingsBuilder::new().scale(1).build();
 122     let mut window = Window::new("LVGL-MM", &output_settings);
 123
 124     // LVGL will render the graphics here first, and seed the rendered image to the
 125     // display. The buffer size can be set freely.
 126     let buffer = DrawBuffer::<{ (HOR_RES * VER_RES) as usize }>::default();
 127
 128     // Register your display update callback with LVGL. The closure you pass here will be called
 129     // whenever LVGL has updates to be painted to the display.
 130 //    let display = Display::register(buffer, HOR_RES, VER_RES, |refresh| {
 131 //        sim_display.draw_iter(refresh.as_pixels()).unwrap();
 132 //    })?;
 133
 134     println!("Before all widgets: {:?}", mem_info());
 135
 136     let mut screen_style = Style::default();
 137     screen_style.set_bg_color(Color::from_rgb((0, 0, 0)));
 138     screen_style.set_text_color(Color::from_rgb((255, 255, 255)));
 139     screen_style.set_bg_opa(Opacity::OPA_COVER);
 140     screen_style.set_radius(0);
 141
 142     // Create screen and widgets
 143     let (_display, mut screen) = lv_drv_disp_sdl!(buffer, HOR_RES, VER_RES)?;
 144     screen.add_style(Part::Main, &mut screen_style);
.....

Not sure how this is supposed to work with code which isn't in the main() function or 'static scope

@nia-e
Copy link
Collaborator Author

nia-e commented Jun 26, 2023

This is "intended" and fixable but needs documentation. The issue is that theoretically Box::new(x: T) requires that T lives arbitrarily long, which is fine to say for structs etc. but not for most references. I think what you could do is make the Boxing happen sooner and access the screen through it, so

let boxed_screen = Box::new(screen); // not &mut screen
boxed_screen.add_style(&mut style);

Not sure how this is supposed to work with code which isn't in the main() function or 'static scope

Assigning the style in a function that doesn't move that style into the same scope or greater than the object/screen it's assigned to is technically a use-after-free. You can get around this by leaking the style object (with mem::leak(), or by wrapping it in a ManuallyDrop) or by returning the style(s) from the called function.

@nia-e
Copy link
Collaborator Author

nia-e commented Jun 26, 2023

Also note that this branch is WIP and will segfault if you do anything with it - tests pass, examples don't work though. I'm currently in the middle of a move so can't really work more until wednesday, sorry.

@cydergoth
Copy link
Contributor

No hurry, this is hobby space for me

@nia-e nia-e removed the help wanted Extra attention is needed label Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaks or changes the API enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants