Andrew: This is our last spot to add something. In the future we could have an option to select what goes in that last bit.
We could even add LOGGING_OPTIONS with a bit enabling a NAMED_VALUE_FLOAT to send this information. Peter: This one doesnt need to be permanent, we can always change into another display.
Approved!
UTC0715
Randy: Sorry it’s taken so long for me to review it.
Perhaps if the variable was converted into a volatile it would improve things?
Andy: I don’t think so. The conflict stems elsewhere. R: I still don’t understand how this bug can manifest itself and how the PR fixes it. A: Seems like the motor test flag is never checked now. Andy: The only reason it was checked is because it wasn’t thread-safe before. A: Is the rate thread doing the output now? Andy: Yes. That’s what doing the output now always when rate thread is enabled. R: Right, that’s nice and consistent. P: Could we use actual thread locks to do this? A: Yes, we could have a mutex, but the motor_test flag is necessary for the MAVLink interaction. R: How about the user issuing two consecutive motor test commands? Are we safe against that? A: No, that can’t happen. MAVLink packets are processed in-order. One is processed fully before the other is examined.
A mutex would still be nice, in the case of Linux boards. In case the compiler re-ordered instructions. Not likely to happen in the STM processors.
Merged!
UTC0734
A: My personal preference would be to include the enum in the header to make sure all enum definitions align. And have shifting operations instead of final powers of 2. P: Okay, done! A: A switch statement would be preferrable, but this one is likely correct as well.
Qurt is failing. You could move AP_AHRS:: as a top-level namespace. And rename Status into NavStatus or something.
P: What if I put the enums next to the bits? A: It takes away the point of AP_Nav_Common.h.
UTC0803
R: What if we rename the message into “initializing” to not worry the user too much? P: This can only happen in a corner case that would cause a hardfault.
We can remove the check_failed() and not bother with warning the user.
Approved!
UTC0811
R: This has been observed by 2-3 developers and partners. A: Is it safe to call init() again?
Probably yes, the init() override isn’t doing anything anyways.
Merged!
UTC0816
A: The risk is that in MAVLink2 we run a different code-path that has different behaviour. P: I’d rather link only the MAVLink streams that are doing signing. And leave the rest independent. A: It makes me a bit nervous, as it’s suggested by the PR.
Merged!
UTC0827
P: It now conflicts with the previous PR. A: There will be issues with stale GCS documentation, but we can’t help it.
Randy should probably look at this also, w.r.t. user experience.
We could make a transitional parameter index, to copy the old one but have the moved bits cleared.
Adding an arming check if these bits are set is possible, perhaps a bit extreme.