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(lua): add screenshot() function #5181

Merged
merged 2 commits into from
Aug 20, 2024
Merged

Conversation

rotorman
Copy link
Member

Adds screenshot() function to Lua.
Here (positive) feedback about this PR being tested on EdgeTX Discord: https://discord.com/channels/839849772864503828/842693696629899274/1251223608173264979

@rotorman rotorman added enhancement ✨ New feature or request color Related generally to color LCD radios lua-api Lua API related B&W Related generally to black and white LCD radios lua labels Jun 14, 2024
@3djc
Copy link
Collaborator

3djc commented Jun 14, 2024

Nothing against this PR, my only concern is that screenshots take a very severe toll on the radio, I have seen radio completely frozen by misuse of screenshots. Maybe we should make sure that we don't recall one when previous hasn't completed at least

@rotorman
Copy link
Member Author

Good point. Any tips/ideas how to solve that?

@pfeerick pfeerick removed color Related generally to color LCD radios B&W Related generally to black and white LCD radios labels Jun 16, 2024
@pfeerick pfeerick changed the title feat(lua): exposes screenshot functionality to Lua feat(lua): add screenshot() function Jun 16, 2024
@pfeerick pfeerick added this to the 2.11 milestone Jun 16, 2024
@derelict
Copy link

Nothing against this PR, my only concern is that screenshots take a very severe toll on the radio, I have seen radio completely frozen by misuse of screenshots. Maybe we should make sure that we don't recall one when previous hasn't completed at least

it would be great to be able to trigger a screenshot from lua ... as this would be something i'd like to use in my lua script (take a screenshot of the flight statistics after flight) ... but of course we have to make sure a lua script can't go nuts with taking screenshots.

@rotorman
Copy link
Member Author

I would suggest the limitation to trigger a new screenshot only if currently no screenshot is being taken to go into C++ function writeScreenshot(); and in that sense be a separate PR from this Lua API addition.

@derelict
Copy link

Or rate limit the c++ function to 1-2 seconds per Screenshot ... This may be beneficial for other "problems" too

@pfeerick
Copy link
Member

pfeerick commented Jun 17, 2024

Sounds reasonable. Any further rate limiting can be considered when overhauling Lua APIs... i.e. if it makes sense to put rate limits/other safeguards on Lua functions on top of any rate limiting happening in the radio code.

@pfeerick
Copy link
Member

@JimB40 This would be a perfect time for you to comment re: API documentation ;)

@pfeerick pfeerick mentioned this pull request Jul 2, 2024
1 task
@JimB40
Copy link
Collaborator

JimB40 commented Jul 10, 2024

@rotorman there is new format for ETX luadoc (work in progress), so every new API funcion shoud have entry before merged.
API func list:
https://luadoc.edgetx.org/v/next-snapshot/lua-api-reference

You can place it in "Display LCD" category or "System" category as it has no lcd.* prefix

Here is sample of newly formated API function page:
https://luadoc.edgetx.org/v/next-snapshot/lua-api-reference/display-lcd/drawbitmap

  • Func name and desctiption
  • Syntax
  • Parameters in table stated type and if required
  • Return value
  • Notes: here can go information that screenshot function can be run only cerain period time not to hand OS
  • Availability (BW/Grayscale/Color)
  • API status
  • Examples

@rotorman
Copy link
Member Author

@JimB40 Can you point me to the C++ example of the desired comment format? In which fork/branch do I need to check, as

/*luadoc
@function lcd.drawBitmap(bitmap, x, y [, scale])
Displays a bitmap at (x,y)
@param bitmap (pointer) point to a bitmap previously opened with Bitmap.open()
@param x,y (positive numbers) starting coordinates
@param scale (positive numbers) scale in %, 50 divides size by two, 100 is unchanged, 200 doubles size.
Omitting scale draws image in 1:1 scale and is faster than specifying 100 for scale.
@notice Only available on Horus
@status current Introduced in 2.2.0
*/
seems in the similar vain as I used in this PR.

@JimB40
Copy link
Collaborator

JimB40 commented Jul 10, 2024

There is none.
Preferred is to update luadoc 2.11 as automated script code parser is 'out of order'

If we want to keep this API func information in C++ code, you may create one based on information is needed in luadoc API func template. Excluding examples of course.

@pfeerick
Copy link
Member

This is where you should probably say/discuss how much needs to actually be in the C++ code, and suggested format, @JimB40 As the one seeking change ;) Or does it make sense for it to be done in the PR, as part of the initial comment (or other) - as this is also markdown compatible.

@JimB40
Copy link
Collaborator

JimB40 commented Jul 10, 2024

This is where you should probably say/discuss how much needs to actually be in the C++ code, and suggested format, @JimB40 As the one seeking change ;) Or does it make sense for it to be done in the PR, as part of the initial comment (or other) - as this is also markdown compatible.

I'm not seeking change, I'm seeking well documented and up-to-date API, aren't you? :)
Discussion thread then.
#5277

@pfeerick
Copy link
Member

pfeerick commented Aug 6, 2024

@JimB40 So, is this correct for this PR then, given the proposal in #5277? This probably belongs in System since it is not related to drawing to the LCD. Do we have to specify all the targets if it is unconditioanlly / availble for all? No [ALL] specifier?

/*luadoc
@name screenshot

@description Takes a screenshot, which is saved to the SCREENSHOTS folder on the radio SD card.

@syntax screenshot()

@return none

@notes This command is currently not rate limited, so repeated frequent calls will slow down the UI and can even freeze the entire radio, so should be used with care. 

@target [BW]
@target [GS]
@target [COLOR]

@status current Introduced in 2.11
*/

@pfeerick pfeerick merged commit 5e95e52 into EdgeTX:main Aug 20, 2024
47 checks passed
@rotorman rotorman deleted the luascreenshot branch August 20, 2024 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ New feature or request lua lua-api Lua API related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants