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

Replace PASSTHROUGH with MANUAL flight mode (FW) #2661

Conversation

shellixyz
Copy link
Collaborator

@shellixyz shellixyz commented Jan 16, 2018

MANUAL flight mode is equivalent to PASSTHROUGH mode but with dedicated roll/pitch/yaw rates and expo settings. This PR implements feature request described in #2648.

New settings:

  • rc_manual_roll_expo / rc_manual_pitch_expo / rc_manual_yaw_expo: dedicated roll/pitch/yaw expo settings similar to rc_expo and rc_yaw_expo but for manual mode
    - rc_manual_expo / rc_manual_yaw_expo: dedicated roll/pitch/yaw expo settings similar to rc_expo and rc_yaw_expo but for manual mode
  • manual_rc_expo / manual_rc_yaw_expo: dedicated roll/pitch/yaw expo settings similar to rc_expo and rc_yaw_expo but for manual mode (since fc72312)
  • manual_roll_rate / manual_pitch_rate / manual_yaw_rate: dedicated roll/pitch/yaw rate settings for manual mode. Unit %. A value of 100 (default) means full deflection.

Configurator update to access new settings through GUI adjustments tab: iNavFlight/inav-configurator#329.

Tasks:

  • Replace PASSTHROUGH mode with MANUAL mode
  • Merge roll and pitch expo
  • Update OSD to show MANU instead of PASS
  • Allow new settings to be changed from OSD menu

@@ -40,14 +40,20 @@ void pgResetFn_controlRateProfiles(controlRateConfig_t *instance)
for (int i = 0; i < MAX_CONTROL_RATE_PROFILE_COUNT; i++) {
RESET_CONFIG(controlRateConfig_t, &instance[i],
.rcExpo8 = 70,
.manual_pitch_rcExpo8 = 70,
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need separate expo for MANUAL?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure, I couldn't fly much since I made these changes because the weather doesn't let me. Makes it possible to tune the settings exactly as you like. Do you think it is an issue adding those regarding memory constraints or other ?

Copy link
Member

Choose a reason for hiding this comment

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

More like simplicity. If there is no need to have it, probably it's not needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe separate roll and pitch expo is not really needed. But I've to point those things:
1)Most pilot that use PASS mode use additional expo in their FrSky radios, I use FlySky and I cannot do it, will be nice if FC does it instead of the transmitter.
2)We really should find a way to link rcExpo8 with manual_rcExpo8. Like people increasing expo when in stabilized mode but ending not touching expo when in manual mode and having to change two settings is not very ideal, ok but not ideal :) What about doing like, manualRcExpo=100 in Unit %, as we do for the rates? 100 will be default and then one can increase or decrease expo in regards to rcExpo value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@giacomo892

  1. Not sure I understand. The changes already makes that possible (more expo in manual compared to the other modes). Are you asking to change the way I done it or only saying it is better now.
  2. I'm not against this change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Redshifft To make what work? If you are talking about this PR it is already working fine. If you are talking about the F1 board no line of code should be required only undefining some defines.

Choose a reason for hiding this comment

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

@shellixyz Thought you wanted separate rate/mixer and expo values for passthrough?

That F1 board is a rev3 Afromini but it never had a separate target just the generic Naze, but it seems DE has pulled all the F1 stuff.

Copy link

Choose a reason for hiding this comment

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

I agree with the points @Redshifft has mentioned. Without any additional features in iNav it is already possible to set up and fly an aircraft in PASSTHROUGH and using RC EXPO in iNav (in PASSTHROUGH) to adjust stick feel. If the aircraft feels more / less agile in ACRO compared to PASSTHROUGH, it is just a matter of lowering / increasing the rates in ACRO. Adjusting an aircraft in let's say ACRO mode first and then trying to compensate any weird behavior in PASSTHROUGH by additional parameters doesn't sound logical to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Dronek The issue at least for me is that I can't use less control surfaces movements in passthrough compared to the other modes. Well I could but not without sacrificing channels or other functionality and even then it wouldn't be very practical or won't allow me to configure the mode switch(es) as I like.

