Make LVGL thread-safe

[HotelCalifornia]

Expected Behavior:

LVGL functions should be thread-safe.

Actual Behavior:

Per lvgl/lvgl#544, we need to mutex guard the call to lv_task_handler, and then make the same mutex available to guard the calls to the lv_* API.

Steps to reproduce:

This hasn't yet been the obvious cause of any observed issues.

System information:

Platform: V5
PROS Kernel Version: any

Additional Information

As I'm thinking about it, it seems the best solution will be to add accessor functions:

lv_mutex_take(void);
lv_mutex_give(void);

My first thought was to just have a single function that would just return the mutex itself, but I have since realized that would potentially allow users to delete it, which would not be desirable. As such, I think the above solution is the way to go.

Screenshots/Output Dumps/Stack Traces

N/A

[HotelCalifornia]

@edjubuh said:

A recursive mutex would be fine here.

A PR to LVGL might not be a terrible idea, allowing for

#define LV_LOCK_TAKE (whatever)
#define LV_LOCK_GIVE (whatever)

and take/give at every entry/exit point of every function.

[lights0123]

Theoretically, shouldn't this only occur if someone yields (such as using delay) to FreeRTOS within an LVGL call? As far as I know, the FreeRTOS configuration used in PROS only context switches with explicit yields, not based on a timer. So the only times thread safety would be an issue would be:

• Someone extends an LVGL object and causes a FreeRTOS yield such as using delay or taskYIELD directly in between LVGL modifications
• Someone (and this is more likely) delays within an LVGL callback, depending on when LVGL invokes callbacks (does it do them all at the end/beginning, or call them in between drawing?)

[HotelCalifornia]

only context switches with explicit yields, not based on a timer.

The configuration we use includes priority-based preemptive scheduling, in which at any given time the currently running task is the highest priority task that isn't blocked. When there are multiple tasks running at the same priority, they are round-robined such that each gets an equal amount of processing time.

As I mentioned in the original comment, this has yet to be the obvious cause of any issues, which means in the end you may be right. My original intent though was just to be safer than sorry (especially as the porting guide now recommends this course of action).

[lights0123]

@HotelCalifornia I completely agree—I was just trying to clarify the situation. I've been working on PROS & LVGL bindings for Rust, where thread safety is a big deal (which is why I came across this thread). I'd love to see more thread-safety functionality.

[HotelCalifornia]

That said, do you have any opinions regarding how we might go about implementing this? We discussed a couple options above but each have their problems

[lights0123]

It looks like LVGL is not interested in adding their own synchronization calls within their funcions. This leaves either requiring the user to manually lock and unlock with the originally proposed solution, or generating our own API that builds on top of LVGL. However, it's probably not worth the effort to rename every LVGL function symbol, and write our own wrapper over every function (unless you hack something together that uses e.g. clang's parser to do it for you).

Realistically, creating our own functions and requiring the user to call them seems to be the best option. I wouldn't prefix them with lv_, because that makes it look like a call provided by LVGL, and the user would probably look in the wrong docs (especially because the LVGL docs kinda suck). However, we need to make this call well known, because people might just skip the PROS docs and skip straight to the LVGL ones.

[HotelCalifornia]

Yeah, I'd looked into function decorators and such in C and determined that there's not really a good way to wrap all the functions without more work than I'm really willing to put in. I also hesitate to throw my lot in with duplicating the lvgl API, as that's just more stuff to maintain. Your mention of the clang parser to generate such an API is intriguing, but I also worry about confusing users.

Another possible solution would be to just leave the lvgl C functions as they are, and finally implement a proper C++ abstraction layer that handles thread-safety. I've done a small amount of preliminary work on such a layer, and I believe there is at least one other member of the community who was working on something similar.

Beyond that, I guess I find I'm leaning back toward a pros_lv_mutex_[un]lock solution. That way, the people who care can use it, and the people who don't won't. Still not really convinced either way though

[lights0123]

I would probably go with both a C++ abstraction and user mutex. The part of LVGL that I don't like the most aside from the lack of detailed documentation is how destructive lv_obj_del is. For example:

• Deleting a parent automatically deletes all of its children. If you're writing a C++ abstraction, if a parent is removed, then that object automatically becomes invalid. This will be awkward to deal with, as an object can point to freed memory without explicitly changing it. I'd like if LVGL added a feature to add children to a "shadow" parent, which does not exist on screen, so that deleted children can still have valid pointers.
• (less important) Some things can just not be deleted—for example, lv_obj_deling an lv_page that was created with lv_tabview_add_tab will lead to a null pointer exception.

[nickmertin]

It would be nice if this included a std::lock_guard-compatible API, which would allow more idiomatic C++ usage.

[HotelCalifornia]

It would be nice if this included a std::lock_guard-compatible API, which would allow more idiomatic C++ usage.

we're hoping to get all of std::thread working for "free" on our pthread branch

[Boldie]

I think it would not be favourable to make the lib thread save, because then you will run in other trouble. For e.g. then you will ad a widget or something and then after releasing the lock it will start redrawing the stuff also if you are not finished with changing the stuff on block. In my opinion this should be left to the user to make it thread save, like other environments will do (for e.g. QT is also not thread save, you have to follow the same conventions as here).

But in the examples the user shall use more callbacks for this type of usage. For e.g. passing a change widget function to the task system with a "future" is a good way to synchronize stuff without the needs for lock but with the benefit to fuse the changes to the objects itself. So for C++ users, this may be a simple lambda if one will write a good adaption.

EDIT: I also saw some code which was added to make it save, but it does not make it save. In the lv_task_handler() there is a section

    /*Avoid concurrent running of the task handler*/
    static bool task_handler_mutex = false;
    if(task_handler_mutex) return;
    /// -> here the fuck happens
    task_handler_mutex = true;

which is not save on a preemptive system, I added the line where the non good stuff may happen, if a schedule will happen there. I saw a lot of code of this kind which is bad. So this code shall better be removed since it is not misleading and will not work every time. Here one needs a TestAndSet strategy to be save.

[WillXuCodes]

Will naturally be done by simplified screen API since it's coupled to the screen mutex, closing to reduce clutter.