edgetx icon indicating copy to clipboard operation
edgetx copied to clipboard

feat(lua): add screenshot() function

Open rotorman opened this issue 1 year ago • 7 comments

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

rotorman avatar Jun 14 '24 17:06 rotorman

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

3djc avatar Jun 14 '24 18:06 3djc

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

rotorman avatar Jun 15 '24 06:06 rotorman

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.

derelict avatar Jun 17 '24 00:06 derelict

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.

rotorman avatar Jun 17 '24 06:06 rotorman

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

derelict avatar Jun 17 '24 07:06 derelict

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 avatar Jun 17 '24 09:06 pfeerick

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

pfeerick avatar Jun 21 '24 00:06 pfeerick

@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

JimB40 avatar Jul 10 '24 05:07 JimB40

@JimB40 Can you point me to the C++ example of the desired comment format? In which fork/branch do I need to check, as https://github.com/EdgeTX/edgetx/blob/b630982b20b6c1e14896f7b33a1cd0857ee5c392/radio/src/lua/api_colorlcd.cpp#L687-L702 seems in the similar vain as I used in this PR.

rotorman avatar Jul 10 '24 06:07 rotorman

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.

JimB40 avatar Jul 10 '24 08:07 JimB40

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.

pfeerick avatar Jul 10 '24 09:07 pfeerick

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. https://github.com/EdgeTX/edgetx/discussions/5277

JimB40 avatar Jul 10 '24 10:07 JimB40

@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 avatar Aug 06 '24 06:08 pfeerick