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 PWM mapping/failsafe to RX Lua #1763

Merged
merged 17 commits into from Oct 12, 2022
Merged

Conversation

CapnBry
Copy link
Member

@CapnBry CapnBry commented Aug 11, 2022

Adds the ability to change PWM output mappings and failsafe positions from Lua
image

Details

Builds on #1757

Adds "Output Mapping" folder with the following items:

  • Output Ch (1-16) - Selects which channel is being edited by the following items. This defaults to Output 5 as it is the most remapped channel, since it defaults to read from CH5/AUX1
  • Input Ch (1-16) - Selects the input channel from the handset to use for the selected Output Ch
  • Output Mode (50Hz;60Hz;100Hz;160Hz;333Hz;400Hz;On/Off) - Sets the PWM output frequency or On/Off mode for the selected Output Ch
  • Inverted (Off;On) - Sets if the channel output is inverted for the selected Output Ch

In addition there is also a new command:

  • Set Failsafe Pos - Executing this and confirming the popup will set the failsafe position of all PWM Output channels

For Discussion

How do we feel about allowing everyone to remap their channel outputs, not just PWM users? This would be a separate PR since that would require a lot of changes to the RX in a lot of places and would make this PR harder to review.

Advantages: The use case is that there are external stabilization units which have functions assigned to fixed channels, and CH5 (AUX1) is usually one of them. If every RX can remap their channels, then the user can set CH5 in the output CRSF stream to come from input CH8 or something.

Disadvantages: The code would have to swizzle the channels around every time which adds overhead (latency) to every packet. IsArmed would also have to reverse map back to original input CH5, not the new output CH5, which is minor.

Major Disadvantage: People are dumb AF and really will F / confuse themselves doing this.

@pkendall64
Copy link
Collaborator

I did some testing of this today, one thing I noticed is that the 750us option is missing from the LUA.
Also, I was a little confused by the Output Ch and Input Ch labels at first, just because everything had 1 in them, until I realised that when you set the output it's basically selecting the mapping for that "output channel pin" on the RX.

Also, it might be good to merge master into these 2 branches to make testing easier.

@pkendall64
Copy link
Collaborator

I sorted your disadvantage of the RX IsArmed problem, by removing IsArmed from RXes altogether 😄 See PR #1765

@CapnBry
Copy link
Member Author

CapnBry commented Aug 12, 2022

did some testing of this today, one thing I noticed is that the 750us option is missing from the LUA.

I didn't include that one because Lua params are pretty fat and I didn't want to include an option I think zero people use. It has to be loaded every time through the OTA. I wasn't sure about the output mode, but I think people will actually use that option. If I were to add something else, I'd probably want the failsafe position but I didn't want to rejigger our entire Lua system just to add it (since our entire system is based on the value only being one byte currently).

Also, I was a little confused by the Output Ch and Input Ch labels at first, just because everything had 1 in them

The alternative would be to download 16 channels x 4 Lua parameters, and that's why this wasn't in the original PWM update PR-- that's just not viable. I came up with this "selector" pattern instead, but it is a little confusing how it works at first. There's no good way to describe it in 15 chars or less. Output Select seems even further off base.

It should have come up with 5 though as the default Output Ch though, are you sure everything had a 1?

@pkendall64
Copy link
Collaborator

did some testing of this today, one thing I noticed is that the 750us option is missing from the LUA.

I didn't include that one because Lua params are pretty fat and I didn't want to include an option I think zero people use. It has to be loaded every time through the OTA. I wasn't sure about the output mode, but I think people will actually use that option. If I were to add something else, I'd probably want the failsafe position but I didn't want to rejigger our entire Lua system just to add it (since our entire system is based on the value only being one byte currently).

That makes sense why its no there then.

Also, I was a little confused by the Output Ch and Input Ch labels at first, just because everything had 1 in them

The alternative would be to download 16 channels x 4 Lua parameters, and that's why this wasn't in the original PWM update PR-- that's just not viable. I came up with this "selector" pattern instead, but it is a little confusing how it works at first. There's no good way to describe it in 15 chars or less. Output Select seems even further off base.

