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

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

Closed
wants to merge 15 commits into from

Conversation

eshifri
Copy link
Contributor

@eshifri eshifri commented Jan 15, 2023

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
Copy link
Collaborator

philmoz commented Jan 15, 2023

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
Copy link
Contributor Author

eshifri commented Jan 15, 2023

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
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
Collaborator

Testing continued...

Companion does not set to enabled by default on new function

@eshifri
Copy link
Contributor Author

eshifri commented Jan 18, 2023

Thank you for testing! Will look into it.

@pfeerick pfeerick marked this pull request as draft January 19, 2023 02:11
@elecpower
Copy link
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
Copy link
Contributor Author

eshifri commented Jan 19, 2023

@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
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.

@elecpower
Copy link
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
Collaborator

@eshifri still some Companion things to do:

  • column headings
  • enabled on add

Do you need any assistance?

@elecpower
Copy link
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
Copy link
Contributor Author

eshifri commented Jan 29, 2023

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

@elecpower
Copy link
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
Copy link
Collaborator

elecpower commented Feb 5, 2023

@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
Copy link
Contributor Author

eshifri commented Feb 5, 2023

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
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
Collaborator

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

@eshifri
Copy link
Contributor Author

eshifri commented Feb 6, 2023

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

@@ -1609,26 +1612,24 @@ static bool w_customFn(void* user, uint8_t* data, uint32_t bitoffs,
break;

default:
add_comma = false;
//add_comma = false;
Copy link
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
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
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
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
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