Skip to content

Add "Enable" button to all CF (#3024) - #3038

Closed
eshifri wants to merge 15 commits into
EdgeTX:mainfrom
eshifri:eshifri/enable_all
Closed

Add "Enable" button to all CF (#3024)#3038
eshifri wants to merge 15 commits into
EdgeTX:mainfrom
eshifri:eshifri/enable_all

Conversation

@eshifri

@eshifri eshifri commented Jan 15, 2023

Copy link
Copy Markdown
Contributor

This PR adds "Enable" switch to all CF/GF for all targets.
By default newly created function is enabled.
Disabled functions are shown with different color.

Companion is supported as well.

Fixes #3024

Summary of changes: add "enable" switch unconditionally, preserve switch status when the function is changed, set special color for disabled function. Split "active" filed filed on the CF structure into "active" and "repeat".

@philmoz

philmoz commented Jan 15, 2023

Copy link
Copy Markdown
Collaborator

Correct me if I'm wrong; but doesn't this also require a change to the internal data storage for custom functions?

AFAIK the same 8 bit property ('active') in the CustomFunctionData struct is used to store both the 'Active' and 'Repeat' values. So at the moment a CF can have one or the other; but not both.

@eshifri

eshifri commented Jan 15, 2023

Copy link
Copy Markdown
Contributor Author

You are right! I have completely missed it. :-(

@eshifri
eshifri marked this pull request as draft January 15, 2023 02:49
@eshifri
eshifri marked this pull request as ready for review January 17, 2023 02:10
@elecpower

Copy link
Copy Markdown
Collaborator

From testing so far...

  1. Companion is not writing and reading yml for the repeat value. On write always stores as 1x and on read interprets any value as 1x.

  2. Companion table headings need tidy up

Screenshot from 2023-01-19 06-09-18

Screenshot from 2023-01-19 06-09-55

Screenshot from 2023-01-19 07-11-55

@elecpower

Copy link
Copy Markdown
Collaborator

Testing continued...

Companion does not set to enabled by default on new function

@eshifri

eshifri commented Jan 18, 2023

Copy link
Copy Markdown
Contributor Author

Thank you for testing! Will look into it.

@pfeerick
pfeerick marked this pull request as draft January 19, 2023 02:11
@elecpower

Copy link
Copy Markdown
Collaborator

@eshifri not sure if you are finished with fixing read/write but just in case Companion is now reading/writing repeat but not TX16S libsim as always 1s on startup

@eshifri

eshifri commented Jan 19, 2023

Copy link
Copy Markdown
Contributor Author

@elecpower Thank you again! I'm almost sure it worked for me last night. :-( Are going into simulator from companion or starting it it as standalone?

@elecpower

Copy link
Copy Markdown
Collaborator

@eshifri both methods. Haptic is the function I have been testing.
The Companion log shows what it wrote to the temp directory before the radio had a chance to overwrite. So if this is as expected then radio side.

Comment thread companion/src/firmwares/customfunctiondata.cpp Outdated
Comment thread companion/src/modeledit/customfunctions.cpp
Comment thread companion/src/modeledit/customfunctions.cpp
@elecpower

Copy link
Copy Markdown
Collaborator

Tested on TX16S libsim and appears all the functions are being interpreted by the radio correctly.

Side point: currently in the CF grid (this maybe fixed in ui PRs) the enabled button does not show in single line CFs.

@eshifri
eshifri marked this pull request as ready for review January 21, 2023 21:56
@pfeerick pfeerick self-assigned this Jan 27, 2023
@pfeerick pfeerick added the enhancement ✨ New feature or request label Jan 27, 2023
@pfeerick pfeerick modified the milestones: 2.8.1, 2.9 Jan 27, 2023
@elecpower

Copy link
Copy Markdown
Collaborator

@eshifri still some Companion things to do:

  • column headings
  • enabled on add

Do you need any assistance?

@elecpower

Copy link
Copy Markdown
Collaborator

Latest testing

  • add new function enabled defaults to ticked- pass
  • without doing anything else change action and enabled changes from ticked to unticked - fail - expected behaviour is for enabled to remain unchanged
  • column widths for repeat and enabled are somewhat random - pass as minor - the fix for this can take some trial and error effort and even then not always achieves the ideal result
  • with the addition of the Enabled column heading the checkbox label 'ON' is somewhat in conflict when the checkbox is unticked - suggest removing checkbox label

Screenshot from 2023-01-30 07-06-04

@eshifri

eshifri commented Jan 29, 2023

Copy link
Copy Markdown
Contributor Author

@elecpower Thank you! I cannot reproduce your second item: for me all changes are saved.

@elecpower

Copy link
Copy Markdown
Collaborator

@elecpower Thank you! I cannot reproduce your second item: for me all changes are saved.

It depends on the before and after action. Steps to reproduce:

  • set to Haptic and enabled to ticked
  • change to Set TMR1 and notice enable changes to unticked

refer void CustomFunctionsPanel::functionEdited()

Fix "disabled" when swicthing functions.
@elecpower

elecpower commented Feb 5, 2023

Copy link
Copy Markdown
Collaborator

@eshifri thinking further about the change of action maybe since enabled is the proposed default then changing an action should also change to the default of enabled rather than keeping old value? The current bug is that it is still applying the old default of disabled.

@eshifri

eshifri commented Feb 5, 2023

Copy link
Copy Markdown
Contributor Author

Isn't it what happens? At least in the simulator?
I have realised it is not the case on the radio. I will change it.

@elecpower

Copy link
Copy Markdown
Collaborator

@eshifri retested and same fix required for B&W radios
Companion is working as expected (I must have been using the branch one commit behind the remove ON)

@elecpower

Copy link
Copy Markdown
Collaborator

@eshifri retested X9D+ libsim and works as expected
So its a tick from me

@eshifri

eshifri commented Feb 6, 2023

Copy link
Copy Markdown
Contributor Author

@elecpower Thank you for investing your time into this! Your help is much appreciated.


default:
add_comma = false;
//add_comma = false;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove this

Comment on lines +1619 to +1626
if (add_comma) {
// ","
if (!wf(opaque, ",", 1)) return false;
}

// "0/1"
if (!wf(opaque, CFN_ACTIVE(cfn) ? "1" : "0", 1)) return false;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please fix the indentation

Comment on lines 1642 to 1643
if (!wf(opaque, "\"", 1)) return false;
return true;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No part of this PR, but this type of formatting should be omitted. The risk of confusing the next person is too high.

@gagarinlg

Copy link
Copy Markdown
Member

I generally like that. It reduces complexity by having all SFs behave the same.

@pfeerick pfeerick added the needs: rebase A git rebase on top of the latest destination branch version is required label May 16, 2023
@pfeerick

Copy link
Copy Markdown
Member

Squash rebased in #3601

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement ✨ New feature or request needs: rebase A git rebase on top of the latest destination branch version is required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add missing Enable switch to SF/GF functions

5 participants