From 1ffcc0a1803a38a22fb948ba32b7b75debe558a7 Mon Sep 17 00:00:00 2001 From: Michael Abbott Date: Mon, 6 Mar 2023 11:35:36 +0000 Subject: [PATCH] Revisit data buffer handling Fix issue with change to wait_for_buffer_ready() which could end up looping while waiting for experiment data. Also simplify the block handling in capture_experiment(). --- driver/panda_device.h | 5 ++++- server/buffer.c | 14 ++++++-------- server/data_server.c | 22 ++++++++++++---------- server/hardware.c | 12 ++++++------ 4 files changed, 28 insertions(+), 25 deletions(-) diff --git a/driver/panda_device.h b/driver/panda_device.h index 3c61bab..b4cb608 100644 --- a/driver/panda_device.h +++ b/driver/panda_device.h @@ -31,4 +31,7 @@ struct panda_block { #define PANDA_COMPLETION_DMA 4 #define PANDA_COMPLETION_OVERRUN 8 -#define PANDA_GET_START_TS _IOR('P', 4, struct timespec64) +/* Returns timestamp associated with the start of capture, created on rising + * edge of capture enable. To ensure a non-zero timestamp this should not be + * called until data has been returned from the data stream. */ +#define PANDA_GET_START_TS _IOR('P', 4, struct timespec64) diff --git a/server/buffer.c b/server/buffer.c index 958db21..2f2c0e4 100644 --- a/server/buffer.c +++ b/server/buffer.c @@ -25,11 +25,11 @@ struct capture_buffer { bool released_once; // Used to wait for first data void *buffer; // Base of captured data buffer + /* Capture and buffer cycle counting are used to manage connections without * having to keep track of clients. If the client and buffer capture_cycle * don't agree then the client has been reset, and the buffer_cycle is used * to check whether the client's buffer has been overwritten. */ - unsigned int capture_cycle; // Counts data capture cycles unsigned int buffer_cycle; // Counts buffer cycles, for overrun detection @@ -303,17 +303,15 @@ static bool wait_for_buffer_ready( * with or until the deadline expires. */ while (true) { - while (!buffer->released_once && !buffer->shutdown) - pwait_deadline(&buffer->mutex, &buffer->signal, &deadline); - if (buffer->shutdown) /* Shutdown forced. */ return false; - if (buffer->state != STATE_IDLE && + else if (buffer->state != STATE_IDLE && buffer->capture_cycle == reader->capture_cycle) - /* New capture cycle ready for us. */ - return true; - if (!pwait_deadline(&buffer->mutex, &buffer->signal, &deadline)) + /* New capture cycle ready for us, but only return success once + * we've started to receive data. */ + return buffer->released_once; + else if (!pwait_deadline(&buffer->mutex, &buffer->signal, &deadline)) /* Timeout detected. */ return false; } diff --git a/server/data_server.c b/server/data_server.c index f1b1d39..8e00644 100644 --- a/server/data_server.c +++ b/server/data_server.c @@ -129,23 +129,25 @@ static void capture_experiment(void) do count = hw_read_streamed_data(block, DATA_BLOCK_SIZE, &at_eof); while (data_thread_running && count == 0 && !at_eof); - if (count > 0) + + /* Do our best to capture a timestamp before releasing the first block + * of the experiment. If the experiment is empty this return an empty + * timestamp, that's how it goes. */ + if (!ts_captured) { - if (!ts_captured) - { - hw_get_start_ts(&pcap_start_ts); - ts_captured = true; - } - release_write_block(data_buffer, count); + hw_get_start_ts(&pcap_start_ts); + ts_captured = true; } + /* Unconditionally release the write block here. Either we have data to + * send (exited loop above with count==0), or we're at the end of the + * experiment, in which case an empty block is harmless. */ + release_write_block(data_buffer, count); + total_bytes += count; experiment_sample_count = total_bytes / sample_length; } - if (!ts_captured) - release_write_block(data_buffer, 0); - completion_code = hw_read_streamed_completion(); end_write(data_buffer); diff --git a/server/hardware.c b/server/hardware.c index e135c66..0b7c1f6 100644 --- a/server/hardware.c +++ b/server/hardware.c @@ -45,15 +45,15 @@ struct register_fields { 32 - BLOCK_REGISTER_BITS - BLOCK_INSTANCE_BITS - BLOCK_TYPE_BITS; }; -// This has to be bigger or same size than the linux kernel structure with the -// same name. -// We are using this to convert to a stardard timespec in a way that we are -// compatible with 32-bit and 64-bit architectures, however, we will not -// need it when we update to a newer glibc (which contains __timespec64) +/* This has to be bigger or same size than the linux kernel structure with the + * same name. + * We are using this to convert to a stardard timespec in a way that we are + * compatible with 32-bit and 64-bit architectures, however, we will not + * need it when we update to a newer glibc (which contains __timespec64) */ struct timespec64 { __time64_t tv_sec; /* Seconds */ uint32_t tv_nsec; /* Nanoseconds */ - uint32_t :32; /* padding */ + uint32_t : 32; /* padding */ };