@Redshifft I can add preprocessor directives to disable the new settings if you like. I personally don't think it is important because there are a lot other less basic things you can disable. Like Spektrum LED strip support (first things coming to my mind).

Choose a reason for hiding this comment

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

@shellixyz that would be good :) I did go as far as loading most of the compiler and dependencies etc and I thought, I will need lots of other stuff etc and software is just not my thing! If I had everything in place I'm sure I could have got something half usable in about 2 weeks :D
It's a simple little board with no USB bridge chip no SPI just 6050 and bmp280 on I2c, no flash.
Just need MSP the 2 ublox autocfgs PPM rx voltage and current adc's and the just the Airplane half of 1.8.1 (although I doubt that can be split easily) soft serial not required but gps and blheli passthrough would be very useful, Thanks

@@ -40,14 +40,20 @@ void pgResetFn_controlRateProfiles(controlRateConfig_t *instance)
for (int i = 0; i < MAX_CONTROL_RATE_PROFILE_COUNT; i++) {
RESET_CONFIG(controlRateConfig_t, &instance[i],
.rcExpo8 = 70,
.manual_pitch_rcExpo8 = 70,
.manual_roll_rcExpo8 = 70,
Copy link
Member

Choose a reason for hiding this comment

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

Also, could you add those new setting to MSP? Either MSP_FW_CONFIG but even better as separate MPS2 frame. We have plenty of IDs over there.
And add frame description to https://github.com/iNavFlight/inav/wiki/INAV-MSP-frames-changelog

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes sure good idea. I didn't think about it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@DzikuVx Are you sure there is something to change to be able to get/set the new settings with MSP2 ? It is the first time I read the code but from what I understand any settings can be get/set with MSP2_COMMON_SETTING/MSP2_COMMON_SET_SETTING frame types.

Copy link
Member

Choose a reason for hiding this comment

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

I gotta agree with @shellixyz on this one. I'd rather NOT see these settings in dedicated MSP commands. They don't seem to fit the use case of being frequently polled and or updated during a flight, so the overhead of MSP2_COMMON_SETTING/MSP2_COMMON_SET_SETTING should make no performance difference. It will, however, save a bit of flash and a bunch of developer time (which is the scarcest resource of them all!).

@DzikuVx
Copy link
Member

DzikuVx commented Jan 16, 2018

awesome, but please add to MSP2. We keep forgetting that and then we had a case when most of the setting was usable only via CLI without any GUI

@shellixyz
Copy link
Collaborator Author

I also think the OSD code should be updated to show when this new mode is selected

@shellixyz
Copy link
Collaborator Author

And also allow the new settings to be changed from the OSD menu

@fiam
Copy link
Member

fiam commented Jan 16, 2018

@shellixyz Adding the settings to the CMS menu should be relatively easy. Check the OSD_SETTING_ENTRY() macro, there are a lot of uses of it in src/main/cms/ cms_menu_navigation.c.

@digitalentity
Copy link
Member

Good stuff! Will review as soon as possible!

.rates[FD_YAW] = CONTROL_RATE_CONFIG_YAW_RATE_DEFAULT,
.manual_rates[FD_ROLL] = 100,
.manual_rates[FD_PITCH] = 100,
.manual_rates[FD_YAW] = 100
Copy link
Member

Choose a reason for hiding this comment

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

Since we change the format of the PG wouldn't it be better to group entries by function? I.e.

  • Throttle/TPA settings
  • Stabilized mode settings/rates
  • Manual mode settings/rates?

Also note that since we change the in-storage format we need to bump up the version number of the PG definition here (the trailing 0):

PG_REGISTER_ARRAY_WITH_RESET_FN(controlRateConfig_t, MAX_CONTROL_RATE_PROFILE_COUNT, controlRateProfiles, PG_CONTROL_RATE_PROFILES, 0);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed will make the code clearer. Done.

@@ -34,7 +34,7 @@ typedef enum {
BOXCAMSTAB = 7,
BOXNAVRTH = 8, // old GPSHOME
BOXNAVPOSHOLD = 9, // old GPSHOLD
BOXPASSTHRU = 10,
BOXMANUAL = 10,
Copy link
Member

Choose a reason for hiding this comment

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

Indentation

input[INPUT_STABILIZED_PITCH] = rcCommand[PITCH];
input[INPUT_STABILIZED_YAW] = rcCommand[YAW];
if (FLIGHT_MODE(MANUAL_MODE)) {
input[INPUT_STABILIZED_ROLL] = rcCommand[ROLL] * currentControlRateProfile->manual_rates[FD_ROLL] / 100L;
Copy link
Member

Choose a reason for hiding this comment

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

I believe manual_rates should be applied when calculating rcCommand values in annexCode. Mixer is not the right place to apply settings from controlRateProfile

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right.

Copy link
Member

@digitalentity digitalentity left a comment

Choose a reason for hiding this comment

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

Suggest to do some minor changes - see comments.

Copy link
Collaborator Author

@shellixyz shellixyz left a comment

Choose a reason for hiding this comment

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

Done

@@ -70,14 +70,14 @@ typedef enum {
NAV_POSHOLD_MODE= (1 << 5), // old GPS_HOLD
HEADFREE_MODE = (1 << 6),
NAV_LAUNCH_MODE = (1 << 7),
PASSTHRU_MODE = (1 << 8),
FAILSAFE_MODE = (1 << 10),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note sure this is important but I just noticed I changed the mode values to fill the previously unused 10th bit. I don't know if this can have any side effect. Tell me if it is wrong I will restore them to the old values.

Copy link
Member

Choose a reason for hiding this comment

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

@shellixyz probably it's better to keep things as they are. We are reporting flight mode flags to blackbox and rearranging the bits will make things all messed up.

input[INPUT_STABILIZED_PITCH] = rcCommand[PITCH];
input[INPUT_STABILIZED_YAW] = rcCommand[YAW];
if (FLIGHT_MODE(MANUAL_MODE)) {
input[INPUT_STABILIZED_ROLL] = rcCommand[ROLL] * currentControlRateProfile->manual_rates[FD_ROLL] / 100L;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right.

.rates[FD_YAW] = CONTROL_RATE_CONFIG_YAW_RATE_DEFAULT,
.manual_rates[FD_ROLL] = 100,
.manual_rates[FD_PITCH] = 100,
.manual_rates[FD_YAW] = 100
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed will make the code clearer. Done.

@shellixyz
Copy link
Collaborator Author

I merged the RC adjustments parts into this PR.

Configurator update to access new settings through GUI adjustments tab: iNavFlight/inav-configurator#329.

@shellixyz shellixyz changed the title Add MANUAL flight mode for fixed wing Replace PASSTHROUGH with MANUAL flight mode (FW) Jan 24, 2018
Copy link
Member

@digitalentity digitalentity left a comment

Choose a reason for hiding this comment

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

Would be great to have a new MSPv2 frame for manual mode settings in GUI but it can be done in a dedicated PR.

.rates[FD_ROLL] = 100,
.rates[FD_PITCH] = 100,
.rates[FD_YAW] = 100
}
Copy link
Member

Choose a reason for hiding this comment

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

Yay! This is even cleaner than I thought it would be 👍


.manual = {
.rcExpo8 = 70,
.rcYawExpo8 = 20,
Copy link
Member

Choose a reason for hiding this comment

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

Suggest we keep manual expos at zero by default to retain 100% compatibility with old PASSTHROUGH behavior. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Expo is applied even in PASSTHROUGH that's why I set the default expo/yaw_expo to 70/20 to keep the old behavior but I don't mind setting them to 0 by default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

About the MSPv2 frame there is already MSP2_COMMON_SETTING and MSP2_COMMON_SET_SETTING frame types that can be used to query/set the new settings. Do we need a new one ?

Copy link
Member

Choose a reason for hiding this comment

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

@shellixyz Regarding expo - I see. Didn't realize that. Then it's ok.

Regarding MSPv2 - it would still be good to set this settings in a single message. Possibly a new message to handle the whole rate profile data.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. New frame IDs are 0x2007 (MSP2_INAV_RATE_PROFILE) and 0x2008 (MSP2_INAV_SET_RATE_PROFILE) because I already used lower IDs for new messages in #2668.

@digitalentity
Copy link
Member

No more remarks on my end.
@DzikuVx - ?

@shellixyz
Copy link
Collaborator Author

shellixyz commented Jan 26, 2018

I tested the new MSPv2 messages. Everything is OK. I have an update for the configurator allowing to set the manual settings with the GUI. It will need some cosmetic changes before being published though and I will need some help to do it.

Where should the new manual rates be configured in the GUI ? For the moment I placed them in the PID tuning tab with the other rates but I know it is not the right place. In the servos or the receiver tab maybe ?

I placed the manual expo settings with the stabilized expo settings in the receiver tab but I can't get them to be displayed as I would like it. I hate HTML/JS so much :p

@DzikuVx It looks like you know how it is working. Do you know how I can inspect the HTML/CSS code inside the running application ?

@digitalentity digitalentity added this to the 1.9 milestone Jan 31, 2018
@digitalentity digitalentity merged commit fd40892 into iNavFlight:development Jan 31, 2018
giacomo892 pushed a commit to giacomo892/inav that referenced this pull request Feb 2, 2018
* FW: add manual flight mode
* manual mode: separate roll and pitch expo
* manual flight mode: cleaning
* replace PASSTHRU mode with MANUAL mode
* manual mode: merge pitch and roll expo
* manual mode: add OSD menu for manual rates and expo
* manual mode: forgot to add yaw rate adjustment in OSD menu
* manual mode: add adjustments
* manual mode: move rates applications in fc_core
* group controlRateConfig settings by function
* group controlRateConfig settings by function: fix ALIENFLIGHTF3 default rates config
* manual mode rc adjustments: adapt to fc72312 changes
* manual mode: clean rc adjustments
* add new MSPv2 messages: MSP2_INAV_RATE_PROFILE and MSP2_INAV_SET_RATE_PROFILE
@shellixyz shellixyz deleted the development_manual_flight_mode_separate_pitchroll_expo branch February 3, 2018 17:57
@asgarPL
Copy link

asgarPL commented Oct 23, 2020

Hello, at the beginning I would like to send my respects and thank all the INAV team for yours support and improving the INAV - BIG THX :). I'm before my INAV 2.5 maiden flight on the Sonic AR WING 900. What worries me the most is taken approach for the common roll and pitch expo factor for the Manual mode. Stabilisation is cool, but manual fixed wing control is still important for me. I've spent almost year on tweaking my TARANIS mixing, like I want it to feel. My preffered manual control feeling (and mixing) is especially important for me for the INAV maiden flight and the servo-trim "plane tunning" stage. What are my possibilites for Manual mode differential roll and pitch expo configuration? Should I resign from the INAV manual mode expo configuration approach and preserve my mixing settings on the TARANIS radio only if Manual mode switch is enabled (I understand in this way my expo mixing won't fight with the INAV built-in stabilisation) or can I achive this in some other way? I'm wondering whether there can be some wing controlling (mixing) issues when switching between stabilised and manual mode? I'm considering also creating some INAV branch with initial Shellixyz differential expo approach. Do you see allready some INAV processing impacts for the initial Shellixyz differential expo approach or was it only "EXPO requirements simplification" to address most common usecases? Thanks in advance for your answers. Best regards.

@Redshifft
Copy link

Regardless of what extra expo setting are available in Inav, Manual mode should be used to set up the Aeroplane for optimal maximum surface deflection and servo/linkage center/trim. Once optimized control stick feel can be tailored with TX Expo or Inavs Expo if available. I think you are best to do Aileron differential etc.. mechanically with servo arms and control horns

@asgarPL
Copy link

asgarPL commented Oct 30, 2020

@Redshifft. Thx for yours answer and for further control sticks optimization hint via TX Expo. However, personally I think that, for the new INAV (or new model) beginners it would be just much easier to move theirs TARANIS (or other radio) expo and weight settings to the INAV Manual mode expo settings to have the same flying feeling, especially when we are talking about the auto-triming in INAV manual mode flight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants