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

feat(color): enhancements for Lua widget scripts. #5926

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

philmoz
Copy link
Collaborator

@philmoz philmoz commented Feb 19, 2025

Summary:

Details:
This PR allow the Lua API functions to access the widget or stand alone script that called the function. In addition this adds the telemetry queue per widget required for #5773.

The code now sends the path of the widget being loaded to the script 'create()' function (as the 3rd parameter).

The following changes have been made to the getSwitchIndex() and getSourceIndex() API functions:

  • Both functions check the name value against the default names for switches and sources as well as the user defined custom names (previously only custom names were checked if set by the user).
  • Both functions will always return the switch or source index, previously they would only return a value if the requested item was available, so could not be used on startup when the widgets are loaded.
  • The getSourceIndex() function will match names with or without the symbol string prefix. E.G. getSourceIndex(CHAR_SWITCH.."SF") and getSourceIndex("SF") will both work.

For the widget 'options' table, entries of type SWITCH and SOURCE, can set their default, min and max values from a string. This is a shortcut to using getSwitchIndex() or getSourceIndex() in the option table.
E.G. these two lines are equivalent:

  { "Switch", SWITCH, "SF"..CHAR_DOWN },
  { "Switch", SWITCH, getSwitchIndex("SF"..CHAR_DOWN) },

@philmoz philmoz added enhancement ✨ New feature or request color Related generally to color LCD radios lua-api Lua API related labels Feb 19, 2025
@philmoz philmoz added this to the 3.0 milestone Feb 19, 2025
@philmoz
Copy link
Collaborator Author

philmoz commented Feb 19, 2025

Added support for the default value of a SWITCH or SOURCE option to be a table of possible values.
The first available value is used. Completes the changes requested in #5917
E.G.:

      { "volt_sensor", SOURCE, {"cell","VFAS","RxBt","A1", "A2"} }

@wimalopaan
Copy link
Contributor

The arc graphical primitive uses functions or values for the startAngle and endAngle properties. Other graphical primitives like retangle or triangle use only values.
I started rewriting some old graphics intensitive widgets and it would be really cool if also rectangle, triangle, circle and line could use functions (or values) as well at least for their coordinates.

@philmoz
Copy link
Collaborator Author

philmoz commented Feb 22, 2025

The position of all objects can be set with the 'pos()' function.
The size of most objects can be set with the 'size()' function.
E.G:

        {type="rectangle", filled=true,
          color=getFillColor,
          size=(function() return fw, math.floor(wgt.vPercent / 100 * (fh)) end),
          pos=(function() return 0, fh - math.floor(wgt.vPercent / 100 * (fh)) end)},

@wimalopaan
Copy link
Contributor

wimalopaan commented Feb 22, 2025

The position of all objects can be set with the 'pos()' function.
The size of most objects can be set with the 'size()' function.

Perfect! Sorry, I missed that in the docu ....

Does pos() really move relative to the window or relative to the parent? What's the behaviour if both xand y are present together with pos()?

One problem with triangle remains: here size() and pos() are not sufficient, I would need also rotate(). But I think it would be much simpler to let the property pts be also a function.

@philmoz
Copy link
Collaborator Author

philmoz commented Feb 22, 2025

Does pos() really move relative to the window or relative to the parent? What's the behaviour if both xand y are present together with pos()?

Parent. I've updated the gitbook docs.

One problem with triangle remains: here size() and pos() are not sufficient, I would need also rotate(). But I think it would be much simpler to let the property pts be also a function.

Triangle is complicated as Lvgl does not have a native triangle object.
The code creates a square mask and draws the triangle into it.
The 'pos()' function can move it; but 'size()' is not implemented.
Rotation would be tricky (what point do you rotate around?).
Having a function for 'pts' could be possible; but it would need to be smart and only redraw the mask when one or more points changed.

@wimalopaan
Copy link
Contributor

Rotation would be tricky (what point do you rotate around?).

Yes. Most libs have to notion of an origin of the drawing canvas. And the rotation rotates around this origin. But I don't know if that's possible for lvgl.

