EdgeTX / edgetx

EdgeTX is the cutting edge open source firmware for your R/C radio
https://edgetx.org
GNU General Public License v2.0
1.59k stars 338 forks source link

6POS switch does not work when set to 'inverted' in hardware settings. #5144

Closed philmoz closed 4 months ago

philmoz commented 4 months ago

Is there an existing issue for this problem?

What part of EdgeTX is the focus of this bug?

Transmitter firmware

Current Behavior

If the 6POS switch is set to 'inverted' in hardware settings, it no longer works and is stuck on the '6' position.

Expected Behavior

I can think of three options (option 1 is my preference):

  1. Remove the 'inverted' checkbox for multipos switches.
  2. Ignore the inverted flag and always return the normal state.
  3. Fix the logic for inversion.

I'm not sure it's worth fixing the inversion logic as having the switch positions swapped (1 <-> 6) does not make sense to me,

Steps To Reproduce

Change 6POS switch setting to inverted. On the main view choose different switch positions - the indicator is stuck on 6.

Version

2.10.1

Transmitter

RadioMaster TX16S / TX16SMK2

Operating System (OS)

No response

OS Version

No response

Anything else?

No response

pfeerick commented 4 months ago

I agree with 1 for the simulated analog implementation of 6POS on T16/T18/TX16/Boxer, but not necessary for the traditional/ actual 6POS switch... having said that... you can swap the wires to change the direction... so 1 is path of least resistance.

What about (4) 1, where flag for emulated 6POS is set (which may needed to be added) :thinking:

If you're thinking about editing that section, do you think you could put little headings above the controls for colorlcd... i.e. so people actually know that the toggle on the end is for invert? Similar to what you did for the customisatble switches settings

image

philmoz commented 4 months ago

I agree with 1 for the simulated analog implementation of 6POS on T16/T18/TX16/Boxer, but not necessary for the traditional/ actual 6POS switch... having said that... you can swap the wires to change the direction... so 1 is path of least resistance.

Does the inversion work for the 'traditional' switch (not sure which radio this applies to)?

pfeerick commented 4 months ago

Don't have any fitted with 6POS switch, sorry... it was an optional extra for X9D+ (and also the X9E and X10 I think), as it had support for a third pot. I think the X9D+2019 may have swapped that for an extra switch. But you can install it to any radio with a pot... it is effectively just a pot with six detents... a switched resistor ladder. Used to be used for flight modes and flap positions.

pfeerick commented 4 months ago

One version looked like this if you were wondering: https://github.com/opentx/opentx/issues/5104#issuecomment-323847327

3djc commented 4 months ago

Given 6pos are wired this way, and you can now invert the corresponding analog channel, I would say disable hardware inversion settings for 6pos.

Maybe we should come back at why we added this inversion for pots/sliders: depending on radio physical layout, and pot usage, they need to behave differently. If you take a Zorro for example, the variable resistor is placed like a slider (in otx term), meaning that they get there max value when toward center (1 is CW, 1 is CCW). And that worked fine for 'traditional' modeler where is actuates surfaces.

Come drone pilot and also more abilities to command some of the radio features with pots. If you wanted to use it for say volume and brightness, then you had an issue, since one was working CCW. So when adc refactor came, we changed that distinction pot/slider and give the opportunity to choose how pot should believe.

But all that clearly doesn't make any sense for 6pos, that is hardwired CW/left to right (as it should, imho)

For custom switches, not sure what hardware invert would mean

raphaelcoeffic commented 4 months ago

Just a small remark: the 6POS calibration routine effectively assumes that the voltage will raise as the position number increases. So if you installed a 6POS potentiometer (a real one, not simulated), and you wire it the wrong way, it is impossible to calibrate properly, and it will always show position 1 where the lowest voltage is.

Given that users adding a custom 6POS is very rare (does it even exist?), we can somehow just expect that it will be wired the "right" way, and I can totally accept that we exclude 6POS from inversion logic. However, I'm not sure this really simplifying the code, as we need to have some exceptions added.

pfeerick commented 4 months ago

Is that really the case though? If you go from 1st to 6th position, and then back to 1st place, would it not still be calibrated, regardless of the direction it is wired up in?

As I initially said, path of least resistance is to not invert, but doing so adds more exceptions, and removes flexibility. After all, what if the user wants it to be right to left, for whatever reason. It's like me saying Chinese is wrong because they read from right to left, because as a "western" left to right seems right to me! lol Personally, it would not be a problem for me, I'd just change the code if needed, but it is the end users we should be thinking of. ;) It's for this same reason I will be advocating for an inversion option for switches also ;)

On Mon, Jun 10, 2024 at 7:21 PM Raphael Coeffic @.***> wrote:

Just a small remark: the 6POS calibration routine effectively assumes that the voltage will raise as the position number increases. So if you installed a 6POS potentiometer (a real one, not simulated), and you wire it the wrong way, it is impossible to calibrate properly, and it will always show position 1 where the lowest voltage is.

Given that users adding a custom 6POS is very rare (does it even exist?), we can somehow just expect that it will be wired the "right" way, and I can totally accept that we exclude 6POS from inversion logic. However, I'm not sure this really simplifying the code, as we need to have some exceptions added.

