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

Creating example grid in Rust based on C code example #107

Closed
vortex314 opened this issue May 3, 2023 · 21 comments · Fixed by #141
Closed

Creating example grid in Rust based on C code example #107

vortex314 opened this issue May 3, 2023 · 21 comments · Fixed by #141
Labels
bug Something isn't working enhancement New feature or request

Comments

@vortex314
Copy link

As I am trying to build the example in Rust which is documented here : https://docs.lvgl.io/8.3/layouts/grid.html?highlight=gris#example

It's not clear how I can create a container within the screen , a Obj cannot be created in the way I would expect , meaning

Obj::create(&mut screen )

The result looks like this :
image
while I was expecting this :
image

Code can be found here :
https://github.com/vortex314/lv_1/blob/main/src/main.rs

Changing Part::Any or Part::Main doesn't make a difference.

@vortex314 vortex314 changed the title Creating example grid in Rust based on C code Creating example grid in Rust based on C code example May 4, 2023
@nia-e
Copy link
Collaborator

nia-e commented May 4, 2023

Hi! Sorry for the late reply, have been busy. That's an oversight in how the codegen was done since methods are not autogenerated for Objects. See #108. Also, thank you for all the issues filed; I'd been very focused on raw feature work with only minimal testing and it's good to see pain points being addressed.

@nia-e nia-e added bug Something isn't working enhancement New feature or request labels May 4, 2023
@nia-e
Copy link
Collaborator

nia-e commented May 4, 2023

Note that you will have to substitute the C call to lv_obj_set_grid_cell() with instead creating a Style with the individual parameters and adding it; we do this to prevent duplicating functionality and creating too many methods which all do the same thing.

@vortex314
Copy link
Author

Some progress, still no grid.
image
Sources on the usual location : https://github.com/vortex314/lv_1

@nia-e
Copy link
Collaborator

nia-e commented May 5, 2023

After digging into everything, I still have absolutely no clue what's going on and why it's not working. I'll get back to this tomorrow and try to get it fixed then though.

@vortex314
Copy link
Author

vortex314 commented May 5, 2023

Already a lot of thanks for all the effort you are putting into this.
Just for information , I also generated this with SDL but the same result.

@nia-e
Copy link
Collaborator

nia-e commented May 6, 2023

Seems like you uncovered something pretty gnarly here; see #110 for details. Until a proper solution is decided on, this works:

let mut buttons = Vec::new();
let mut styles = Vec::new();

for i in 0..9 {
    let col = i % 3;
    let row = i / 3;

    let btn = {
        buttons.push(Btn::create(&mut cont)?);
        buttons.last_mut().unwrap()
    };
    let btn_style =
    {
        styles.push(Style::default());
        styles.last_mut().unwrap()
    };
    btn_style.set_grid_cell_column_pos(col);
    btn_style.set_grid_cell_row_pos(row);
    btn_style.set_grid_cell_column_span(1);
    btn_style.set_grid_cell_row_span(1);
    btn_style.set_grid_cell_x_align(GridAlign::STRETCH);
    btn_style.set_grid_cell_y_align(GridAlign::STRETCH);
    btn_style.set_width(70);
    btn_style.set_height(50);

    info!(" row {} col {}", row, col);
    btn.add_style(Part::Main, btn_style)?;

    let mut label = Label::create(btn)?;
    let s = CString::new(format!("c{}, r{} ", col, row)).unwrap();
    label.set_text(&s)?;
    label.set_align(Align::Center, 0, 0)?;
}

@vortex314
Copy link
Author

Thanks for the deep dive to solve the issue, looks you found something that would create an issue iin any language that uses lvgl. Hope it get addressed quickly.

@rafaelcaricio
Copy link
Collaborator

LVGL seems to expect everything to be statically declared. Examples in C always declare statically the UI elements.

https://github.com/lvgl/lvgl/blob/master/demos/music/lv_demo_music_list.c#L31-L41

@nia-e
Copy link
Collaborator

nia-e commented May 7, 2023

That's.... terrifying, frankly. I suppose we can just leak the Style?

@rafaelcaricio
Copy link
Collaborator

@nia-e yeah, I think that is a fair approach.

@vortex314
Copy link
Author

vortex314 commented May 8, 2023

I can imagine where the static lifetime expectation comes from , it looks a lot like CSS in an HTML document that also exist permanently during the display. Or coming from an embedded system where styles are in FLASH as constants. But then I would expect a big warning in the doc specifying that lvgl styles should be static. Maybe I missed that in the lvgl doc.

Changing this at the LVGL C source code , is probably very bad for backward compatibility

Maybe push the lifecycle of the styles to the programmer. Who has to do the bookkeeping of all these styles in line with the objects to which it applies. Managing that in the RUST layer above LVGL creates a lot of maintenance in the future.

Some other ideas :
Or as the styles can be re-used across objects, do some reference counting (Arc) connected to the objects.
Or put the styles in the object_user_data , but then again this would remove the possibility from the programmer. to use this user data. lv_obj_set_user_data

For the moment I extended your approach, see below.
I wouldn't go for leaking just the memory. It's what Rust tries to avoid at all price.

fn new_grid_style<'a>(
    v: &'a mut Vec<Style>,
    x: i16,
    y: i16,
    x_size: i16,
    y_size: i16,
) -> &'a mut Style {
    v.push(Style::default());
    let style = v.last_mut().unwrap();
    style.set_pad_bottom(0);
    style.set_pad_top(0);
    style.set_pad_left(0);
    style.set_pad_right(0);
    style.set_grid_cell_column_pos(x);
    style.set_grid_cell_row_pos(y);
    style.set_grid_cell_column_span(x_size);
    style.set_grid_cell_row_span(y_size);
    style.set_grid_cell_x_align(GridAlign::STRETCH);
    style.set_grid_cell_y_align(GridAlign::STRETCH);
    style
}