Having a function for 'pts' could be possible; but it would need to be smart and only redraw the mask when one or more points changed.

Mmh, this should be mostly the same as using the set() function for the pts property. But maybe I'm wrong here ...

@philmoz
Copy link
Collaborator Author

philmoz commented Feb 22, 2025

Having a function for 'pts' could be possible; but it would need to be smart and only redraw the mask when one or more points changed.

Mmh, this should be mostly the same as using the set() function for the pts property. But maybe I'm wrong here ...

I am assuming you would only call 'set()' when something changes so it does not check.

@wimalopaan
Copy link
Contributor

Having a function for 'pts' could be possible; but it would need to be smart and only redraw the mask when one or more points changed.

Mmh, this should be mostly the same as using the set() function for the pts property. But maybe I'm wrong here ...

I am assuming you would only call 'set()' when something changes so it does not check.

Ok, then maybe saving the triangle-objects ref and using set() would be best?

@wimalopaan
Copy link
Contributor

Perfect!
Unfortunately it will take a week until I can do some real testing.

@wimalopaan
Copy link
Contributor

I noticed a new (!) problem with the lvgl UI controls. Actually I have no minimal verifying example, so I have to describe it in words:

  1. create a page with a slider and use a get function and put a print() debug message in it.
  2. overwrite the first page with another page that does not contain the slider created under 1)

Now see, that the get function of 1) is still called.

I don't think that this is intentional, because the slider of 1) should be deleted. I think this behaviour changed some time ago.

Unfortunately I have no small(!) example widget ... will need some time.

@philmoz
Copy link
Collaborator Author

philmoz commented Feb 27, 2025

I noticed a new (!) problem with the lvgl UI controls. Actually I have no minimal verifying example, so I have to describe it in words:

It may be a few weeks before I can look at this.

@wimalopaan
Copy link
Contributor

I noticed a new (!) problem with the lvgl UI controls. Actually I have no minimal verifying example, so I have to describe it in words:

It may be a few weeks before I can look at this.

Ok, take your time ;-) Then I have enough time to create a minimal verifying example ...

@wimalopaan
Copy link
Contributor

In the process creating an example i noticed that the problem I reported in #5926 (comment) was my fault! I forgot to use lvgl.clear() to delete the no longer used UI objects.
Sorry for the hassle.

@wimalopaan
Copy link
Contributor

I do not really understand, what the meaning of lvgl.isAppMode() is: the code shows that this is true iff the widget was installed in appMode and it is switched to fullscreen. I think this does not make sense: the widget should be able to detect if it was installed in appMode regardless if it runs fullscreen or not.

This would be my understanding, but maybe I'm totally wrong here.

@philmoz
Copy link
Collaborator Author

philmoz commented Mar 3, 2025

isAppMode() function can be used when building the widget layout to determine if user input controls can be used for an app mode widget.

But looking at the code, we currently have the states show below.

Layout/State isFullScreen() isAppMode()
Normal layout, not full screen false false
Normal layout, full screen true false
App Mode layout, no user input to widget false false
App Mode layout, user input sent to widget true true

So isAppMode() is somewhat redundant as isFullScreen() would also work.

I see two options:

  • Remove isAppMode() or,
  • Change it to return true always when the widget is loaded into the App Mode layout.

Thoughts?

@wimalopaan
Copy link
Contributor

I would go for:

  • Change it to return true always when the widget is loaded into the App Mode layout.

@offer-shmuely
Copy link
Contributor

In my opinion, the app-mode is NOT the layout,
IT is the ability for user to control the widget,
It is the time when the widget moved from read-only mode, to interactive, i.e. the touch controls is actually send to the refresh function

So i suggest to change the AppMode() to reflect that.
It should return true on #2 &#4

  • Normal layout, full screen
  • App Mode layout, user input sent to widget

@philmoz
Copy link
Collaborator Author

philmoz commented Mar 4, 2025

In my opinion, the app-mode is NOT the layout, IT is the ability for user to control the widget, It is the time when the widget moved from read-only mode, to interactive, i.e. the touch controls is actually send to the refresh function

