Skip to content

Rotary encoder additional invert options for B&W screens#1826

Closed
breadoven wants to merge 12 commits intoEdgeTX:mainfrom
breadoven:abo_rotary_encoder_invert_mod
Closed

Rotary encoder additional invert options for B&W screens#1826
breadoven wants to merge 12 commits intoEdgeTX:mainfrom
breadoven:abo_rotary_encoder_invert_mod

Conversation

@breadoven
Copy link
Contributor

Adds 2 additional rotary encoder invert options for B&W screens changing behaviour of menu navigation including popup menus. Not used for colour screens due to different navigation method.

Rotary Invert setting changed from Check box to Combo with OFF, ON, V-N, V-A where:

ON = vertical and horizontal inverted
V-N = vertical movement inverted (Normal), horizontal row movement behaviour unchanged (always cycles through all settings in rows)
V-A = vertical movement inverted (Alternative), horizontal row movement behaviour changed so left scroll moves selection directly down LH settings without cycling through multiple settings in rows. Movement along rows of settings only happens when right scrolling, moving up a row when scrolling beyond last setting in row.

LUA API function added providing rotary encoder inverted status.

Sim tested OK on X9D+ 2019, ZORRO, TX16S, TPRO, T18, TX12, T12. QX7 ACCESS, X10 EXPRESS. Fully tested on ZORRO radio.

Copy link
Member

@pfeerick pfeerick left a comment

Choose a reason for hiding this comment

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

This looks really good! Looking forward to trying it out :)

I guess for the moment you are relying on 1 & 0 being true and false in order for the change to the Combo box in companion still being valid for colorlcd for the supported options? I'll leave the Companion changes up to @elecpower to review. I wonder if we may need to

Can you revert the changes to radio/src/opentx.cpp since there are no actual PR related changes to the file.

Any attempt to introduce this to colorlcd will probably have to wait until later in the 2.8 cycle as I would think it would changes to libopenui, and no changes are to that will be allowed until the LVGL re-write has finally been merged into the main dev branch. Plus once that is in place, some of the new capabilities may help with things like this.

@pfeerick pfeerick linked an issue Apr 13, 2022 that may be closed by this pull request
@breadoven
Copy link
Contributor Author

I guess for the moment you are relying on 1 & 0 being true and false in order for the change to the Combo box in companion still being valid for colorlcd for the supported options? I'll leave the Companion changes up to @elecpower to review. I wonder if we may need to

Yes keeping OFF and ON as the first 2 options keeps it aligned with the colour LCD. The only drawback to this is that selecting the 2 vertical options is a bit tricky because the rotary encoder reverses when you select ON via right scroll so you have to left scroll to get to the vertical options otherwise you just go back to OFF. Confusing until you work out what it's doing but since this is a set and forget type of setting it only needs doing once.

The other thing is this change doesn't allow the horizontal movement to be reversed independently. Personally can't see much use for this given all the rotary encoders are aligned essentially vertically and you just end up with counterintuitive movements. Would require another BIT if more settings were to be added.

@pfeerick pfeerick added this to the 2.8 milestone Apr 16, 2022
@pfeerick pfeerick added B&W Related generally to black and white LCD radios enhancement ✨ New feature or request labels Apr 16, 2022
pfeerick
pfeerick previously approved these changes Apr 16, 2022
@pfeerick pfeerick self-requested a review April 16, 2022 04:35
@pfeerick pfeerick dismissed their stale review April 16, 2022 04:36

was just trying to clear change request state

@breadoven
Copy link
Contributor Author

Is this likely to get merged any time soon ? Gets a little tedious having to resolve conflicts and other changes to keep it current.

@raphaelcoeffic
Copy link
Member

Is this likely to get merged any time soon ? Gets a little tedious having to resolve conflicts and other changes to keep it current.

You should rebase instead of merging main into your branch. It makes the PR totally unreviewable, as it contains lots of unrelated changes.

@raphaelcoeffic
Copy link
Member

These should not be here:
Screenshot 2022-06-12 at 19 32 18

@raphaelcoeffic
Copy link
Member

@breadoven i think you’re taking the wrong route. You should read about how it rebase ;-) let me know if you need some help.

@breadoven
Copy link
Contributor Author

Probably is the wrong way but I've never had a problem merging main/master in before and in fact the first merge caused no issues. However, nothing worked today, couldn't build due to cmake errors, references to missing files etc, etc, and various other problems that didn't exist before. Don't have time to sort this out now with the repetitive conflicts to fix, so some other time.

@raphaelcoeffic raphaelcoeffic force-pushed the abo_rotary_encoder_invert_mod branch from 2af1e2e to 2e1f59a Compare June 12, 2022 21:46
@raphaelcoeffic
Copy link
Member

@breadoven I just did it for you. Now please verify that everything is alright. DO NOT MERGE while pulling the changes, use a force pull (basically force-reset your local branch on the remote one after fetching the remote). Otherwise you will break everything.

@breadoven
Copy link
Contributor Author

Well I managed to break it probably because I use Github desktop and I'm not sure it does force pull + I'm no Github expert if that isn't already obvious. Having said that I don't know what Github did with that upstream/main merge because fundamentally as I understand it merging isn't much different to rebasing as far as changes are concerned.

However, I have managed to use the commits you made to bring the changes into a new branch which looks OK except I can't build the firmware to check it because of problems with thirdparty stuff. One relates to:

CMake Error at radio/src/targets/common/arm/stm32/f4/CMakeLists.txt:56 (add_library):
  Cannot find source file:

    thirdparty/FreeRTOS/list.c

This is using a clean Build folder set up with:
cmake -DPCB=X7 -DPCBREV=ZORRO -DDEFAULT_MODE=2 -DGVARS=YES -DPPM_UNIT=PERCENT_PREC1 -DHELI=NO -DINTERNAL_GPS=NO -DCMAKE_BUILD_TYPE=Release ../

Built using:
make -jnproc firmware

The radio/src/thirdparty/FreeRTOS folder is empty.

Also if I create a new branch or switch branches I keep getting changes related to "libopenui" appearing even though nothing was changed. This wasn't happening before. Do these third party libraries need to be added somewhere as separate repositories or accounted for in some way ?

If I can get the new branch to work then it might be easier to bin this PR and open new replacement.

@raphaelcoeffic
Copy link
Member

raphaelcoeffic commented Jun 13, 2022

@breadoven merging is a lot different from rebasing!

In your particular case it makes the following differences:

  • merge newer main into your branch: at once, the pull request contains lots of changes not related to your PR (totally impossible to review then).
  • rebase on top of main: only your changes are seen, and you are forced to port the original code the new main by resolving conflicts as it gets rebased (on a commit-by-commit base). It is then very easy to review as only the feature specific changes are seen.

Also, and believe me on this one (+20 years experience), lots of things go wrong / lost in this kind of merges and subsequent fix commits. Most of the time some changes get lost, or land doubled somewhere in the code.

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

Labels

B&W Related generally to black and white LCD radios enhancement ✨ New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Re-code behaviour of rotary encoder (roller) input

3 participants