search

cacao-org / cacao

Compute And Control for Adaptive Optics
GNU General Public License v3.0
24 stars 9 forks source link

added atime update to WFScamsim #28

Closed jaredmales closed 1 year ago

DasVinch commented 1 year ago

Following to your corresponding PR regarding the writetime in ImageStreamIO...

Shouldn't this bookkeeping be part of ImageStreamIO?

I think it should be either the outcome of a successful semwait, or an ImageStreamIO-encapsulated memcopy, that would take care of updating the atime/readtime (possibly one atime per semaphore?)

oguyon commented 1 year ago

I agree this should part of ISIO

DasVinch commented 1 year ago

Actually brings back an interesting idea, related to efficient use of circular buffers in ImageStreamIO. It could be defined that upon a successful semwait/semtrywait etc... the ImageStreamIO function returns the pointer to the next good data. This allows having a strong interface to circular buffers (past memory), while avoiding doublecopies with the main buffer (img.array). And it restores the feature that if a reader is late, it may still process data without loss, as long as it is still in the ring buffer.

Downside: must rewrite every single shmim read call in cacao and milk to move from raw memcopies to some ImageStreamIO_getdata calls.

jaredmales commented 1 year ago

According to ImageStruct.h comments, atime is the "acquisition time", not the "access time" (which would be md->lastaccesstime). And I have always used it as such. My thinking is that the acquisition time (atime) is the responsibility of the hardware. E.g. I use that to record the time stamp from the PCIe card for when the frame showed up, or from the camera itself if provided. So this could be passed as a parameter to UpdateIm (say), but it is something ISIO doesn't know about by itself.

DasVinch commented 1 year ago

I thought it was the accesstime, so take all of the above as if it were.

Yes, then I understand your point for the acquisition time. So... modify most of CACAO to always pass is down...? I like the updateIm idea.

We can merge this PR nonetheless as a hotfix if it fixes a problem for you. At SCExAO, I actually ended using a keyword for acquisition time, thus duplicating the feature.

jaredmales commented 1 year ago

It's not a critical problem, but rtimv uses atime (and cnt0) to estimate framerate and display it. So this makes the simulator look nicer when using rtimv. I might switch to writetime anyway since that should be more universal.

However, what if we instead add an ImageStreamIO_UpdateIM_atime() or something to provide this ability but not change the existing API?