Skip to content

Commit

Permalink
DataFlash: check all backends for sensor health
Browse files Browse the repository at this point in the history
  • Loading branch information
@peterbarker @rmackay9
peterbarker authored and rmackay9 committed Jun 15, 2017
1 parent 3c3fee6 commit 268d652
Showing 1 changed file with 12 additions and 2 deletions.
14 changes: 12 additions & 2 deletions libraries/DataFlash/DataFlash.cpp
View file
Expand Up @@ -242,15 +242,25 @@ bool DataFlash_Class::logging_enabled() const
if (_next_backend == 0) {
return false;
}
return backends[0]->logging_enabled();
for (uint8_t i=0; i<_next_backend; i++) {
if (backends[i]->logging_enabled()) {
return true;
}
}
return false;
}
bool DataFlash_Class::logging_failed() const
{
if (_next_backend < 1) {
// we should not have been called!
return true;
}
return backends[0]->logging_failed();
for (uint8_t i=0; i<_next_backend; i++) {
if (backends[i]->logging_failed()) {
return true;
}
}
return false;
}

void DataFlash_Class::Log_Write_MessageF(const char *fmt, ...)
Expand Down

14 comments on commit 268d652

@fnoop
Copy link
Contributor

@fnoop fnoop commented on 268d652 Jun 18, 2017

Choose a reason for hiding this comment

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

This now fails prearm checks if LOG_BACKEND_TYPE set to 3 and it doesn't detect a mavlink log receiver. Is this really desirable?

@peterbarker
Copy link
Contributor Author

@peterbarker peterbarker commented on 268d652 Jun 19, 2017 via email

Choose a reason for hiding this comment

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

@fnoop
Copy link
Contributor

@fnoop fnoop commented on 268d652 Jun 19, 2017

Choose a reason for hiding this comment

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

But this doesn't affect or improve flight performance in any way. Surely this should be a warning, rather than a hard fail? Plus you have to reset the flight controller to change logging, so it's quite a major fail.
My 2c, anyway..

@Pedals2Paddles
Copy link
Contributor

Choose a reason for hiding this comment

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

The major fail is setting the parameter wrong, not the prearm failure as a result. This is operator error, plain and simple.

@fnoop
Copy link
Contributor

@fnoop fnoop commented on 268d652 Jun 19, 2017

Choose a reason for hiding this comment

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

It wasn't an error, it was deliberate. I set it to 3 so I could have mavlink logging when CC was attached, and SD card logging otherwise as a backup. This workflow worked just fine until this -rc. Having to reconfigure the firmware and reboot the flight controller, or else fiddle around with bitmasks to turn off pre-arm checks, seems like bad UX to me. This is the only check that doesn't affect flight safety, and as such could either be implemented as an optional (non-default) pre-arm check or a warning.
But clearly you feel a better way forward is to tell the users that we're idiots and continue to make this software harder and harder to use, except by 'those in the know'. Good luck with that.

@Pedals2Paddles
Copy link
Contributor

Choose a reason for hiding this comment

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

Insulting the developers who would be considering your suggestion is probably not effective either.

ARMING_CHECK set for 7166 will ignore logging prearm failures if you have this odd one off scenario of sometimes using a CC and sometimes not.

@fnoop
Copy link
Contributor

@fnoop fnoop commented on 268d652 Jun 19, 2017

Choose a reason for hiding this comment

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

Nobody was insulting anyone until you came along. I was simply giving my opinion on this as someone who was directly affected by this change, to Pete and Randy who authored/approved this change, who I have the upmost respect for. Not sure why you feel the need to tell me what, how or why I should be doing something. I'll just keep quiet in the future.

@tridge
Copy link
Contributor

@tridge tridge commented on 268d652 Jun 19, 2017

Choose a reason for hiding this comment

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

settle down everyone - all friends here I hope :-)
I think the issue is really whether the arming error should be associated with "no logging available" or "one of the configured types of logging not available".
@peterbarker given LOG_BACKEND_TYPE is an enum at the moment not a bitmask, would you be OK with a LOG_BACKEND_TYPE=4 meaning that it tries to start both, but only gives an arming error is neither is available?

@fnoop
Copy link
Contributor

@fnoop fnoop commented on 268d652 Jun 19, 2017

Choose a reason for hiding this comment

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

Just to clarify, when I said 'so it's quite a major fail' I didn't mean 'major fail' in terms of bad coding or bad decision or anything like that, I meant 'major fail' in terms of the action you have to take to get round it (change parameter, reboot flight controller), just in case that was misunderstood.

I absolutely don't expect anyone to change anything just because I think otherwise. I understand logging is very important, if you guys decide (again!) that whatever is set should be working to arm then for sure I'm not going to argue further against the greater experience. (Unsurprisingly) I like tridge's suggestion, very good, but if that introduces ambiguity into the pre-arm model then I understand if it's not included.

Ultimately what I was thinking is that the flight controller should be concerned with what happens on-board the flight controller and directly attached sensors and what directly affects flight safety, not what happens elsewhere on the mavlink network. Making a fail on pre-arm check for the SD card logging absolutely makes sense. However, as the proliferation of companion computers or other networked devices and use types increases, I believe that adding hard dependencies on these loosely connected devices will lead to confusion and bad UX. It doesn't take much to confuse me, but I am probably a reasonably typical user :)

@Pedals2Paddles
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, the former is how I interpreted the major fail thing. I understand what you mean now, Sorry about that.

Anyway, I like Tridge's idea. It just then becomes how to appropriately warn/caution the operator. Is there already a DF logging status that can be used for this?

@peterbarker
Copy link
Contributor Author

@peterbarker peterbarker commented on 268d652 Jun 20, 2017 via email

Choose a reason for hiding this comment

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

@OXINARF
Copy link
Member

Choose a reason for hiding this comment

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

I would say that the type parameter is in practice a bitmask, although there is an enum value defined as both. The only reason I don't like using the type parameter for this is that if we later add another backend type, the list of acceptable values will be very confusing.
Maybe add a parameter to do this? Something like LOG_HEALTHY_ALL that would only change between checking all or at least one, or something like LOG_BACK_HEALTHY that would be a bitmask of the types to check are healthy.

@fnoop
Copy link
Contributor

@fnoop fnoop commented on 268d652 Jun 20, 2017

Choose a reason for hiding this comment

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

@Pedals2Paddles No problem, I'll try to be clearer in the future :)

Someone else on the 3.5 forum reporting exactly the same confusion today with this, so I'm not alone. Thanks for considering this. I would say that whatever gets done should be simple and obvious, because it's such a critical function for the user to understand and set right, and to minimise confusion/frustration when the user gets it wrong (as I so ably demonstrated).

As a wider question, should there perhaps be a separate set of pre-arm checks that don't hard fail, but produce warnings or other actions instead? These checks could be non-flight-critical checks, such as optional logging, is the flight camera running, is VO active. Or certain pre-arm checks that are not active by default when ARMING_CHECK=1?

Perhaps it's easier just to leave it the way it is!

@peterbarker
Copy link
Contributor Author

@peterbarker peterbarker commented on 268d652 Jun 20, 2017 via email

Choose a reason for hiding this comment

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

Please sign in to comment.