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: add inverted sources #4722

Merged
merged 12 commits into from
May 24, 2024
Merged

feat: add inverted sources #4722

merged 12 commits into from
May 24, 2024

Conversation

elecpower
Copy link
Collaborator

@elecpower elecpower commented Mar 11, 2024

Summary of Changes

As a pre-requisite for #4452 add the ability to invert a 'Source'.

Currently supports Inputs, Mixes, Logical Switches and Special/Global functions.

TODO:

  • B&W UI
  • Companion support
  • Add other 'Sources'

Companion changes:
The Companion refactor changes RawSource index from zero to one based to support negative indexes (as RawSwitch).

The opportunity was taken to undertake some overdue housekeeping of RawSource and move timers from source type Special to new Timers. Also rename legacy type SOURCE_TYPE_STICK to SOURCE_TYPE_INPUT as more appropriate.

@elecpower elecpower marked this pull request as draft March 11, 2024 09:36
@philmoz
Copy link
Collaborator

philmoz commented Mar 11, 2024

It might be easier for me to revert my Companion changes and rebase my PR to this PR.

I'll have a play with it later in the week.

@elecpower
Copy link
Collaborator Author

elecpower commented Mar 13, 2024

I need to 'merge' #4735 into this one

@elecpower
Copy link
Collaborator Author

Merge of 4735 done

@philmoz
Copy link
Collaborator

philmoz commented Mar 25, 2024

@elecpower When you have a chance can you rebase this to current main. I'd like to continue testing in conjunction with the radio side changes.

@elecpower
Copy link
Collaborator Author

@elecpower When you have a chance can you rebase this to current main. I'd like to continue testing in conjunction with the radio side changes.

Request noted

@elecpower elecpower force-pushed the elecpower/feat-cpn-rawsource-invert branch from 3b967d2 to e8ed206 Compare March 25, 2024 20:51
@elecpower
Copy link
Collaborator Author

@philmoz rebased

@philmoz
Copy link
Collaborator

philmoz commented Mar 25, 2024

Looks good, thanks. If you're ok with it, I will add the radio side changes to this PR.

@elecpower
Copy link
Collaborator Author

I'm okay with you adding your changes.

@philmoz
Copy link
Collaborator

philmoz commented Mar 26, 2024

I have merged the radio changes into this PR, I will close 4513 in favour of the one.
I think this is ready for review.

@elecpower
Copy link
Collaborator Author

Might be a good idea to include the other PR's summary above and remove my what happens next.

@philmoz philmoz changed the title feat(cpn): add inverted sources feat: add inverted sources Mar 26, 2024
@pfeerick pfeerick added this to the 2.11 milestone Mar 29, 2024
@pfeerick pfeerick added the enhancement ✨ New feature or request label Mar 29, 2024
@philmoz philmoz force-pushed the elecpower/feat-cpn-rawsource-invert branch from 0c4cb94 to 7c1100e Compare April 9, 2024 03:22
@philmoz
Copy link
Collaborator

philmoz commented Apr 9, 2024

@elecpower - when you have a chance can you please check the rebase to make sure I haven't messed anything up.

@elecpower
Copy link
Collaborator Author

@elecpower - when you have a chance can you please check the rebase to make sure I haven't messed anything up.

Will check out now I have Cloudbuild in a testable state.

@elecpower
Copy link
Collaborator Author

elecpower commented Apr 13, 2024

@philmoz rebase okay but I have found some more areas where some fixes are required due to the +1 shift:

  • Conversion of Timers 1 - 3
  • Conversion of SF Adjust GV and when using a GV 1 - 9
  • Conversion 6 pos switches are wrong in the GUI dropdown lists due to hardware settings - this is a ADC refactor caused issue with incorrect type being set Companion v2.10.0 importing otx and etx files mangles hardware settings #4876. This has a flow on affect to model settings as they are imported. I'll need to create a new PR to fix v2.10-rc3 too.

I'll fix these with individual commits

@elecpower
Copy link
Collaborator Author

Refer PR #4890 for last part of fixes

@philmoz philmoz force-pushed the elecpower/feat-cpn-rawsource-invert branch from b5abc08 to e75e400 Compare May 7, 2024 12:21
@pfeerick pfeerick marked this pull request as ready for review May 16, 2024 22:48
@philmoz philmoz force-pushed the elecpower/feat-cpn-rawsource-invert branch from e75e400 to e57603f Compare May 17, 2024 03:04
@pfeerick pfeerick self-requested a review May 19, 2024 03:13
@pfeerick
Copy link
Member