So i suggest to change the AppMode() to reflect that. It should return true on #2 &#4

  • Normal layout, full screen
  • App Mode layout, user input sent to widget

That would make it redundant as it becomes identical to isFullScreen() (which was traditionally used for this purpose).

@pfeerick
Copy link
Member

pfeerick commented Mar 4, 2025

Please correct me if this is wrong, but does "Normal layout, full screen" also not necessarily mean touch input? It seems to me that 3 & 4 should both be true for isAppMode(), and you know the widget has touch input focus if isAppMode() and isFullScreen() ... resulting in you being able to identify any of the listed four states via the correct combination of isAppMode() and isFullScreen() tests...

Layout/State isFullScreen() isAppMode()
Normal layout, not full screen false false
Normal layout, full screen true false
App Mode layout, no user input to widget false true
App Mode layout, user input sent to widget true true

@offer-shmuely
Copy link
Contributor

Ok, so fullscreen represent the ability to use touch (bad name, but it is what it is)
What can be the use for appmode()?
I can not think yet of any usecase

@wimalopaan
Copy link
Contributor

The API has the two functions pos() and size(). This is really good.

I came across an use case where it would be best to have a function e.g. dim() to return all 4 values: x, y, w, h. Clearly, I can use both functions to achieve the same goal. But if e.g. I need to call a compute intensive function both in pos and size to compute say y and h. This could be reduced to only one call in such an function dim().
One alternative would be to give the functions pos() and size() a shared state. But I would like to avoid that.

@philmoz
Copy link
Collaborator Author

philmoz commented Mar 6, 2025

The refresh function is called first. You could do the calculations there and store the result in local variables to be used in pos and size functions.

@philmoz philmoz force-pushed the philmoz/lua_widget_enhancments branch from aa423fc to 97c2625 Compare March 6, 2025 22:01
@wimalopaan
Copy link
Contributor

The values property of the UI class choice is a value of type table. It would be cool, if values may also contain a function returning a table of entries.

@offer-shmuely
Copy link
Contributor

Regarding the widget name on create function,
I thought it's enough, but I was wrong.
One of its uses is for name of the widget.
But it is discovered earlier then creat()

I wonder what is the relation between the folder name of the widget, and the name passed int the "return {name="the widget name", "
Should they be the same? I remember that in the past it was, something with the configurator
Can we live without the defining the name?
Can it user the widget folder name?

@wimalopaan
Copy link
Contributor

wimalopaan commented Mar 8, 2025

Having the directory passed as third argument to create() is very handy: if the widget comprises of many files there is no need for any assumptions of a fixed directory name. Thats perfect.

The name in the return table of the widget chunk is informative for the widget selection and therefore the length is limited. I usually use a longname for all other display purposes of that widget (e.g. the header line), and this name should have some clear relationship to the short name name.

@wimalopaan
Copy link
Contributor

I encountered a problem with the function lvgl.isAppMode(): this function returns true if a widget is installed in appMode. But this does not work directly after the widget is loaded when the simulator (radio) starts up. In this case the function lvgl.isAppMode() returns false. So, the widget works in the wrong mode after startup.

@wimalopaan
Copy link
Contributor

Ok, so fullscreen represent the ability to use touch (bad name, but it is what it is) What can be the use for appmode()? I can not think yet of any usecase

In my understanding appMode was created to have a widget that seemlessly integrates into the UI of EdgeTx. This would require to send touch and key events to the widget, even if not in fullScreen mode.

Regarding to key events, this should have some exceptions: the page keys should still work to switch the screen.

If a widget wants also to use the page keys, it should be able to do that (maybe an entry in the return-table of the widget chunk or a possiblity to install a page key event handler).
This leads to the problem how to switch the screen if the widget uses the page keys: maybe a long press on the page keys should allways switch the screen.
Maybe it would also have a lvgl function the switch the screen, e.g. lvgl.nextScreen() and lvgl.prevScreen().

I hope that makes any sense ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
color Related generally to color LCD radios enhancement ✨ New feature or request lua-api Lua API related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants