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

Autostart: Allow overriding airframe defaults #8006

Closed
wants to merge 1 commit into from

Conversation

LorenzMeier
Copy link
Member

@LorenzMeier LorenzMeier commented Sep 24, 2017

This is necessary to enable setting new airframe defaults after significant configuration changes.

@dagar needs to be considered in your param PR.

@dagar
Copy link
Member

dagar commented Sep 24, 2017

Good idea! Could we handle it from here though without the extra variable? https://github.com/PX4/Firmware/blob/master/ROMFS/px4fmu_common/init.d/rcS#L147

If the param ver changes then set AUTOCNF, but skip the full param reset. That keeps each airframe config simpler.

@dagar
Copy link
Member

dagar commented Sep 24, 2017

How about this? Use the AUTOCNF mechanism if SYS_PARAM_VER is different and only set it to DEFAULTS_VER if we make it to the end of autostart.
https://github.com/PX4/Firmware/compare/master...dagar:pr-autostart_defaults?expand=1

@LorenzMeier
Copy link
Member Author

Autocnf wipes params first and therefore we can't use the same mechanism.

@dagar
Copy link
Member

dagar commented Sep 26, 2017

SYS_AUTOCONFIG resets the params, then sets AUTOCNF.
https://github.com/PX4/Firmware/blob/master/ROMFS/px4fmu_common/init.d/rcS#L150

This would be setting AUTOCNF independently.

@LorenzMeier
Copy link
Member Author

That's alright then.

@dagar
Copy link
Member

dagar commented Sep 28, 2017

Updated this branch to AUTOCNF version.

@dagar dagar removed their request for review September 28, 2017 13:28
@dagar dagar self-assigned this Sep 28, 2017
@dagar dagar added this to the Release v1.7.0 milestone Sep 28, 2017
@dagar dagar requested a review from bkueng September 29, 2017 14:38
Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

The param comment needs to be updated as well: https://github.com/PX4/Firmware/blob/master/src/modules/systemlib/system_params.c#L141

I find this a bit problematic. It can lead to unexpected effects for both devels and users. For example if you use a generic quad config, this will reset your configured PWM range (https://github.com/PX4/Firmware/blob/master/ROMFS/tap_common/init.d/rc.mc_defaults#L8). Even worse for all VTOLs, it changes the tuning gains: https://github.com/PX4/Firmware/blob/master/ROMFS/tap_common/init.d/rc.vtol_defaults#L7

fi

param set SYS_PARAM_VER ${DEFAULTS_VER}
unset DEFAULTS_VER
param save
Copy link
Member

Choose a reason for hiding this comment

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

param save is not needed

@dagar
Copy link
Member

dagar commented Oct 1, 2017

VTOL defaults shouldn't be setting tuning parameters.

We could try to be more conservative with the AUTOCNF section of the the default (MC, FW, VTOL, GND). I'm more concerned with users missing defaults. In practice many select an airframe once, upgrade in place, and miss out on new defaults.

To be honest I don't know what the best solution is.

@bkueng
Copy link
Member

bkueng commented Oct 2, 2017

We could try to be more conservative with the AUTOCNF section of the the default (MC, FW, VTOL, GND). I'm more concerned with users missing defaults. In practice many select an airframe once, upgrade in place, and miss out on new defaults.

I agree we should have something like this in place, but it should be more fine-grained. Some options that I see:

  • Use 2 levels for autoconf: full and reduced (instead of the current yes only). Selecting the airframe would set full and param version update reduced. full then sets all params while reduced only sets a subset.
  • A more elaborate solution would be that QGC tells you 'hey there are param updates, do you want to apply them?'. For this we can add a constant param SYS_PARAM_VER_INTERNAL (equal to DEFAULTS_VER in this PR) that we increase, and then QGC compares it against SYS_PARAM_VER after loading the params, and then sets SYS_AUTOCONFIG. A fancier version would be that QGC shows you which params changed, including the new values. We could build on top of FTP for that (since that is reliable now), such that QGC can download the relevant information from the vehicle.

@dagar
Copy link
Member

dagar commented Oct 2, 2017

Alright, let's keep thinking this through. I like the idea of doing more on the QGC side, but obviously quite a lot is missing to do that usefully.

What if we added another mechanism to track these alternate defaults? Params actually set by a user should be preserved if at all possible. Params that were never touched by a user should continue to track the default.

If the default (or alternate default) value of a parameter changes and the user has set that parameter they should be alerted.

@bkueng
Copy link
Member

bkueng commented Oct 3, 2017

Params actually set by a user should be preserved if at all possible. Params that were never touched by a user should continue to track the default.

Sounds good to me. I'm just wondering how easy it is to distinguish. For example if we treat all params changed via mavlink as user-set, it means if you backup the params and then load them via QGC, all will be marked as user-set. Similar for the 'set to default' functionality (which uses the same mavlink message) and param set via console vs. scripts.

@dagar
Copy link
Member

dagar commented May 26, 2018

We still need a solution for this.

@stale
Copy link

stale bot commented Jan 20, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale
Copy link

stale bot commented Feb 4, 2019

Closing as stale.

@stale stale bot closed this Feb 4, 2019
@dagar dagar deleted the pr-autostart_defaults branch February 6, 2019 03:03
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.

5 participants