NFC: Reboot required #6231
NFC: Reboot required #6231
Conversation
|
The EKF one is wrong and SerialManager isn't necessarily true. Others look correct to me, but need to be confirmed. |
|
I've pulled in the Copter and Proximity fixes to master. Normally we don't pull in parts of PRs but they look correct and I needed something to kick off the AC3.5 release so I thought perhaps two birds with one stone. |
| @@ -123,6 +123,7 @@ const AP_Param::GroupInfo NavEKF2::var_info[] = { | |||
| // @Description: This enables EKF2. Enabling EKF2 only makes the maths run, it does not mean it will be used for flight control. To use it for flight control set AHRS_EKF_TYPE=2. A reboot or restart will need to be performed after changing the value of EK2_ENABLE for it to take effect. | |||
There was a problem hiding this comment.
Why is this one wrong ? The description explicitly tells it
There was a problem hiding this comment.
A reboot isn't needed when enabling, just if disabling.
There was a problem hiding this comment.
Is it not better to always warn the user ? When they change something and it does not work, it's confusing. Some people might not come to the idea that they need a reboot, and simply try to find other parameters to change, or get frustrated. If a reset is needed in some situations just warn all the time. This way it will work all the time and not only in some situations. BTW where is it documented that this requires reboot when disabling and not on enabling ? And for other parameters ? Where are the docs ? will the users find them? will they even search for it ?
I vote that is better to request a reset more than a reset less. :)
|
If I change serial 4 to GPS, the second GPS does not start until I reboot. |
|
@amilcarlucas It depends on the library using the serial ports. You can for example enable MAVLink2 without rebooting. As I said, it might happen for some cases, but is not necessarily true that a reboot is needed. |
|
If it requires a reboot on different cases, then just add the warning and always require rebooting |
|
Regarding the serial ports, I got a lot of trouble in the begining of using ardupilot, because I did some serial changes and it did not ask me to reset. For other stuff it asked me for a reset. For serial it did not asked and I assumed it was not needed. BIG mistake. That took me some time to figure out. Here I'm trying to save other people from committing the same errors I did in the past. |
|
@amilcarlucas This doesn't ask you anything, it's just a documentation tag. In my opinion it shouldn't have a reboot required tag unless it is always and absolutely needed, but take it to the dev call and you'll get others' opinion, then it's decided the way forward. |
|
On Tue, 16 May 2017, Amilcar Lucas wrote:
Regarding the serial ports, I got a lot of trouble in the begining of using
ardupilot, because I did some serial changes and it did not ask me to reset.
For other stuff it asked me for a reset. For serial it did not asked and I
assumed it was not needed. BIG mistake. That took me some time to figure
out.
Here I'm trying to save other people from committing the same errors I did
in the past.
I think we should have a technical fix for this problem, rather than a
documentation fix.
Currently we have a technical fix for the compass calibration - it informs
the user via statustext that a reboot is required.
We could have a "char *reboot_required_reason" which can be set by
anything, and is checked by AP_Arming (and it gets emitted as a
statustext). This will solve the whole "sometimes requires reboot" issue.
AP_Arming::set_reboot_required(const char *reason);
Alternatively, AP_Arming could check more libraries for reboot-required
as we do here:
https://github.com/ardupilot/ardupilot/blob/master/libraries/AP_Arming/AP_Arming.cpp#L200
Depends on whether you want everything to know about AP_Arming or
AP_Arming to know about everything. Since AP_Arming already pretty much
has to know about everything, the latter scheme is probably preferable.
|
|
ok, so best solution would be not to be necessary to ever reboot after changing parameters, next best solution would be Peter's suggestion on the AP_Arming, third best solution is the current documentation improvement. As we don't have the first two let's go with the third. I share the "pain" with @amilcarlucas and many other users that actually use ArduPilot on the field and not on a desk, so let's make life easy to the normal users. |
|
Can someone merge this ? It has already been discussed and accepted at the team meeting. |
|
@amilcarlucas Can you please rebase and remove the ones that Randy already cherry-picked? |
|
a rebase should auto-remove them |
amilcarlucas
force-pushed
the
reboot-required
branch
from
c93b949
to
c8998b2
Compare
|
Rebase done, as requested |
|
Merged, thanks! |
I'm not 100% sure about this, .... but I think these are missing.