Meaning of timestamps in DMA results appears to have changed in 2024.2.1

[jkuszmaul]

Describe the bug

It appears that the results of doing a tDMAManager::read has changed, where the timestamp that is populated in the last 4 bytes of the results no longer has the same semantics that it did in the 2024.1.1 roborio image/libraries. As such, any code that attempts to access the timestamp, e.g.

allwpilib/hal/src/main/native/athena/DMA.cpp

Lines 740 to 742 in e408f3a

	uint32_t lower_sample =
	dmaSample->readBuffer[dma->captureStore.captureSize - 1];
	dmaSample->timeStamp = HAL_ExpandFPGATime(lower_sample, status);

is now broken.
It is hard to tell exactly what is happening, but we have some code that measures the difference between the rising & falling edges of an incoming pulse, and where previously we would get out timestamps that looked like e.g.

falling edge, rising edge
4004366, 4001866
4009366, 4006866
4014366, 4011866

we now get numbers that look like

falling edge, rising edge
4294074366, 44694252
4294074366, 44709252

Where the falling edge number stays constants (note that these are being interpreted as unsigned 32-bit numbers).

To Reproduce

1. Install a 2024.2.1 roborio image.
2. Attempt to read timestamps from the tDMAManager object; observe that falling edges appear to never change.

Happy to try to come up with some more cohesive reproduction case, but I suspect that someone may just know what the issue is, and I suspect that coming up with a convenient reproduction case may be obnoxious.

Note that the only change we made between working and not was reverting the https://github.com/wpilibsuite/ni-libraries and roborio image to 2024.1.1. I do not see any obvious changes in the allwpilib codebase that would reflect any change in behavior.

• Link to code:

Our use can be found at https://github.com/frc971/971-Robot-Code/blob/master/frc971/wpilib/dma_edge_counting.cc and https://github.com/frc971/971-Robot-Code/blob/master/frc971/wpilib/dma.cc --- I am unsure what users there are within the regular allwpilib codebase.

Expected behavior
I did not expect a change in behavior of the underlying national instruments libraries.

Additional context

I am unsure the preferred location for this bug; if it is not a subtle issue on our end, I expect it may be more properly directed at NI, but given that I'd expect any changes to the NI headers/behavior to be reflected in this repository, I figure there may be fixes needed here as well.

[ThadHouse]

What is likely happening is the v2.0 image fixed an issue where HMB was not passing the high parts of the timestamp. I wonder if the fix to that broke DMA.

Either way, this is likely something in the FPGA, not something that could be solved from our end.

[ThadHouse]

@jkuszmaul actually, this is on our side/your side. The timestamp coming out of DMA is now 64 bits, not 32 bits.

What this means is the accumulator size needs to be increased by 2 instead of 1. And then the timestamp is read from the last 2 bytes of the buffer. Which means you no longer need to expand the FPGA time.

What was happening is N bytes was being placed by DMA, and N-1 bytes were being read. So the values in the buffer were just getting offset. Its not just timestamps that were bad. Everything was garbage.

[jkuszmaul]

Ah, so

allwpilib/hal/src/main/native/athena/DMA.cpp

Line 679 in e408f3a

dma->captureStore.captureSize = accum_size + 1;

(or https://github.com/frc971/971-Robot-Code/blob/master/frc971/wpilib/dma.cc#L293 in our code)

needs to be a +2 and then the

allwpilib/hal/src/main/native/athena/DMA.cpp

Line 741 in e408f3a

dmaSample->readBuffer[dma->captureStore.captureSize - 1];

(https://github.com/frc971/971-Robot-Code/blob/master/frc971/wpilib/dma.cc#L330 on our end) should have a - 2 and use both 4-byte entries.

Do we know which 4-byte word we expect to be the higher half of the timestamp? Experimentation will probably make it obvious.

[ThadHouse]

@jkuszmaul see #6278

[ThadHouse]

Note that I haven't done a 72 minute run yet, so I haven't seen the upper bit rollover to 1. But I'll run that right now to verify.

[jkuszmaul]

Thanks! Should be easy enough for us to make the change on our end as well.