— Reply to this email directly, view it on GitHub https://github.com/EdgeTX/edgetx/issues/5144#issuecomment-2157816073, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABJ66KM4EXJOKWTF5KLGSFLZGVV2XAVCNFSM6AAAAABJBJP2W2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJXHAYTMMBXGM . You are receiving this because you commented.Message ID: @.***>

3djc commented 4 months ago

Has there been such a request? In both Arabic and Chinese keyboards, 1 is left, 9 right. It also opens a can of worms? What do you want to invert ? The switch number or the analog channel?

pfeerick commented 4 months ago

This is the hardware page, no? So just like with pots and sliders, only the input mapping, everything else remains unchanged. And yes, being able to invert the switch at the hardware level has been requested before.

On Mon, 10 June 2024, 8:50 pm 3djc, @.***> wrote:

Has there been such a request? It also opens a can of worms? What do you want to invert ? The switch number or the analog channel?

— Reply to this email directly, view it on GitHub https://github.com/EdgeTX/edgetx/issues/5144#issuecomment-2158014578, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABJ66KIEUZNCXR5WL67OALDZGWAFXAVCNFSM6AAAAABJBJP2W2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJYGAYTINJXHA . You are receiving this because you commented.Message ID: @.***>

3djc commented 4 months ago

I was talking 6pos here...

What is 6pos invert ? Sw1 becomes sw6 ? or Sw1 position is high instead of low (because thats what the other invert do)

philmoz commented 4 months ago

What is 6pos invert ? Sw1 becomes sw6 ? or Sw1 position is high instead of low

Isn't that the same thing?

pfeerick commented 4 months ago

Both the direction of 6POS AND whether 6POS1 or 6POS6 high, etc, would need to be "inverted" if the toggle were to be made working properly. SW1-SW6 aren't relevant here since that is customisable switches.

On Tue, 11 June 2024, 6:59 am philmoz, @.***> wrote:

What is 6pos invert ? Sw1 becomes sw6 ? or Sw1 position is high instead of low

Isn't that the same thing?

— Reply to this email directly, view it on GitHub https://github.com/EdgeTX/edgetx/issues/5144#issuecomment-2159273942, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABJ66KL3MMLU7TRESVJN4C3ZGYHU7AVCNFSM6AAAAABJBJP2W2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJZGI3TGOJUGI . You are receiving this because you commented.Message ID: @.***>

philmoz commented 4 months ago

Inversion of the other pots/sliders only reverses the ADC value so the control works reversed. The only use case for inverting the 6POS is when using a physical 6 position switch wired backwards - in which case it should work the same and just reverse the ADC value. 6POS1 should remain as low and 6POS6 as high otherwise the inversion has no effect.

philmoz commented 4 months ago

On a different note - why is the default state for LS set to inverted on the TX16S now?

pfeerick commented 4 months ago

Inversion of the other pots/sliders only reverses the ADC value so the control works reversed.

Exactly. Thus 6POS source would be inverted, and setting the switch to what was previously shown as 6POS1 would now be 6POS6.

On a different note - why is the default state for LS set to inverted on the TX16S now?

Because the sliders should be -100 when at the lowest position, and 100 when at the highest position... (just like with the T20 sliders). Not inverting one of them on the TX16S would result in one being upside down ;)

philmoz commented 4 months ago

On a different note - why is the default state for LS set to inverted on the TX16S now?

Because the sliders should be -100 when at the lowest position, and 100 when at the highest position... (just like with the T20 sliders). Not inverting one of them on the TX16S would result in one being upside down ;)

But it makes LS work upside down in the main view UI. Pushing LS towards the top of the radio makes the on-screen slider go down.

On my TX16S LS is at -100 when it is towards the bottom of the radio (same as RS).

pfeerick commented 4 months ago

No, you are right, sorry. I would have sworn that when I just checked my TX16S was behaving as if LS needed to be inverted to act the same as RS, but on repeat both my test and MK2 TX16S both want it un-inverted. So either a bug, or is inverted for the wrong hardware.

philmoz commented 4 months ago

radio/src/targets/horus/hal.h:

#elif defined(RADIO_TX16S)
  #define ADC_DIRECTION                 {1,-1,1,-1,  1,1,1,   -1,1,1,1,  -1,1 }

The 8th value is LS, the 12th value is EXT3 - both get set to inverted in tx16s.json file. As far as I can see ADC_DIRECTION is only used to generate the hw_defs JSON file.

3djc commented 4 months ago

What is 6pos invert ? Sw1 becomes sw6 ? or Sw1 position is high instead of low

Isn't that the same thing?

No, what I try to explain here is that inverting 6pos can have 2 meanings: 1) 6P1 (left) becomes called 6P6, but the left most button stays low, thats what I read from:

After all, what if the user wants it to be right to left, for whatever reason. It's like me saying Chinese is wrong because they read from right to left, because as a "western" left to right seems right to me!

