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(color): Display current stick position on CurveEdit screen #2580

Merged
merged 7 commits into from
Dec 6, 2022

Conversation

eshifri
Copy link
Contributor

@eshifri eshifri commented Oct 16, 2022

Fixes #2508

This PR adds functionality to display stick position on the edit curve screen.
Functionality is similar to OTX and ETX B/W: if you enter CurveEdit from input or mixer the stick position is shown; if you go to this screen from CURVES tab - no cursor is shown on the curve.
(It is possible to keep "last used stick", but it does not look right to me.)

@raphaelcoeffic
Copy link
Member

It is possible to keep "last used stick", but it does not look right to me

You mean using auto-source? The solution implemented here looks quite cumbersome to me. It assumes the source of the last edited item should be used. What if I have not edited anything?

@eshifri
Copy link
Contributor Author

eshifri commented Oct 16, 2022

As description says, this works only if you enter the EditCurve from edit input or edit mixer.
If the EditCurve is entered from CURVES, the currentInput is set to zero and no cursor is shown.
What I have meant by "It is possible to keep last used stick", is we can remove this set to zero and use the sticjk from the previous edit. If such is not available - still no cursor.
This does not look as a right behavior to me, thus there is no cursor if we enter the curve edit from the CURVES.
This is the same behavior we currently have for B/W radios and OTX implemented for color lcd radios in 2.3.14.

@Eldenroot
Copy link
Contributor

I believe this is the first step just to get the functionality for color radios as well. The limitation now will be same for bw and color radios.

Anyway this should be extended and cover all places in GUI. Otherwise it could be confusing... I am not asking in this PRs, but in future... if @eshifri can work on this one :)

Thank you!

@pfeerick
Copy link
Member

pfeerick commented Oct 17, 2022

The solution implemented here looks quite cumbersome to me. It assumes the source of the last edited item should be used. What if I have not edited anything?

This is at is a valid assumption to make, since the premise is the same as current B&W behaviour - if you access the curves screen via input or mixes, then the configured source for the curve input should be used. However, if you enter via the curves screen (or via an input with no configured source (?!)), then yes, there is no source, and thus the next level - an option that lets you pick a source, is valid.

@raphaelcoeffic
Copy link
Member

@pfeerick fine with me, then I'll leave it in your skilled hands and probably make a further PR then to add auto-source.

@pfeerick pfeerick added this to the 2.9 milestone Oct 17, 2022
@pfeerick pfeerick added enhancement ✨ New feature or request color Related generally to color LCD radios labels Oct 17, 2022
@eshifri
Copy link
Contributor Author

eshifri commented Oct 17, 2022

This commit adds automatic source selection.
Now the source is locked if the CurveEdit is entered from inputs or mixers and uses auto source selection if curve editor is entered from CURVES.

@pfeerick pfeerick self-assigned this Nov 29, 2022
@pfeerick pfeerick force-pushed the eshifri/curve_cursor branch from 33b6a59 to 1e5909a Compare December 6, 2022 01:57
@pfeerick
Copy link
Member

pfeerick commented Dec 6, 2022

Seems to be working perfectly on TX16S - uses configured source if curve editor accessed via inputs or mixes, otherwise uses whichever source is moved.

I have found an edge case where it stops working - if you go into the curve editor via inputs or mixes, graph moves when you move the stick. If you then go into the curve editor directly via the curves tab, autosource doesn't work. But it does if you go in the other order.

In other words, accessing the curves via inputs/mixes breaks autosource on the curve editor tab.

@pfeerick
Copy link
Member

pfeerick commented Dec 6, 2022

This seemed to have fixed it... I think it was still locked from going in via the inputs/mixes screens?