Thanks for the rebase @philmoz ... am I right in the assumption it is ready for review?

@philmoz
Copy link
Collaborator

philmoz commented May 19, 2024

Thanks for the rebase @philmoz ... am I right in the assumption it is ready for review?

I think so.

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.

Colorlcd (TX16S):

  • "Invert" button seems to be a couple pixels too far down and to the right, means the toolbar moves when you press the button

Generally:

  • SF -> Adjust -> GV# -> mixer source : inverted source does not survive power cycle
  • LS -> a>x seems generally broken visually - colourlcd won't let me set V2 values any more - shows !GPS instead of 0, bw128 shows wrong V1 on overview
    image
    image
  • bw128 EM'd when paging from curves to LS screen with above config after having written it via Companion, or after attempting to configure it on radio and exiting edit page.
  • bw212 appears to be missing implementation of showing ! but it is marked as a TODO

@philmoz
Copy link
Collaborator

philmoz commented May 23, 2024

Possibly issues from all the rebases - will take a look.

@philmoz philmoz force-pushed the elecpower/feat-cpn-rawsource-invert branch from e57603f to 57d6b23 Compare May 23, 2024 11:47
@philmoz
Copy link
Collaborator

philmoz commented May 23, 2024

Colorlcd (TX16S):

  • "Invert" button seems to be a couple pixels too far down and to the right, means the toolbar moves when you press the button

Generally:

  • SF -> Adjust -> GV# -> mixer source : inverted source does not survive power cycle
  • LS -> a>x seems generally broken visually - colourlcd won't let me set V2 values any more - shows !GPS instead of 0, bw128 shows wrong V1 on overview
  • bw128 EM'd when paging from curves to LS screen with above config after having written it via Companion, or after attempting to configure it on radio and exiting edit page.
  • bw212 appears to be missing implementation of showing ! but it is marked as a TODO

These should be fixed now.

@3djc
Copy link
Collaborator

3djc commented May 23, 2024

BW128: LS -> a<x V1 does not show invert option. That said, I'm unsure if this is an issue or not, since you can always choose your V2 accordingly. But I feel it should be there.

@philmoz
Copy link
Collaborator

philmoz commented May 23, 2024

BW128: LS -> a<x V1 does not show invert option. That said, I'm unsure if this is an issue or not, since you can always choose your V2 accordingly. But I feel it should be there.

It's working for me - can you share an image of where it's wrong for you.

@pfeerick
Copy link
Member

pfeerick commented May 23, 2024

Looking a lot better, thanks! :)

I think I caught it happening... on both bw128 and bw212, the "Invert" option falls off the popup menu if "Telemetry" item is present... ie. it will work just fine with a blank model, but seems as if it has telemetry sensors the option is never visible.

Without telem

image

With telem

image

bw212 inputs screen needs to learn to show ! also... rest of the screens seem to fine (inverted but no !)

image

common to both bw128 and 212 is that with some inverted inputs the V2 field changes it's units incorrectly... i.e. I have seen it show "0.0V" for one input, and can get this to happen in simulator (V2 field is blank, but somehow editable).

image

@philmoz
Copy link
Collaborator

philmoz commented May 24, 2024

Looking a lot better, thanks! :)

I think I caught it happening... on both bw128 and bw212, the "Invert" option falls off the popup menu if "Telemetry" item is present... ie. it will work just fine with a blank model, but seems as if it has telemetry sensors the option is never visible.

bw212 inputs screen needs to learn to show ! also... rest of the screens seem to fine (inverted but no !)

common to both bw128 and 212 is that with some inverted inputs the V2 field changes it's units incorrectly... i.e. I have seen it show "0.0V" for one input, and can get this to happen in simulator (V2 field is blank, but somehow editable).

I think these are fixed now.

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.

bw128, bw212 and colorlcd (tx16s) all appear to generally be working as expected! Looking good! :)

@pfeerick pfeerick merged commit 80659c3 into main May 24, 2024
47 checks passed
@pfeerick pfeerick deleted the elecpower/feat-cpn-rawsource-invert branch May 24, 2024 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants