Skip to content

Error output improvements #8076

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Oct 18, 2018
Merged

Error output improvements #8076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1094c08
Add support for UARTSerial::write from critical section
kjbracey Sep 10, 2018
78f4b4b
Make mbed_error not serial-specific
kjbracey Sep 10, 2018
d05c60e
Sync output devices on exit
kjbracey Sep 10, 2018
c989845
mbed_error_vfprintf -> mbed_error_vprintf
kjbracey Sep 11, 2018
2df322c
Don't overrun in error prints
kjbracey Sep 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions drivers/UARTSerial.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,23 @@ void UARTSerial::sigio(Callback<void()> func)
core_util_critical_section_exit();
}

/* Special synchronous write designed to work from critical section, such
* as in mbed_error_vprintf.
*/
ssize_t UARTSerial::write_unbuffered(const char *buf_ptr, size_t length)
{
while (!_txbuf.empty()) {
tx_irq();
}

for (size_t data_written = 0; data_written < length; data_written++) {
SerialBase::_base_putc(*buf_ptr++);
data_written++;
}

return length;
}

ssize_t UARTSerial::write(const void *buffer, size_t length)
{
size_t data_written = 0;
Expand All @@ -143,6 +160,10 @@ ssize_t UARTSerial::write(const void *buffer, size_t length)
return 0;
}

if (core_util_in_critical_section()) {
return write_unbuffered(buf_ptr, length);
Copy link

Choose a reason for hiding this comment

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

Should we advertise after this change, that printf from ISR is valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even after this, printf from ISR isn't valid - you'll hit a FILE mutex inside the C library. What may work is write(STDERR_FILENO), which avoids the C library, but that in turn will depend on what exactly stderr is hooked to.

And we don't absolutely stop the user redirecting stderr to a File, eg using freopen.

The potential write from ISR is a "last resort" - it has nothing else it can reasonably do. I don't think we should absolutely require that write to stderr work from interrupts, but we can encourage people to try to use stderr devices that can handle this.

}

api_lock();

// Unlike read, we should write the whole thing if blocking. POSIX only
Expand Down
3 changes: 3 additions & 0 deletions drivers/UARTSerial.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,9 @@ class UARTSerial : private SerialBase, public FileHandle, private NonCopyable<UA
/** Release mutex */
virtual void api_unlock(void);

/** Unbuffered write - invoked when write called from critical section */
ssize_t write_unbuffered(const char *buf_ptr, size_t length);

/** Software serial buffers
* By default buffer size is 256 for TX and 256 for RX. Configurable through mbed_app.json
*/
Expand Down
58 changes: 29 additions & 29 deletions platform/mbed_board.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,8 @@
#include "platform/mbed_wait_api.h"
#include "platform/mbed_toolchain.h"
#include "platform/mbed_interface.h"
#include "platform/mbed_retarget.h"
#include "platform/mbed_critical.h"
#include "hal/serial_api.h"

#if DEVICE_SERIAL
extern int stdio_uart_inited;
extern serial_t stdio_uart;
#endif

