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 support for ExpressLRS optional arming method #5641

Merged
merged 27 commits into from
Nov 22, 2024

Conversation

mha1
Copy link
Contributor

@mha1 mha1 commented Oct 25, 2024

This PR adds support for ExpressLRS/ExpressLRS#3008

Changes:

  • adds UI to select Arming mode Switch and to set a switch (logical, physical or any other suitable source)
  • sends requested arming state in extended CRSF message 0x16 in arming mode Switch

For more details please refer to ExpressLRS PR #3008

@ulfhedlund
Copy link
Contributor

Great new functionality!
SE translation:
#define TR_SF_ARM "Aktivera"
#define TR_ARMING_MODE "Aktivering"

@pfeerick pfeerick added enhancement ✨ New feature or request user manual change Will impact on the user manual in some respect labels Oct 30, 2024
@ajjjjjjjj
Copy link
Contributor

ajjjjjjjj commented Oct 30, 2024

This seems to be designed as CRSF specific, but is only tied to ELRS. How about adding "ELRS" to all labels to improve UX?
Also this could be done in lua by using getSwitch*() and/or getLogicalSwitchValue() functions.

I'm not EdgeTX advocate nor involved as developer/UX designer but I don't like how ELRS configuration must be done in many places (lua, specific module configuration, specific special function) where all of this can be kept in lua.

I know historically MPM is also integrated in EdgeTX but because there was no lua by the time and other constraints. Now, there is better way, so i don't think it is the way to go nor user friendly.

I'm not against your change, I just see other path that could be considered by project maintainers to make both projects better.

@philmoz
Copy link
Collaborator

philmoz commented Oct 30, 2024

Why does this need a special function? This seems overly complicated to me.

Wouldn't it be simpler and easier for users to just use a switch that the user can select.
Then the options would be 'Channel5' and 'Switch'.
If 'Switch' is selected then the user can use the existing switch selection UI to pick which switch to use for arming.
There looks to be plenty of room in the ModuleData structure to put both the arming mode flag and the selected arming switch in the 'crsf' section.

@pfeerick
Copy link
Member

pfeerick commented Oct 30, 2024

This seems to be designed as CRSF specific, but is only tied to ELRS. How about adding "ELRS" to all labels to improve UX?
Also this could be done in lua by using getSwitch*() and/or getLogicalSwitchValue() functions.

I guess if the same mechanism as is used to display the bind option only for ELRS that could be done. Especially since this is a ELRS only option. For configuration, sure Lua is fine, but I don't like it for critical mixer tasks - i.e. I would want to be certain this wouldn't cause the link to be dropped if Lua is terminated.

Why does this need a special function? This seems overly complicated to me.

Then the options would be 'Channel5' and 'Switch'.
If 'Switch' is selected then the user can use the existing switch selection UI to pick which switch to use for arming.

As long as you keep in mind it may not even need to be a physical switch but could be a LS (or even constant 😀), that would be a much better UX, as well as it bit more self-documenting.

@mha1
Copy link
Contributor Author

mha1 commented Oct 30, 2024

Why does this need a special function? This seems overly complicated to me.

Wouldn't it be simpler and easier for users to just use a switch that the user can select. Then the options would be 'Channel5' and 'Switch'. If 'Switch' is selected then the user can use the existing switch selection UI to pick which switch to use for arming. There looks to be plenty of room in the ModuleData structure to put both the arming mode flag and the selected arming switch in the 'crsf' section.

Actually that's a good idea idea if "Switch" can be a logical switch too which I think it does, doesn't it?

I agree on "Channel" vs "Channel5". maybe even better "AUX1(CH5)" because ExpressLRS/BF mostly use the term AUX1 if that fits the small b&w screens too.

@pfeerick
Copy link
Member

pfeerick commented Oct 30, 2024

AUX1 is inav/bf notation though? What about ardupilot, sbus etc? May be better to not have non-ELRS specific verbiage. After all, we don't use any other BF/INAV specific.

I.e. Channel 5 for long form, CH5 for short.

@mha1
Copy link
Contributor Author

mha1 commented Oct 30, 2024