diff --git a/radio/src/gui/colorlcd/curveedit.cpp b/radio/src/gui/colorlcd/curveedit.cpp
index a6afc9115..29418c841 100644
--- a/radio/src/gui/colorlcd/curveedit.cpp
+++ b/radio/src/gui/colorlcd/curveedit.cpp
@@ -127,6 +127,8 @@ void CurveEdit::SetCurrentSource(uint32_t source)
   CurveEdit::currentSource = source;
   if (source) 
     lockSource = true;
+  else
+    lockSource = false;
 }
 
 mixsrc_t CurveEdit::currentSource = 0;

@pfeerick pfeerick changed the title Display current stick position on the CurveEdit screen (fix #2508) feat(color): Display current stick position on the CurveEdit screen Dec 6, 2022
@pfeerick pfeerick changed the title feat(color): Display current stick position on the CurveEdit screen feat(color): Display current stick position on CurveEdit screen Dec 6, 2022
@eshifri
Copy link
Contributor Author

eshifri commented Dec 6, 2022

Yes, I think this is the proper fix.

@eshifri
Copy link
Contributor Author

eshifri commented Dec 6, 2022

Many thanks to @pfeerick for finding the problem and the solution!

@pfeerick
Copy link
Member

pfeerick commented Dec 6, 2022

Great! And you're welcome! :)

@pfeerick pfeerick force-pushed the eshifri/curve_cursor branch from 27e7c90 to 05911e2 Compare December 6, 2022 09:42
@pfeerick pfeerick merged commit 1fe9de6 into EdgeTX:main Dec 6, 2022
@Alex6892
Copy link

Alex6892 commented Dec 7, 2022

Большой! Добро пожаловать! :)

Thanks for the work done, How can I use it, there is a ready-made nightly firmware for RM TX16S?

@pfeerick
Copy link
Member

pfeerick commented Dec 7, 2022

Once a PR is merged, it's in the next nightly build ;) https://github.com/EdgeTX/edgetx/releases/tag/nightly

@Alex6892
Copy link

Alex6892 commented Dec 7, 2022

Как только PR объединен, он находится в следующей ночной сборке;) https://github.com/EdgeTX/edgetx/releases/tag/nightly

Thank you very much, I will test.

@Alex6892
Copy link

Как только PR объединен, он находится в следующей ночной сборке;) https://github.com/EdgeTX/edgetx/releases/tag/nightly

Thank you very much, I will test.

Hello.
I found a cursor movement issue in the Curve menu.
It occurs on an ascending-descending curve and only if the curve is used in the "inputs" menu.
Please watch the video, I tried to point out the problem in detail.
Thank you for your attention and your work.

https://disk.yandex.ru/i/6Pnn4Vq4n3D7Bw

@eshifri
Copy link
Contributor Author

eshifri commented Dec 18, 2022

I can reproduce it on the simulator.
And it happens for both Curves and Mixes.
The underlying problem is the source jumps momentarily from 4 to 87 for Rudder and from 1 to 90 for Ailerons.
And only if the curve is used by the corresponding input.

If the curve is used by Rudder source detector occasionally returns 87 instead of 4 when the stick is near zero crossing.
For Ailerons it jumps from 1 to 90.
It happens only when the curve has extremum.
For relatively flat curve source 90 is detected on the flat part of the curve.

So I guess sometimes it is a stick - sometimes it is an Input?
Actually, with such curve I see the same jumps od source between stick and Input in mixes source selection menu.

mha1 pushed a commit to mha1/edgetx that referenced this pull request Dec 22, 2022
…TX#2580)

* Display current stick position on the CurveEdit screen

* auto source selection

* Display current stick position on the CurveEdit screen

* auto source selection

* unlock source when entering directly
 from model menu.

* Whitespace

* Git EoF newline

Co-authored-by: Peter Feerick <peter.feerick@gmail.com>
@eshifri eshifri deleted the eshifri/curve_cursor branch January 1, 2023 20:50
@pfeerick pfeerick mentioned this pull request Jan 24, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
color Related generally to color LCD radios enhancement ✨ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stick position indicator on the curve graph setting.
5 participants