diff --git a/libraries/DataFlash/DataFlash_File.cpp b/libraries/DataFlash/DataFlash_File.cpp index 58b0ab56c1..f82a877843 100644 --- a/libraries/DataFlash/DataFlash_File.cpp +++ b/libraries/DataFlash/DataFlash_File.cpp @@ -96,6 +96,11 @@ void DataFlash_File::Init() AP_HAL::panic("Failed to create DataFlash_File semaphore"); return; } + write_fd_semaphore = hal.util->new_semaphore(); + if (write_fd_semaphore == nullptr) { + AP_HAL::panic("Failed to create DataFlash_File write_fd_semaphore"); + return; + } #if CONFIG_HAL_BOARD == HAL_BOARD_PX4 || CONFIG_HAL_BOARD == HAL_BOARD_VRBRAIN // try to cope with an existing lowercase log directory @@ -195,6 +200,9 @@ void DataFlash_File::periodic_1Hz(const uint32_t now) // If you try to close the file here then it will almost // certainly block. Since this is the main thread, this is // likely to cause a crash. + + // semaphore_write_fd not taken here as if the io thread is + // dead it may not release lock... _write_fd = -1; _initialised = false; } @@ -826,12 +834,19 @@ uint16_t DataFlash_File::get_num_logs() */ void DataFlash_File::stop_logging(void) { + // best-case effort to avoid annoying the IO thread + const bool have_sem = write_fd_semaphore->take(1); if (_write_fd != -1) { int fd = _write_fd; _write_fd = -1; log_write_started = false; ::close(fd); } + if (have_sem) { + write_fd_semaphore->give(); + } else { + _internal_errors++; + } } @@ -873,12 +888,17 @@ uint16_t DataFlash_File::start_new_log(void) if (fname == nullptr) { return 0xFFFF; } + if (!write_fd_semaphore->take(1)) { + _open_error = true; + return 0xFFFF; + } _write_fd = ::open(fname, O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0666); _cached_oldest_log = 0; if (_write_fd == -1) { _initialised = false; _open_error = true; + write_fd_semaphore->give(); int saved_errno = errno; ::printf("Log open fail for %s - %s\n", fname, strerror(saved_errno)); @@ -891,6 +911,7 @@ uint16_t DataFlash_File::start_new_log(void) _write_offset = 0; _writebuf.clear(); log_write_started = true; + write_fd_semaphore->give(); // now update lastlog.txt with the new log number fname = _lastlog_file_name(); @@ -1078,8 +1099,13 @@ void DataFlash_File::flush(void) _io_timer(); } hal.scheduler->resume_timer_procs(); - if (_write_fd != -1) { - ::fsync(_write_fd); + if (write_fd_semaphore->take(1)) { + if (_write_fd != -1) { + ::fsync(_write_fd); + } + write_fd_semaphore->give(); + } else { + _internal_errors++; } } #endif @@ -1132,6 +1158,13 @@ void DataFlash_File::_io_timer(void) } } + if (!write_fd_semaphore->take(1)) { + return; + } + if (_write_fd == -1) { + write_fd_semaphore->give(); + return; + } ssize_t nwritten = ::write(_write_fd, head, nbytes); if (nwritten <= 0) { hal.util->perf_count(_perf_errors); @@ -1151,6 +1184,7 @@ void DataFlash_File::_io_timer(void) ::fsync(_write_fd); #endif } + write_fd_semaphore->give(); hal.util->perf_end(_perf_write); } diff --git a/libraries/DataFlash/DataFlash_File.h b/libraries/DataFlash/DataFlash_File.h index 13a3879564..2e08259d5b 100644 --- a/libraries/DataFlash/DataFlash_File.h +++ b/libraries/DataFlash/DataFlash_File.h @@ -161,7 +161,12 @@ private: const uint32_t _free_space_check_interval = 1000UL; // milliseconds const uint32_t _free_space_min_avail = 8388608; // bytes + // semaphore mediates access to the ringbuffer AP_HAL::Semaphore *semaphore; + // write_fd_semaphore mediates access to write_fd so the frontend + // can open/close files without causing the backend to write to a + // bad fd + AP_HAL::Semaphore *write_fd_semaphore; // performance counters AP_HAL::Util::perf_counter_t _perf_write;