edgetx
edgetx copied to clipboard
EdgeTX
Special Function to set main screen (#2135) for color LCD
New custom function to allow selection of the main screen page Fixes #2135
Summary of changes: new custom function with everything around it. Works only on color LCD TXs.
@elecpower I searched for the names of the existing functions and nothing came out in the companion. Can you please pointe me to starting point on the companion side?
This is starting to look better... Just cleaned up some whitespace and added to the lua constants list... I'm was wondering atm if the yaml_datastrucs_funcs.cpp implementation is the right way to go... as this function is the only one that is #ifdef'd ... yet others like FUNC_HAPTIC aren't. But some later poking around suggests this is may partly be because some bad assumptions have been made... ie. that there will always be haptic with colorlcd, so if you turn it off... stuff breaks... so will fix that one separately as it just needs some ifdefs to fix those up. Well, compilation at least... the second part to that is that in the case of Haptic, it exists in the UI when there is no capability compiled in... 😆 ... so I think you did this much better than the current code ;)
General comment. We really need to stop ifdefing enums and use dynamic filters if trying to achieve the objective of less compile options. Companion has to replicate the enum ifdef shuffle across all variants and apply filters so that only those relevant to the current radio profile are displayed. Yaml has made the Companion import/export slightly easier since using text rather than enum value but it still it introduces conversion risk between radios especially once loaded into memory and you then change profiles.
@elecpower I searched for the names of the existing functions and nothing came out in the companion. Can you please pointe me to starting point on the companion side?
Will advise
We really need to stop ifdefing enums and use dynamic filters if trying to achieve the objective of less compile options.
Very true. But this does not apply to hardware specific stuff... i.e. colorlcd isn't going to be dynamically filtered. Nor is the presence of haptic, sound, touch, etc. That's where for a feature like this it gets a bit murky, as it is a colorlcd only feature (for now at least), but it's not hardware related. There is where I also think FUNC_DISABLE_TOUCH func is a bit naughty too, and follows COLORLCD, rather than HARDWARE_TOUCH in some places, and because of that is actually visible on some colorlcd radios without touch 🤦 Since this isn't SF isn't tied to hardware but more capability, I don't think it should be #ifdef'd per say... or at least, be done through use of isAssignableFunctionAvailable filter as Malte suggested.
Or am I wrong in that thinking? ;)
@eshifri I haven't checked all the dependencies but these should get you started companion/src/firmwares/edgetx/yaml_customfunctiondata.cpp companion/src/modeledit/customfunctions.cpp companion/src/firmwares/customfunctiondata.h companion/src/firmwares/customfunctiondata.cpp
Your lucky ETX now uses yaml as these types of changes are a nightmare in OTX.
@pfeerick Companion is as bad if not worse than the radio side with the parallel use of legacy and newer references to hardware and capabilities. When I come across them I think to myself I should fix that and then think I need to fix the bug or add the enhancement.
@pfeerick Companion is as bad if not worse than the radio side with the parallel use of legacy and newer references to hardware and capabilities. When I come across them I think to myself I should fix that and then think I need to fix the bug or add the enhancement.
Yeah, it's sort of what like what we initially said with LVGL... to do the port + redo the whole UI from scratch would basically mean no release version for a year... so that wasn't happening. Baby steps, and certainly another long term project... simplifying and making things consistent as we go. Same thing is happening at the driver level.
@eshifri my dumb questions time. Why range zero based when user sees 1 based when maintaining views? Why repeat parameter? I would have thought enabled/active since I assume it is a one off trigger. How do you handle the situation where the user deletes a view(s) after configuring a SF(s) ie could be pointing to a non-existent or wrong view?
I'll knock up a function to get the number of configured views (see commit)
@eshifri Set Main View is listed as an available function for ALL radios in Companion
@elecpower
- I'm too used to C/C++; for me numbering starts from zero. :-) It is the same on the TX side - I will change to 1-based in both locations.
- Repeat parameter allows temporary overwrite: a user can switch to any other screen for a pre-defined time.
- If a screen is deleted the existing code for switching screen takes care of that automatically: an attempt to go to non-existing screen has no effect.
Tested Companion range 1 to number of screens - passed
Tested TX16S sim yaml settings transferred - failed If you change the parameter from 1 to anything else and back to 1 the yaml is generated correctly. So appears an initialisation bug?
Tested TX16S sim range 1 to number of screens - passed even with incorrect starting value
Tested convert TX16S settings to X9D+ function not available - passed however the residual is messy since the conversion did not clean out the parameter though this is a common issue.
@eshifri seems I've partially misled you as the yaml screenshot should have been between Companion and radio sim. So whilst you have tightened up the radio side Companion still needs a fix so it generates valid yaml.
@eshifri I was intially going to use a variable to buffer the value for the display of the screen value shown by paintSpecialFunctionLine as it can't alter SF parameter data as it's defined as constant there (which makes sense as when simply painting the data, to screen, no data should be harmed/modified 😉 )... but I then though that why is it being done there at all... it should be showing what the configured value is... so changed it to that instead... so at the moment the effect is if you say had four screens, set it to the fourth, and then deleted two, the overview will say 4, and as soon as you edit the SF it will jump back into range.
Given this potential problem (as Neil pointed out earlier):
How do you handle the situation where the user deletes a view(s) after configuring a SF(s) ie could be pointing to a non-existent or wrong view?
The code as it stands will blindly attempt to switch to screen 4, but is basically ignored if out of range. So I think at the present time, this gives some idea as to why this is happening, by showing the true value that is set. I'm not sure what more we can do, as yes, you could constrain it within the function, so it switched to screen 2, etc... but then it's not true to configured value, etc. I'm not sure it's valid to do other trickery like altering SFs when the views are deleted (and I guess potentially inserted or moved in the future) either.
btw, I'm guessing the issue
Tested TX16S sim yaml settings transferred - failed If you change the parameter from 1 to anything else and back to 1 the yaml is generated correctly. So appears an initialisation bug?
is because Companion is still indexing from 0, whereas radio is now indexing from 1?
Leave the value as it is when the number of screens is changed. The code to switch screens has to do boundary checks anyways and I prefer not to change config values behind the back of the users.
I agree. But companion problem is still there. And indexing in companion is also 1-based.
Please see comment https://github.com/EdgeTX/edgetx/pull/2160#issuecomment-1198351855 for repeat param.
@eshifri sorry if I missed something about the repeat param. Is the goal to be able to basically "force" some screen, but using the repeat param having it fall back there after you've visited another screen? I have the strong feeling it is a misuse of that param, but I do not doubt it would work. The way it is now is basically: the moment the function gets active, it does switch the screen, but not continuously, so you can go to another by scrolling. If you want it to return to the screen set by function, you'll have to "re-activate" that function (probably moving the switch?). But you wanted some sort of "auto return to screen"?
I mean, don't get me wrong, we can totally re-add it ;-) I could just wipe these commits if necessary.
Another question: has anyone tested how that interacts with "Fullscreen mode"?
@eshifri sorry if I missed something about the repeat param. Is the goal to be able to basically "force" some screen, but using the repeat param having it fall back there after you've visited another screen? I have the strong feeling it is a misuse of that param, but I do not doubt it would work. The way it is now is basically: the moment the function gets active, it does switch the screen, but not continuously, so you can go to another by scrolling. If you want it to return to the screen set by function, you'll have to "re-activate" that function (probably moving the switch?). But you wanted some sort of "auto return to screen"?
I mean, don't get me wrong, we can totally re-add it ;-) I could just wipe these commits if necessary.
I think it should be re-added, it could be really useful and we could avoid another PR :)
@raphaelcoeffic You are the BOSS! :-) Yes, the use case I had in mind was: user connects the model and sees screen #1. When motors are armed, this SF allows to switch to screen #2 - the main "flying" screen. If during the flight user wants to go to screen#1 or #3 he can do it and the system will go back to the main "flight" screen and user does not need to sweep back and force. And re-arming the motors to activate the SF is not an option.
I hope @MadMonkey87 will chime in - this SF is his idea.
Another question: has anyone tested how that interacts with "Fullscreen mode"?
Looks like it does not work in full screen mode: in full screen it is impossible to change the screen. When you exit full screen functionality of this SF is restored.
Full screen being "widget full screen"? If so, if that's all that happens that sounds fine. And your use case sounds excellent... you can either set it to only trigger once, or every x seconds, giving you time to look at something else, and the screen is then reset back. And I think even script stuff so it would set a particular screen on entering the model, i.e. if you have a pre-arm screen, then an armed screen, and even an end-of-flight screen... you could make it so they are automatically switched between (with flight screen basically persistent on a delay) etc. Fancy stuff like that. It is a bit of a "misuse" of the repeat parameter, but close enough IMO.
It can be done even without scripting: pre-flight on connection, flight - on arm, post-flight - on disarm or disconnected telemetry. Or some more fency combination of events.
Maybe a poor choice of words on my part - was thinking purely LSs/SFs when I said scripting... not full blown Lua ;) ... but I really like the potential uses for such a "simple" SF! 😸