AUX1 is inav/bf notation though? What about ardupilot, sbus etc? May be better to not have non-ELRS specific verbiage. After all, we don't use any other BF/INAV specific.

Hah, same thought about "Switch". And you're right about ON/OFF too. Maybe I'm wrong but the reason why I chose SF Arm is the fact all the logic for activation/deactivation is already there and can be used. Having a "Switch" selection in the CRSF UI just defines the switch. If "Switch" is more than physical switch, ie more like "Trigger" the logic to determine the status needs to be done. It'd be like a hidden internal SF.

AUX1 is also ExpressLRS lingo. Open the online documentation at expresslrs.org and search for AUX1. But really, I'm good with Channel5 or CH5 (if Channel5 doesn't fit the b&w screens).

Sorry about the missing translation. I missed that one. Will update everything once we agree on the UI. I like the idea to keep it all to the CRSF UI.

@mha1
Copy link
Contributor Author

mha1 commented Nov 11, 2024

And how about replacing the label "Arming" with "Arm using". It'd fit b&w too and might help to make the intention clear.

WhatsApp Image 2024-11-11 at 12 54 56_ab6825db
WhatsApp Image 2024-11-11 at 12 55 32_d585ffc4
WhatsApp Image 2024-11-11 at 13 01 58_df73a655

@pfeerick
Copy link
Member

pfeerick commented Nov 11, 2024 via email

@mha1
Copy link
Contributor Author

mha1 commented Nov 11, 2024

That sounds good. Can be surprising how a small change makes all the difference.

Thanks, great, will do it.

@mha1
Copy link
Contributor Author

mha1 commented Nov 12, 2024

@pfeerick I updated all the translations files. CH5 and Switch are now reusing TR_CH and TR_SWITCH, so no need to ask for translations but #define TR_CRSF_ARMING_MODE "Arm using" needs translating.

If you're happy with this could you please trigger the translators.

@ulfhedlund
Copy link
Contributor

SE translation:
#define TR_CRSF_ARMING_MODE TR("Aktivera m.", "Aktivera med"), unless the full length translation actually fits on BW small screen.

@mha1
Copy link
Contributor Author

mha1 commented Nov 13, 2024

SE translation: #define TR_CRSF_ARMING_MODE TR("Aktivera m.", "Aktivera med"), unless the full length translation actually fits on BW small screen.

If I count correctly b&w allows a max of 10 characters. Your b&w translation might be too long.

@3djc
Copy link
Collaborator

3djc commented Nov 13, 2024

BW doesnt use fixed width fonts, so it depends on the text itself, not char count

@mha1
Copy link
Contributor Author

mha1 commented Nov 16, 2024

Translators, can we get some translations for this PR please.

#define TR_CRSF_ARMING_MODE "Arm using" is best understood as "Use arming method to arm/disarm your ELRS TX module. For more context please refer to the first post of ExpressLRS/ExpressLRS#3008 and #5641 (comment).

@froqstar
Copy link

DE
#define TR_CRSF_ARMING_MODE "Armen via"

@other german speakers: Do we have a german word for arming in EdgeTX/ExpressLRS yet? Personally I use "armen/gearmed" in german conversations, but not sure how widespread that is. "Entsichern" or "scharf schalten" are too long and militaristic for me.

@Pat6874
Copy link
Contributor

Pat6874 commented Nov 17, 2024 via email

@robustini
Copy link
Contributor

robustini commented Nov 17, 2024

IT
#define TR_CRSF_ARMING_MODE TR("Modo arm", "Modo d'armamento")

Also in Italian since time immemorial “armed” has referred more to weapons, there is little use of the term “armed,” e.g. we do not say “the house alarm is armed” but “the house alarm is enabled, or activated.”
So in italian an armed F-35 means it carries bombs or missiles on board, unarmed means it has no armament on board.
But a house alarm cannot have bombs or missiles installed! :p
In fact for that reason DJI people were smarter, they always used “start or stop motors,” never arm/disarm.

@3djc
Copy link
Collaborator

3djc commented Nov 17, 2024

We also need shorter forms for BW