@rafaelcaricio
Copy link
Collaborator

There is no Vec or Arc for embedded programming, that is a big restriction for this crate.

@nia-e
Copy link
Collaborator

nia-e commented May 8, 2023

We might have a way out of this still, though. We have the lvgl_alloc feature and we could use some of that code to allow us to use alloc in a few contexts, e.g. creating a Vec. I'm not sure how much it'll help us here, but shrug

@vortex314
Copy link
Author

vortex314 commented May 9, 2023

It's hidden / mentioned in the doc : https://docs.lvgl.io/master/overview/style.html#initialize-styles-and-set-get-properties

Styles are stored in lv_style_t variables. 
Style variables should be static, global or dynamically allocated. 
In other words they cannot be local variables in functions which are destroyed when the function exits.

Should have a big red flashing box around it. ;-)

@nia-e
Copy link
Collaborator

nia-e commented May 9, 2023

I saw that but it's very very easy to cause problems if we never drop styles, e.g. a UI which creates a new screen on function call instead of initializing a screen once and reusing it causing memory leaks all over the place (which is exactly what I first intuited coming from Rust, assuming that everything will be nicely dropped as needed)

@rafaelcaricio
Copy link
Collaborator

I thought a lot about storing and handling the life-time of those types of objects within the crate. But that may take considerable memory, and I'm not certain about how acceptable that is in constrained environments. I agree that leaking memory is not great, I just don't know what to suggest as an alternative.

@nia-e
Copy link
Collaborator

nia-e commented May 9, 2023

As far as I understand, as long as we strictly use PhantomDatas for holding lifetimes, verification is entirely handled at compile time and we don't pay for it at runtime in any way since it exclusively serves to tell the borrow checker what's okay and what isn't. I'm unsure if this exact idea works, but vaguely:

struct Style<'a> {
    inner: lv_style_t,
}

struct Obj<'a> {
    ptr: *mut lv_obj_t,
    // We pretend to own a reference (this gets optimized away)
    styles_used: PhantomData<&'a lv_style_t>,
}

impl<'a> Obj<'a> {
    fn add_style(&mut self, &'b style: Style) {

        /* call the lvgl_sys function for adding a style as we do now */

        // Sum lifetimes; 'a + 'b becomes the new 'a
        // If this is for some reason impossible, we can simply make add_style consume self and return a new Self
        self.styles_used = PhantomData<&'a + 'b lv_style_t>;
    }
}

Since at the end we assign to a zero-sized marker type, I'm pretty sure it gets optimized away. I will manually check the generated code before merging anything like this, though.

@nia-e
Copy link
Collaborator

nia-e commented May 15, 2023

TL;DR I was right about it being fine

I tested this out in Compiler Explorer and the codegen is indeed just 1 mov even with a PhantomData. I was wrong about the syntax but thankfully it's simpler than what I thought (specifying 2 different lifetimes as 'a takes the intersection so whichever is shorter)
See https://godbolt.org/z/vo1TfdYn5

@vortex314
Copy link
Author

vortex314 commented May 15, 2023

The approach of PhatomData ( which I don't fully understand being noob on Rust ) has been implemented ? So I would be able to avoid keeping styles in a static Vec ?

@nia-e
Copy link
Collaborator

nia-e commented May 15, 2023

No, not yet - this is still a sketch as it would be a massive change to the internals. Also, holding the styles in some sort of collection or moving them into the greater scope would still be necessary (just we're making it so the compiler should properly complain instead of silently allowing it)

@nia-e
Copy link
Collaborator

nia-e commented May 15, 2023

Re: CE code, seems like we can also reliably bet on this happening and it's not "just" that we got lucky; in MIR, the create function ends up as

fn <impl at /app/example.rs:12:1: 12:19>::create(_1: u8, _2: &Ghost) -> Holds<'_> {
    debug data => _1;                    // in scope 0 at /app/example.rs:14:19: 14:23
    debug _ghost => _2;                  // in scope 0 at /app/example.rs:14:29: 14:35
    let mut _0: Holds<'_>;               // return place in scope 0 at /app/example.rs:14:51: 14:55

    bb0: {
        _0 = Holds::<'_> { data: _1, phantom: const PhantomData::<&Ghost> }; // scope 0 at /app/example.rs:15:9: 21:10
        return;                          // scope 0 at /app/example.rs:22:6: 22:6
    }
}

whereas in LLVM IR (with unmangled names), we have

define noundef i8 @create(i8 noundef returned %data, ptr noalias nocapture noundef readonly align 1 dereferenceable(64) %_ghost) unnamed_addr #0 {
  ret i8 %data
}

so it gets eliminated even before LLVM IR is generated but after MIR which makes sense (on low opt-level it seems to still get optimised out but the struct creation is pretty ugly:

define i8 @create(i8 %data, ptr align 1 %_ghost) unnamed_addr #0 {
  %0 = alloca i8, align 1
  store i8 %data, ptr %0, align 1
  %1 = load i8, ptr %0, align 1, !noundef !2
  ret i8 %1
}

such is the price for debugging, I suppose). I guess this makes sense since there's no machine-code way to express the idea of doing anything with a ZST (or even acknowledging it exists)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants