-
Notifications
You must be signed in to change notification settings - Fork 167
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
flight: osd: properly display reset reason #1117
Conversation
We immediately returned before which prevented it from working.
int32_t len = AlarmString(&alarm, buf + pos, sizeof(buf) - 1 - pos, | ||
blink, &state); | ||
|
||
len += pos; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer pos += len
, or pos += AlarmString(...
instead of len, and then write a null char at pos but I suppose this will do. :-P
Looks good. |
} | ||
|
||
uint8_t state; | ||
int32_t len = AlarmString(&alarm, buf, sizeof(buf) - 1, blink, &state); | ||
int32_t len = AlarmString(&alarm, buf + pos, sizeof(buf) - 1 - pos, | ||
blink, &state); | ||
|
||
if (len > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
len won't ever be < 0, so it doesn't seem all that useful to have this condition here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just in case it ever returns error in the future, I figured. This is my second variant of this..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it will be 0 if there are no warning/error/critical alarms?
dRonin/flight/Libraries/alarms.c
Lines 329 to 396 in 080150a
int32_t AlarmString(SystemAlarmsData *alarm, char *buf, size_t buflen, bool blink, uint8_t *state) { | |
*state = SYSTEMALARMS_ALARM_OK; | |
buf[0] = '\0'; | |
int pos = 0; | |
// TODO(dustin): sort the alarms by severity. Alarm messages | |
// will get truncated. We want the most urgent stuff to show | |
// up first, having warnings show up only if there's space. | |
for (int i = 0; i < SYSTEMALARMS_ALARM_NUMELEM; i++) { | |
if (((alarm->Alarm[i] == SYSTEMALARMS_ALARM_WARNING) || | |
(alarm->Alarm[i] == SYSTEMALARMS_ALARM_ERROR) || | |
(alarm->Alarm[i] == SYSTEMALARMS_ALARM_CRITICAL))) { | |
if (!alarm_names[i][0]) { | |
// zero-length alarm names indicate the alarm is | |
// explicitly ignored | |
continue; | |
} | |
// Returned state is the worst state. | |
if (alarm->Alarm[i] > *state) { | |
*state = alarm->Alarm[i]; | |
} | |
char current_msg[LONGEST_MESSAGE+1] = {0}; | |
switch (i) { | |
case SYSTEMALARMS_ALARM_SYSTEMCONFIGURATION: | |
strncpy(current_msg, | |
(const char*)config_error_names[alarm->ConfigError], | |
sizeof(*config_error_names)); | |
current_msg[sizeof(*config_error_names)] = '\0'; | |
break; | |
case SYSTEMALARMS_ALARM_MANUALCONTROL: | |
strncpy(current_msg, | |
(const char*)manual_control_names[alarm->ManualControl], | |
sizeof(*manual_control_names)); | |
current_msg[sizeof(*manual_control_names)] = '\0'; | |
break; | |
default: | |
strncpy(current_msg, (const char*)alarm_names[i], sizeof(*alarm_names)); | |
current_msg[sizeof(*alarm_names)] = '\0'; | |
} | |
int this_len = strlen(current_msg); | |
if (pos + this_len + 2 >= buflen) { | |
break; | |
} | |
if ((alarm->Alarm[i] != SYSTEMALARMS_ALARM_WARNING) && !blink) { | |
this_len += 1; | |
while (this_len > 0) { | |
buf[pos++] = ' '; | |
this_len--; | |
} | |
continue; | |
} | |
memcpy(&buf[pos], current_msg, this_len); | |
pos += this_len; | |
buf[pos++] = ' '; | |
} | |
} | |
buf[pos] = '\0'; | |
return pos; | |
} |
-e- derp, you are suggesting just unconditionally adding len. Fair comment.
Looked over this pretty well. The existing code has potential for a future bug (#1120), but I'm happy this is safe for now. |
We immediately returned before which prevented it from working.