@Pat6874
Copy link
Contributor

Pat6874 commented Nov 17, 2024 via email

@robustini
Copy link
Contributor

We also need shorter forms for BW

IT edited.

@Eldenroot
Copy link
Contributor

#define TR_CRSF_ARMING_MODE "Arm mód"

@zyren
Copy link
Contributor

zyren commented Nov 18, 2024


CN

#define TR_CRSF_ARMING_MODE            "解锁类型" 

TW

#define TR_CRSF_ARMING_MODE            "解鎖類型" 

@mha1
Copy link
Contributor Author

mha1 commented Nov 18, 2024

Thank you guys. So far I have:

CN: #define TR_CRSF_ARMING_MODE "解锁类型"
TW: #define TR_CRSF_ARMING_MODE "解鎖類型"
CZ: #define TR_CRSF_ARMING_MODE "Arm mód"
IT: #define TR_CRSF_ARMING_MODE TR("Modo arm", "Modo d'armamento")
FR: #define TR_CRSF_ARMING_MODE TR("Mod. Arm.", "Mode d'armement")
DE: #define TR_CRSF_ARMING_MODE "Armen via"
SE: #define TR_CRSF_ARMING_MODE TR("Aktivera m.", "Aktivera med")
RU: #define TR_CRSF_ARMING_MODE "Arm режим"
DA: #define TR_CRSF_ARMING_MODE TR("Aktiver m.", "Aktiver med")
ES: #define TR_CRSF_ARMING_MODE TR("Modo Arm.", "Modo Armado")
PT: #define TR_CRSF_ARMING_MODE TR("Armar via", "Armar usando")
JP: #define TR_CRSF_ARMING_MODE "アーム ロック解除"

@kobakirill
Copy link
Contributor

kobakirill commented Nov 18, 2024

RU
#define TR_CRSF_ARMING_MODE "Arm режим"

@HThuren
Copy link
Contributor

HThuren commented Nov 18, 2024

DA translation:
#define TR_CRSF_ARMING_MODE TR("Aktiver m.", "Aktiver med")

@Luznatural
Copy link

Luznatural commented Nov 19, 2024

ES translation:
#define TR_CRSF_ARMING_MODE TR "Modo Armado"
Shorter version
#define TR_CRSF_ARMING_MODE TR "Modo Arm."

@zandorsp
Copy link
Contributor

zandorsp commented Nov 19, 2024

PT
#define TR_CRSF_ARMING_MODE TR("Armar via", "Armar usando")

@zandorsp
Copy link
Contributor

zandorsp commented Nov 19, 2024

Also in Italian since time immemorial “armed” has referred more to weapons, there is little use of the term “armed,” e.g. we do not say “the house alarm is armed” but “the house alarm is enabled, or activated.”

Portuguese doesn't have this problem. We say 'the house alarm is armed', even outside a military context, but this is the Brazilian use. I am curious about other countries that use Portuguese, mainly Portugal. Anyone?

@ToshihiroMakuuchi
Copy link
Contributor

JP
#define TR_CRSF_ARMING_MODE "アーム ロック解除"

@mha1
Copy link
Contributor Author

mha1 commented Nov 19, 2024

So far only FI, HE, NL and PL are missing. Does anyone know who to contact for FI, HE, NL?

@pfeerick
Copy link
Member

HE translator asked not to be pinged for each PR, I think they plan on doing translations nearer to a release as a batch. For the others, nada, nope... so until someone says "I speak the lingo, I'll do it"... they get ignored 😇

@mha1
Copy link
Contributor Author

mha1 commented Nov 20, 2024

HE translator asked not to be pinged for each PR, I think they plan on doing translations nearer to a release as a batch. For the others, nada, nope... so until someone says "I speak the lingo, I'll do it"... they get ignored 😇

Alright, then we're through

@pfeerick pfeerick added this to the 2.11 milestone Nov 22, 2024
@pfeerick pfeerick merged commit 8fd3d50 into EdgeTX:main Nov 22, 2024
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ New feature or request user manual change Will impact on the user manual in some respect
Projects
None yet
Development

Successfully merging this pull request may close these issues.