WEAK void mbed_die(void)
{
Expand Down Expand Up @@ -55,36 +50,41 @@ void mbed_error_printf(const char *format, ...)
{
va_list arg;
va_start(arg, format);
mbed_error_vfprintf(format, arg);
mbed_error_vprintf(format, arg);
va_end(arg);
}

void mbed_error_vfprintf(const char *format, va_list arg)
void mbed_error_vprintf(const char *format, va_list arg)
{
#if DEVICE_SERIAL
#define ERROR_BUF_SIZE (128)
core_util_critical_section_enter();
char buffer[ERROR_BUF_SIZE];
int size = vsnprintf(buffer, ERROR_BUF_SIZE, format, arg);
if (size > 0) {
if (!stdio_uart_inited) {
serial_init(&stdio_uart, STDIO_UART_TX, STDIO_UART_RX);
}
#if MBED_CONF_PLATFORM_STDIO_CONVERT_NEWLINES
char stdio_out_prev = '\0';
for (int i = 0; i < size; i++) {
if (buffer[i] == '\n' && stdio_out_prev != '\r') {
serial_putc(&stdio_uart, '\r');
}
serial_putc(&stdio_uart, buffer[i]);
stdio_out_prev = buffer[i];
char buffer[132];
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this buffer size be configurable? Now we have one rare MBED_ASSERT()ion failure in CI, which only logs part of the filename:
mbed assertation failed: FALSE, file: ./mbed-os/targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F439xI/TARGET_MODULE_UBLOX_ODIN_W�°���DY��`—��Ý<
I suppose it would be better to have only the N trailing chars from the filename, but that might be a bigger change.

Copy link
Contributor Author

@kjbracey kjbracey Oct 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This does partially address that assert issue; and both a commit I removed and #8255 address the other half.

This commit will make that be mbed assertation failed: FALSE, file: ./mbed-os/targets/TARGET_STM/TARGET_STM32F4/TARGET_STM32F439xI/TARGET_MODULE_UBLOX_ODIN_W...\n

Which is an improvement - the \n means that you'll see the line in GreenTea, so you at least get the FALSE. (Moral of that story - use meaningful expressions in your asserts).

The commit I used to have here broke the assert up into 4 prints, so it was less likely to overflow and even if the filename was truncated you'd still see the line number.

#8255 does the same job by passing through to mbed_error, which also breaks up the prints.

Still, I can see there may be a desire for bigger buffer. Sadly, I can't see any way to get the "tail" of a printf, so can't break it up in to bits - you need a buffer to hold the entire formatted string.

Alternatively, how about avoiding the need for a big buffer by having mbed_error_puts instead, and using that for potentially-long things like the filename?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was trying to implement and use mbed_error_puts, but I realise now that #8255 does impose its own very heavy truncation. It munges the full filename into its "context" structure, if MBED_CONF_ERROR_FILENAME_CAPTURE_ENABLED is on, then outputs that.

You might only get the first 16 characters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Having a line number and non-garbage error print is definitely a step forward;). Could the filename trimming be done at mbed_assert_internal(), https://github.com/ARMmbed/mbed-os/blob/master/platform/mbed_assert.c#L25?
That mbed_error_puts() would fix the problem completely, without need for trimming at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, mbed_assert_internal could use puts, or trim itself. But I've pulled back from any changes to that to defer to #8255. But as I said, that introduces a pile of new problems - or at least applies its existing problems to mbed_assert.

int size = vsnprintf(buffer, sizeof buffer, format, arg);
if (size >= sizeof buffer) {
/* Output was truncated - indicate by overwriting last 4 bytes of buffer
* with ellipsis and newline.
* (Note that although vsnprintf always leaves a NUL terminator, we
* don't need a terminator and can use the entire buffer)
*/
memcpy(&buffer[sizeof buffer - 4], "...\n", 4);
size = sizeof buffer;
}
#if MBED_CONF_PLATFORM_STDIO_CONVERT_NEWLINES || MBED_CONF_PLATFORM_STDIO_CONVERT_TTY_NEWLINES
char stdio_out_prev = '\0';
for (int i = 0; i < size; i++) {
if (buffer[i] == '\n' && stdio_out_prev != '\r') {
const char cr = '\r';
write(STDERR_FILENO, &cr, 1);
}
write(STDERR_FILENO, &buffer[i], 1);
stdio_out_prev = buffer[i];
}
#else
for (int i = 0; i < size; i++) {
serial_putc(&stdio_uart, buffer[i]);
}
write(STDERR_FILENO, buffer, size);
#endif
}
core_util_critical_section_exit();
#endif
}

void mbed_error_vfprintf(const char *format, va_list arg)
{
mbed_error_vprintf(format, arg);
}
2 changes: 1 addition & 1 deletion platform/mbed_error.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ WEAK void error(const char *format, ...)
#ifndef NDEBUG
va_list arg;
va_start(arg, format);
mbed_error_vfprintf(format, arg);
mbed_error_vprintf(format, arg);
va_end(arg);
#endif
exit(1);
Expand Down
2 changes: 1 addition & 1 deletion platform/mbed_error.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ extern "C" {
#else //MBED_CONF_PLATFORM_MAX_ERROR_FILENAME_LEN
#if MBED_CONF_PLATFORM_MAX_ERROR_FILENAME_LEN > 64
//We have to limit this to 64 bytes since we use mbed_error_printf for error reporting
//and mbed_error_vfprintf uses 128bytes internal buffer which may not be sufficient for anything
//and mbed_error_vprintf uses 128bytes internal buffer which may not be sufficient for anything
//longer that 64 bytes with the current implementation.
#error "Unsupported error filename buffer length detected, max supported length is 64 chars. Please change MBED_CONF_PLATFORM_MAX_ERROR_FILENAME_LEN or max-error-filename-len in configuration."
#endif
Expand Down
7 changes: 7 additions & 0 deletions platform/mbed_interface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

#include <stdarg.h>

#include "mbed_toolchain.h"
#include "device.h"

/* Mbed interface mac address
Expand Down Expand Up @@ -146,9 +147,15 @@ void mbed_error_printf(const char *format, ...);
* @param arg Variable arguments list
*
*/
void mbed_error_vprintf(const char *format, va_list arg);

/** @deprecated Renamed to mbed_error_vprintf to match functionality */
MBED_DEPRECATED_SINCE("mbed-os-5.11",
"Renamed to mbed_error_vprintf to match functionality.")
void mbed_error_vfprintf(const char *format, va_list arg);
/** @}*/


#ifdef __cplusplus
}
#endif
Expand Down
2 changes: 2 additions & 0 deletions platform/mbed_retarget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1250,6 +1250,8 @@ extern "C" void exit(int return_code)
#if MBED_CONF_PLATFORM_STDIO_FLUSH_AT_EXIT
fflush(stdout);
fflush(stderr);
fsync(STDOUT_FILENO);
fsync(STDERR_FILENO);
#endif
#endif

Expand Down
1 change: 0 additions & 1 deletion rtos/TARGET_CORTEX/TARGET_CORTEX_M/mbed_rtx_fault_handler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
#include "device.h"
#include "platform/mbed_error.h"
#include "platform/mbed_interface.h"
#include "hal/serial_api.h"

#ifndef MBED_FAULT_HANDLER_DISABLED
#include "mbed_rtx_fault_handler.h"
Expand Down