2) left stays 6P1, but is now highest analog value Since 6P switch number is printed on the case, it would follow what we do for other sliders/pot (ie not change the name, change the analog value for a given position.

Because of this ambiguity, I think 6pos invert should be disabled plain and simple, or it will require, a choice ("name, value, both"), which would be an even bigger exception than simple disabling the inversion

pfeerick commented 4 months ago

Don't confuse the internal handling with end user interface. As an end user, I don't give two hoots if it is high, low, or sideways. If when I set the switch to 6P1, the firmware says 6P6, invert the physical to radio mapping. This also means 6POS, as the overall representation of the input, needs to flip also to suit. If done at low level, it is done in one place only.

On Tue, 11 June 2024, 4:07 pm 3djc, @.***> wrote:

What is 6pos invert ? Sw1 becomes sw6 ? or Sw1 position is high instead of low

Isn't that the same thing?

No, what I try to explain here is that inverting 6pos can have 2 meanings:

  1. 6P1 (left) becomes called 6P6, but the left most button stays low, thats what I read from:

After all, what if the user wants it to be right to left, for whatever reason. It's like me saying Chinese is wrong because they read from right to left, because as a "western" left to right seems right to me! auth/ABJ66KM4EXJOKWTF5KLGSFLZGVV2XAVCNFSM6AAAAABJBJP2W2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJXHAYTMMBXGM> . You are receiving this because you commented.Message ID: @.***>

  1. left stays 6P1, but is now highest analog value Since 6P switch number is printed on the case, it would follow what we do for other sliders/pot (ie not change the name, change the analog value for a given position.

Because of this ambiguity, I think 6pos invert should be disabled plain and simple, or it will require, a choice ("name, value, both"), which would be an even bigger exception than simple disabling the invertion

— Reply to this email directly, view it on GitHub https://github.com/EdgeTX/edgetx/issues/5144#issuecomment-2159868436, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABJ66KL4EATJZST7IRDILY3ZG2HYZAVCNFSM6AAAAABJBJP2W2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJZHA3DQNBTGY . You are receiving this because you commented.Message ID: @.***>

3djc commented 4 months ago

That’s an interesting comment since the whole feature IS about where is low and where is high!

Switch inversion is another topic that can be discussed, but is outside of the scope here

philmoz commented 4 months ago

The default 'inverted' state for the pots & sliders on the T20V2 and MT12 are also wrong. (Comparing radio settings to what Companion sets when creating a new ETX document).

This only seems to affect creating a new ETX document in Companion. Creating a new radio setup on the transmitter seems to be ok - at least on T20V2.

3djc commented 4 months ago

The default 'inverted' state for the pots & sliders on the T20V2 and MT12 are also wrong. (Comparing radio settings to what Companion sets when creating a new ETX document).

Can you help me understand what you mean ? Radio defaults to: image

Meaning nothing is inverted (for user), and that matches the physical implementation (pot CW, slider toward antenna high). So as far as I can tell, it is ok radio side. Are you talking a bout a Companion issue ?

philmoz commented 4 months ago

Create a new blank document in Companion for T20V2. Edit Radio Settings: Screenshot 2024-06-11 at 5 55 31 PM

3djc commented 4 months ago

ok, understood, Companion issue. Companion should not be reading ADC_DIR (assuming that this is what it does here since it matches #define ADC_DIRECTION {1,-1,1,-1, -1,1,-1,-1,-1,-1}) since it match a hardware design, not a user intention to invert a channel

Default should be all unticked (in colorlcd 'term', -> in bw language), for all radio

philmoz commented 4 months ago

ADC_DIRECTION is used in hal_adc.py when generating the xxx.json files for the radios.

3djc commented 4 months ago

Yes, but I mean companion should not care about "inverted": true, in the json.file as this is not relevant to Companion

philmoz commented 4 months ago

As far as I can tell it is ONLY Companion that is using the "inverted" line in the JSON files.

3djc commented 4 months ago

But why ? Definitely not relevant to Companion.

It is usefull in the json file to generate { // P1 // ADC_INPUT_FLEX, GPIOC, LL_GPIO_PIN_1, LL_ADC_CHANNEL_11, 1 // inverted }, { // P2 // ADC_INPUT_FLEX, GPIOC, LL_GPIO_PIN_0, LL_ADC_CHANNEL_10, 0 // normal }, { // SL1 // ADC_INPUT_FLEX, GPIOC, LL_GPIO_PIN_3, LL_ADC_CHANNEL_13, 1 // inverted },

philmoz commented 4 months ago

It would be relevant to Companion if there were cases where the default state should be inverted. In this case you do want Companion to create a radio settings file with the correct state.

3djc commented 4 months ago

It would be relevant to Companion if there were cases where the default state should be inverted. In this case you do want Companion to create a radio settings file with the correct state.

But we insure when creating the hal description that those are never user inverted, thats what ADC_DIRECTION is all about, handling the low level hardware inversion, it should not matter to companion, and be used only to generate things like stm32_adc_inputs.inc

philmoz commented 4 months ago

Got it - thanks. So just ignore 'inverted' in Companion.

3djc commented 4 months ago

Got it - thanks. So just ignore 'inverted' in Companion.

Exactly, the goal is to have the radio UI (and Companion) show all non inverted when you receive the radio, and have the radio working 'normaly'