Get start timestamp and add it to header #34

EmilioPeJu · 2023-03-02T10:46:07Z

This requires that the readers wait for the start of the acquisition.

struct timespec doesn't match the timespec we get from the kernel module, on top of that, it could have different sizes in 32-bit and 64-bit architecture, a cast between an intermediate structure was used to workaround this.

This fixes #7

Araneidae

Looks good to me, but we'll need to discuss the buffer changes, I don't understand the significance of the change you've made here to buffer.c.

server/sim_hardware.c

Araneidae · 2023-03-02T12:49:02Z

server/data_server.c

+    if (!ts_captured)
+            release_write_block(data_buffer, 0);


Interesting. If this is needed it's addressing an outstanding bug, so I'd expect the condition should be count == 0, as a missing release_write_block will happen any time the capture end on a timeout. Also, there is an indentation error here!

Maybe end_write is papering over the cracks here and triggering the missing release_write_block, because we haven't seen a problem with this before ... or have we?

Ah, I see from the commit message that this is an outstanding bug. Then maybe there should be a guard in end_write against this, either silently (or with a warning) perform the missing release, or just an assert fail.

Araneidae · 2023-03-02T12:50:24Z

server/buffer.c

+        while (!buffer->released_once && !buffer->shutdown)
+            pwait_deadline(&buffer->mutex, &buffer->signal, &deadline);


Hmmmm. Think we'll need to talk about this, don't understand what this is all about, why it's needed.

This is what is making the reader wait for the start event, otherwise the header will be sent before the start timestamp was captured

Oh I see. Hmm. Not sure I like this.

So we have two timeouts ticking away, one in the kernel interface, one in the buffer, but we're blocking the buffer timeout until the experiment has triggered.

Guess this is the right way.

driver/panda_stream.c

Araneidae

The driver macro can be tidied up a little, but the rest of my comments are mostly me thinking aloud; this is fine.

Araneidae · 2023-03-03T08:10:15Z

driver/panda_stream.c

 #define IRQ_STATUS_DMA_ACTIVE(status)   ((bool) ((status) & 0x80))
 #define IRQ_STATUS_COMPLETION(status)   ((size_t) (((status) >> 1) & 0x0F))
+#define IRQ_START_EVENT(status)         ((bool) (status  & 0x100))


Forgot: we need brackets around (status) for macro safety!

Also for consistency might be better to call this macro IRQ_STATUS_START_EVENT to match the other status register interrogation macros.

Araneidae · 2023-03-03T08:14:08Z

driver/panda_stream.c

@@ -282,6 +290,8 @@ static void start_hardware(struct stream_open *open)
    open->isr_block_index = 0;
    open->read_block_index = 0;
    open->read_offset = 0;
+    // having a zeroed timestamp means the start event did not happen
+    open->start_ts = (const struct timespec64){0};


Yes, this makes sense, and ensures that each time we arm the timestamp is reset.

Araneidae · 2023-03-03T08:45:06Z

server/data_server.c

+    if (!ts_captured)
+        release_write_block(data_buffer, 0);


Hmm. On the face of it the condition !ts_captured should be replaced with count == 0, as each call to get_write_block ought to be balanced by a call to release_write_block, and this call will be omitted any time the loop above exits with count equal to 0.

On the other hand, get_write_block doesn't actually have any side effects, and calling release_write_block will trigger an empty update to the reader. Still, I think I'd prefer a form that looks correct locally.

Araneidae · 2023-03-03T08:46:11Z

server/hardware.c

+// 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 {


Is it possible to switch on the architecture version and use that version if it exists? This is quite a nasty hack...!

Araneidae · 2023-03-03T08:52:48Z

server/buffer.c

+        while (!buffer->released_once && !buffer->shutdown)
+            pwait_deadline(&buffer->mutex, &buffer->signal, &deadline);


Oh I see. Hmm. Not sure I like this.

So we have two timeouts ticking away, one in the kernel interface, one in the buffer, but we're blocking the buffer timeout until the experiment has triggered.

Guess this is the right way.

EmilioPeJu · 2023-03-03T09:20:14Z

It is worth mentioning that this change comes together with PandABlocks/PandABlocks-FPGA#110

This bug was already present before, but because PCAP timeouts happens every 100ms, it was hard to reproduce, now that a buffer reader waits for the start of the capture, it is easier to get this bug.

EmilioPeJu mentioned this pull request Mar 2, 2023

Add PCAP interrupt on PCAP.ENABLE high when armed and enabled PandABlocks/PandABlocks-FPGA#110

Merged

EmilioPeJu requested a review from Araneidae March 2, 2023 11:54

Araneidae reviewed Mar 2, 2023

View reviewed changes

EmilioPeJu force-pushed the ts-cap-interrupt branch 4 times, most recently from f453437 to f6fb2ad Compare March 2, 2023 19:40

Araneidae approved these changes Mar 3, 2023

View reviewed changes

EmilioPeJu force-pushed the ts-cap-interrupt branch from f6fb2ad to 9a54961 Compare March 3, 2023 09:18

EmilioPeJu force-pushed the ts-cap-interrupt branch from 9a54961 to 0e02a37 Compare March 3, 2023 09:22

EmilioPeJu added 4 commits March 3, 2023 11:10

Capture timestamp when PCAP becomes armed and enabled

a77c071

Increase driver compatibility version

e008182

Fix buffer not working after arming and disarming PCAP quickly

122f2fb

This bug was already present before, but because PCAP timeouts happens every 100ms, it was hard to reproduce, now that a buffer reader waits for the start of the capture, it is easier to get this bug.

driver: remove verbose parameter (using dynamic debug instead)

a68304d

EmilioPeJu force-pushed the ts-cap-interrupt branch from 0e02a37 to a68304d Compare March 3, 2023 11:11

EmilioPeJu merged commit ab47acb into master Mar 3, 2023

EmilioPeJu deleted the ts-cap-interrupt branch March 3, 2023 12:11

coretl mentioned this pull request Nov 7, 2023

Add start_time to docs #43

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get start timestamp and add it to header #34

Get start timestamp and add it to header #34

EmilioPeJu commented Mar 2, 2023

Araneidae left a comment

Araneidae Mar 2, 2023

Araneidae Mar 2, 2023

Araneidae Mar 2, 2023

EmilioPeJu Mar 2, 2023

Araneidae Mar 3, 2023

Araneidae left a comment

Araneidae Mar 3, 2023

Araneidae Mar 3, 2023

Araneidae Mar 3, 2023

Araneidae Mar 3, 2023

Araneidae Mar 3, 2023

Araneidae Mar 3, 2023

EmilioPeJu commented Mar 3, 2023

		while (!buffer->released_once && !buffer->shutdown)
		pwait_deadline(&buffer->mutex, &buffer->signal, &deadline);

Get start timestamp and add it to header #34

Get start timestamp and add it to header #34

Conversation

EmilioPeJu commented Mar 2, 2023

Araneidae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Araneidae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmilioPeJu commented Mar 3, 2023