I fully agree with this, it's just that the names were a little confusing, i.e. Output Ch seems wrong because it's not really a channel, it's a pin that just happens to be labeled Ch 1 etc. Maybe the label could be Output Pin and the values could be Ch 1, Ch 2 etc.

It should have come up with 5 though as the default Output Ch though, are you sure everything had a 1?

It did come up with 5, but when I changed that to 1, 2, 3 ,4 etc all the input channels were 1.

@CapnBry
Copy link
Member Author

CapnBry commented Aug 14, 2022

Output Ch seems wrong because it's not really a channel, it's a pin that just happens to be labeled Ch 1 etc. Maybe the label could be Output Pin and the values could be Ch 1, Ch 2 etc.

Yeah that's why in the webui I went with just "Output". It seemed more confusing with just "Output" versus "Output Ch". I think we should allow people to remap the channels even on CRSF output as well, to work with fixed-input FCs that have stuff on CH5. If that were the case then "Output Ch" is more appropriate. What about Output Ch/Pin? Oof that doesn't fit 😞 Output No? Argh there's nothing clear!

I'm pretty wary of changing the option type to a select, since that changes the size of the option:

  • As a number = 0
  • CH1;CH2;CH3;CH4;CH5;CH6;CH7;CH8;CH9;CH10;CH11;CH12;CH13;CH14;CH15;CH16 = 70
  • Ch 1;Ch 2;Ch 3;Ch 4;Ch 5;Ch 6;Ch 7;Ch 8;Ch 9;Ch 10;Ch 11;Ch 12;Ch 13;Ch 14;Ch 15;Ch 16 = 86
  • CH1;CH2;CH3;CH4;CH5(AUX1);CH6(AUX2);CH7(AUX3);CH8(AUX4);CH9(AUX5);CH10(AUX6);CH11(AUX7);CH12(AUX8);CH13(AUX9);CH14(AUX10);CH15(AUX11);CH16(AUX12) = 145

At just 5 bytes of data per telemetry packet those really really slow down the loading of the parameter list, 100Hz FullRes takes 7-8 seconds to load that last one, 50Hz takes around 12s. Every time you change anything on the Mapping page it take 7-9 seconds for the list to refresh, even if we went with the 70 byte version it really crushes the usability, so I think this is a bad idea even if it is a little clearer.

It did come up with 5, but when I changed that to 1, 2, 3 ,4 etc all the input channels were 1.

Sounds like your config has them all set to 1 for all those Outputs? If that's what you're seeing then I'd check the webui to see if the same values are there. I've flashed a few random RXes and they all have the default mapping (1->1, 2->2 , 3->3, etc), and the PWM RXes I use with CH5 remapped showed the correct values I'd configured.

@pkendall64
Copy link
Collaborator

Sad trombone!

I think you're right, theres just no good option on the name and changing the number to an enum is going to be too slow, especially for the people on slower rates (which is really the target audience as they'll be using this the most).

Could we use the unit for some information? i.e. (Ch/Pin) for Output?

The problem with the config, I'm pretty sure it was a me problem, but it just exacerbated the confusion. 😄

Copy link
Contributor

@schugabe schugabe left a comment

Choose a reason for hiding this comment

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

I would not add the remapping for "no pwm users". If it's just about channel 5 we could think about options to skip it from the output. But that is probably also confusing to configure.

# Conflicts:
#	src/lib/LUA/rx_devLUA.cpp
@pkendall64
Copy link
Collaborator

pkendall64 commented Oct 12, 2022

I would not add the remapping for "no pwm users". If it's just about channel 5 we could think about options to skip it from the output. But that is probably also confusing to configure.

The PWM remapping LUA info is not there on a non-PWM RX.

I see what you're saying. You're talking about the questions Cap has in the Discussion section.

@pkendall64 pkendall64 merged commit 8586df2 into ExpressLRS:master Oct 12, 2022
@CapnBry CapnBry deleted the rx-lua-mapping branch October 25, 2022 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants