Rust bindings API review #51

rafaelcaricio · 2021-06-04T20:35:34Z

This PR makes the Rust bindings usage more ergonomic and flexible.

rafaelcaricio · 2021-06-04T20:36:28Z

lvgl/src/lv_core/obj.rs

+//         let mut parent = crate::display::DefaultDisplay::get_scr_act()?;
+//         Ok(Self::create_at(&mut parent)?)
+//     }
+// }


This is used during development, to test the code generation options.

rafaelcaricio · 2021-06-04T20:37:20Z

lvgl/src/mem.rs

@@ -69,14 +69,33 @@ impl<T> AsMut<T> for Box<T> {
    }
 }

+fn mem_info() -> lvgl_sys::lv_mem_monitor_t {


Probably just need to make safe bindings for this simple function and add to the public API. It's useful for app debugging.

rafaelcaricio · 2021-06-04T20:38:52Z

lvgl/src/widgets/label.rs

+#[cfg(feature = "alloc")]
+impl<S: AsRef<str>> From<S> for Label {
+    fn from(text: S) -> Self {
+        let text_cstr = CString::new(text.as_ref()).unwrap();


This is a test of what is possible with alloc feature enabled. In reality, the From trait implementation should use the TryFrom trait implementation and just call .unwrap() here. People will decide which guarantees they want to have.

rafaelcaricio · 2021-06-04T20:40:47Z

lvgl/src/lib.rs

+    use embedded_graphics::pixelcolor::Rgb565;
+
+    pub(crate) fn initialize_test() {
+        init();


All tests run in the same LVGL "instance" so the tests might run out of memory at some point. :) Just to keep in mind.

lvgl/src/display.rs

rafaelcaricio · 2021-06-04T20:43:14Z

lvgl/src/display.rs

+
+pub struct DrawBuffer<const N: usize> {
+    initialized: RunOnce,
+    refresh_buffer: Mutex<RefCell<heapless::Vec<lvgl_sys::lv_color_t, N>>>,


I use a Mutex here, to satisfy the Rust guarantees. But maybe it is not necessary... some food for thought.

rafaelcaricio · 2021-06-04T20:44:13Z

lvgl-sys/build.rs

@@ -114,6 +114,7 @@ fn main() {
    let bindings = bindgen::Builder::default()
        .header(shims_dir.join("lvgl_sys.h").to_str().unwrap())
        .generate_comments(false)
+        .derive_default(true)


Extremely useful feature of bindgen. 💅🏽

rafaelcaricio · 2021-06-08T20:51:59Z

lvgl/src/display.rs

+            // TODO: needs to be 'static somehow
+            // Cannot be in the DrawBuffer struct because the type `lv_disp_buf_t` contains a raw
+            // pointer and raw pointers are not Sync and consequently cannot be in `static` variables.
+            let mut inner: MaybeUninit<lvgl_sys::lv_disp_buf_t> = MaybeUninit::uninit();


I would like to find a way to make this memory statically allocated. This seems to be a requirement for LVGL v8.

rafaelcaricio · 2021-06-08T20:52:46Z

lvgl/src/display.rs

+
+pub struct DrawBuffer<const N: usize> {
+    initialized: RunOnce,
+    refresh_buffer: Mutex<RefCell<[MaybeUninit<lvgl_sys::lv_color_t>; N]>>,


I'm using a Mutex here just to make this Sync, not sure if there is a way to remove it but still keep the memory statically allocated.

rafaelcaricio · 2021-06-08T20:53:27Z

examples/app.rs

+    lvgl::init();
+
+    const REFRESH_BUFFER_SIZE: usize = lvgl::DISP_HOR_RES * lvgl::DISP_VER_RES / 10;
+    static DRAW_BUFFER: DrawBuffer<REFRESH_BUFFER_SIZE> = DrawBuffer::new();


Making this statically allocated will avoid dropping this memory address by mistake in an app code.

nia-e · 2023-03-02T01:04:19Z

What's the status on this PR? I was considering attempting to upgrade LVGL to 8.x and it seems like this is necessary. I can rebase on current master and flesh out any missing features if that would be desirable.

rafaelcaricio · 2023-03-02T10:26:42Z

@nia-e As described in the README of this project. I've moved on from it, since I couldn't find anyone to help me with the development. That means this PR is in its "final" state, as I don't plan to work further on it. I'm pleased that you seem to have interest in this project, and if you think this can or should be merged, please go ahead!

I think this PR has some things that need a second opinion. I've added some comments to things I wanted to highlight. The public API changes here, and we remove explicit dependency on embedded_graphics (it is optional).

There is a part of this PR that I stopped working on before making use of it, which is in the code generation crate. I think we could do much better there. Just keep in mind that I've started this project to help me "learn Rust", so take with a grain of salt the things this library does. If you see something that seems wrong to you, it is because it is probably wrong indeed. 👀

nia-e · 2023-03-02T11:32:17Z

To my utter surprise, a rebase and minimal fixes later, this compiles. I'd rather test more things before I do anything but it seems like my job is easier than expected. Should I open a new PR or push to this branch? I squashed all commits.

rafaelcaricio · 2023-03-02T11:43:03Z

@nia-e Awesome, Rust is great. lol

You can commit to this branch and continue this PR.

nia-e · 2023-03-02T15:16:24Z

What were the plans for lvgl_codegen? It has some unused code and behavior is missing. I could try to finish implementing whatever the original idea was.

nia-e · 2023-03-02T17:30:44Z

Also; further digging on the Mutexes revealed that there is Probably No Way (that I could find) to get rid of them short of marking this entire library as non-thread-safe which doesn't feel great. I'd love to be proven wrong, though. parking_lot is a very small crate though and this shouldn't be a huge deal - certainly far better than pulling in all of embedded-graphics at all times like before.

rafaelcaricio · 2023-03-02T17:58:45Z

@nia-e The idea with the lvgl-codegen crate was to be able to generate the entirety of the LVGL C API instead of manually writing bindings for every single API function LVGL exposes. I wanted to have in the lvgl crate the "core abstractions" that would be used by the codegen. This was a suggestion I got at the begging of this project. That thread might be a good source of what I was thinking at the time too (it has been a while ago 😬 ).

LVGL is not thread-safe itself, so making this crate non-thread-safe shouldn't be a bid deal? 👀 For us to make this crate "thread-safe" we would need to (have and) lock a global Mutex every time we call LVGL C API, not sure if that is a good idea!?

nia-e · 2023-03-02T18:01:51Z

Perfect, okay. I'll drop the Mutexes and read through that thread afterwards.

Revising the public API for better flexibility Use old approach, write directly to DrawTarget Simplified API for display initialization Possibility to register a shared native display Fix shared display Update demo app to use new api Use ref Add basic API functions Add required features Update examples Make disp buffer and driver statically allocated Allow to create statically allocated DrawBuffer Make DrawBuffer customizable using const generics Update default features to empty Allow users to define a display refresh callback closure Make use of embedded_graphics optional Update app example Remove dependency on heapless Wrap alloc extension in its own module

nia-e · 2023-03-02T19:25:04Z

~~Seems like extending lvgl-codegen is a can I can safely kick down the road. All in all, I think this is complete and ready for review. Squashed commits.~~
One more thing I need to fix then it'll be ready.

nia-e · 2023-03-02T20:23:23Z

Okay, now it's ready. Give this a look when you have some time @rafaelcaricio. Also this hopefully beings to close #42

get rid of warnings

rafaelcaricio · 2023-03-02T22:42:01Z

LGTM. I cannot approve as I originally opened the PR. Feel free to merge ;)

rafaelcaricio commented Jun 4, 2021

View reviewed changes

lvgl/src/display.rs Outdated Show resolved Hide resolved

rafaelcaricio commented Jun 4, 2021

View reviewed changes

rafaelcaricio commented Jun 8, 2021

View reviewed changes

rafaelcaricio mentioned this pull request Mar 2, 2023

Integrate bindings to lv_drivers into lvgl-sys #64

Merged

10 tasks

nia-e force-pushed the api-review branch 2 times, most recently from fbfd868 to 14054a3 Compare March 2, 2023 11:44

nia-e self-assigned this Mar 2, 2023

nia-e mentioned this pull request Mar 2, 2023

Tracking issue for version bump to 0.6 #65

Closed

10 tasks

rafaelcaricio and others added 2 commits March 2, 2023 20:23

update examples & extend functionality

abaf8e1

nia-e force-pushed the api-review branch from 620a17e to abaf8e1 Compare March 2, 2023 19:24

nia-e marked this pull request as ready for review March 2, 2023 19:25

nia-e approved these changes Mar 2, 2023

View reviewed changes

nia-e self-requested a review March 2, 2023 19:57

nia-e approved these changes Mar 2, 2023

View reviewed changes

documentation and deduplication

184d4f1

get rid of warnings

nia-e force-pushed the api-review branch from e186af9 to 184d4f1 Compare March 2, 2023 22:07

nia-e mentioned this pull request Mar 2, 2023

Write docs for the public interface #42

Closed

nia-e added the enhancement New feature or request label Mar 2, 2023

nia-e merged commit 0cb1ab4 into master Mar 2, 2023

rafaelcaricio deleted the api-review branch March 2, 2023 23:23

nia-e mentioned this pull request Mar 8, 2023

Fix regression in font support #70

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust bindings API review #51

Rust bindings API review #51

rafaelcaricio commented Jun 4, 2021

rafaelcaricio Jun 4, 2021

rafaelcaricio Jun 4, 2021

rafaelcaricio Jun 4, 2021 •

edited

Loading

rafaelcaricio Jun 4, 2021

rafaelcaricio Jun 4, 2021

rafaelcaricio Jun 4, 2021

rafaelcaricio Jun 8, 2021

rafaelcaricio Jun 8, 2021

rafaelcaricio Jun 8, 2021 •

edited

Loading

nia-e commented Mar 2, 2023

rafaelcaricio commented Mar 2, 2023

nia-e commented Mar 2, 2023

rafaelcaricio commented Mar 2, 2023

nia-e commented Mar 2, 2023

nia-e commented Mar 2, 2023 •

edited

Loading

rafaelcaricio commented Mar 2, 2023

nia-e commented Mar 2, 2023

nia-e commented Mar 2, 2023 •

edited

Loading

nia-e commented Mar 2, 2023 •

edited

Loading

rafaelcaricio commented Mar 2, 2023

Rust bindings API review #51

Rust bindings API review #51

Conversation

rafaelcaricio commented Jun 4, 2021

rafaelcaricio Jun 4, 2021

Choose a reason for hiding this comment

rafaelcaricio Jun 4, 2021

Choose a reason for hiding this comment

rafaelcaricio Jun 4, 2021 • edited Loading

Choose a reason for hiding this comment

rafaelcaricio Jun 4, 2021

Choose a reason for hiding this comment

rafaelcaricio Jun 4, 2021

Choose a reason for hiding this comment

rafaelcaricio Jun 4, 2021

Choose a reason for hiding this comment

rafaelcaricio Jun 8, 2021

Choose a reason for hiding this comment

rafaelcaricio Jun 8, 2021

Choose a reason for hiding this comment

rafaelcaricio Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

nia-e commented Mar 2, 2023

rafaelcaricio commented Mar 2, 2023

nia-e commented Mar 2, 2023

rafaelcaricio commented Mar 2, 2023

nia-e commented Mar 2, 2023

nia-e commented Mar 2, 2023 • edited Loading

rafaelcaricio commented Mar 2, 2023

nia-e commented Mar 2, 2023

nia-e commented Mar 2, 2023 • edited Loading

nia-e commented Mar 2, 2023 • edited Loading

rafaelcaricio commented Mar 2, 2023

rafaelcaricio Jun 4, 2021 •

edited

Loading

rafaelcaricio Jun 8, 2021 •

edited

Loading

nia-e commented Mar 2, 2023 •

edited

Loading

nia-e commented Mar 2, 2023 •

edited

Loading

nia-e commented Mar 2, 2023 •

edited

Loading