Coordinating camera stage movement and data readout

Question

The purpose of this code is important, so here's a quick rundown.

This code runs controls a physical machine which utilizes a camera and a moving stage holding an object to be imaged. Imaging can be broken down into two parts; exposing (collecting photons) and readout (reading the formatted image from the camera over USB).

Stage moves are performed asynchronously. The object being imaged is broken up into many (700+) "tiles" which must be imaged separately, so one move + expose + readout for each tile.

An optimization which saves ~3 minutes per imaging operation is to begin moving to tile N+1 after exposing tile N but before readout has completed. We then wait for readout to complete, expose the current tile, and repeat the process again.

This leads to a loop structure that, while correct, is a bit confusing. It's not a horrible piece of code, but it's been bugging me and I'd like to spend some time today refactoring it. Problem is, I haven't come up with any great ideas.

Here's the loop structure:

void ScanLoopExample()
{
    var tiles = GetAllTiles();
    var firstTile = tiles.First();

    // get into position for the first tile
    MoveStageAsync(firstTile.X, firstTile.Y, firstTile.Z);

    foreach(var nextTile in tiles.Skip(1))
    {
        // wait for the move to complete
        WaitForMotionEnd();

        // expose over the first tile
        ExposeTile();

        // get into position for the next tile
        // we *must* make this move right here in 
        // order to save time moving while the camera is busy
        MoveStageAsync(nextTile.X, nextTile.Y, nextTile.Z);

        // wait for the image to arrive from the camera,
        // we cannot expose again until this completes
        WaitForReadout();
    }

    // because we only moved to the last tile
    // without capturing, capture it here
    WaitForMotionEnd();
    ExposeTile();
    WaitForReadout();
}

Like I said, not the worst code ever, but there are a couple of things I don't like:

1. I don't like that part of the logic is separated from the loop itself (the move to the first tile).

2. I don't like that the foreach variable is the tile to move to, not the tile being captured. It has slightly confused two coworkers I showed it to (though they got it quick enough after I explained the process).

3. I don't like that the capture code (wait + expose + readout) must be repeated after the loop is finished.

I've tried a couple of variants, but nothing I have come up with can get all of the capture logic in the body of the loop. This is a bit of an obsessive need to make the code as pretty as possible, but hey, it's the weekend and I'm not on the clock.

The tricky bit is that we must begin the asynchronous move of the stage after exposure but before readout. That places a strict rule on the ordering of operations in the loop.

Answer 1

Since what matters here is the ordering of the operations, not what the operations mean, I'm going to use these abbreviations:

A - MoveStageAsync
B - WaitForMotionEnd
C - ExposeTile
D - WaitForReadout

(I'm trying to explain how I got my version using these abbreviations, if you find them confusing, skip to the code.)

So with 3 iterations, your code is doing this: ABCADBCADBCD, and the naive, slow version you optimized from did this: ABCDABCDABCD. If we wrap parentheses around the part that is in the loop body, we can abbreviate what your code is doing as A(BCAD)BCD and the unoptimized version as (ABCD). Assuming that we have to keep the same order as what your code is currently doing, we can shift what's in the body of the loop slightly to get AB(CADB)CD and ABC(ADBC)D. There isn't a way to shift the loop all the way to the beginning or end and do things in the same order you're currently doing them in.

If we try to find an ordering that lets us just toss the whole thing in a loop (like (ABCD), only efficient), we have to move to the first location first (nothing else makes sense), we have to wait for the motion to end next (nothing else makes sense, again), then we have to expose before we can wait for the exposure. So the only way to do it with everything in the loop was the original, slow version.

So we have 3 possibilities: A(BCAD)BCD, AB(CADB)CD, and ABC(ADBC)D. If we look at what those 3 would mean, I think AB(CADB)CD is the most readable.

Here it is as code:

MoveStageAsync(firstTile.X, firstTile.Y, firstTile.Z);
WaitForMotionEnd();

foreach(var nextTile in tiles.Skip(1))
{
    ExposeTile();

    MoveStageAsync(nextTile.X, nextTile.Y, nextTile.Z);

    WaitForReadout();
    WaitForMotionEnd();
}

ExposeTile();
WaitForReadout();

There are a few nice things about it: the setup and teardown around the loop both make sense as standalone actions, the waits in the loop body are both next to each other, and the loop body as a whole makes sense as a standalone action. Although the loop index is a tile, it's really a loop over the transitions from one tile to another.

Answer 2

First, let's revert to the slow version:

void ScanLoopSlow()
{
    var tiles = GetAllTiles();
    foreach (var tile in tiles)
    {
        // get into position for the first tile
        MoveStageAsync(tile.X, tile.Y, tile.Z);

        // wait for the move to complete
        WaitForMotionEnd();

        // expose over the first tile
        ExposeTile();

        // wait for the image to arrive from the camera,
        // we cannot expose again until this completes
        WaitForReadout();
    }
}

The limitation that you would like to enforce is that the camera must not expose another tile until the previous tile's data has been transferred. Therefore, it might be a good idea to emphasize that need for that lockout in the ExposeTile() function itself.

void WaitForReadout(Tile prevTile)
{
    if (prevTile == null)
    {
        return;
    }
    // Wait...
    ...
}

Tile ExposeTile(Tile tile, Tile prevTile)
{
    // wait for the image to arrive from the camera,
    // we cannot expose again until this completes
    WaitForReadout(prevTile);

    // Do exposure stuff
    ...

    return tile;
}

void ScanLoopWithLockingExposeTile()
{
    var tiles = GetAllTiles();
    Tile exposedTile = null;

    foreach (var tile in tiles)
    {
        // get into position for the first tile
        MoveStageAsync(tile.X, tile.Y, tile.Z);

        // wait for the move to complete
        WaitForMotionEnd();

        // expose over the first tile
        exposedTile = ExposeTile(tile, exposedTile);
    }
    WaitForReadout(exposedTile);
}

Alternatively, you could leave the helper functions ExposeTile() and WaitForReadout() unmodified. In my opinion, this look-behind solution is better than your original look-ahead code. (For example, if the list of tiles is empty, your original code would barf.)

void ScanLoopWithoutLockingExposeTile()
{
    var tiles = GetAllTiles();
    Tile exposedTile = null;

    foreach (var tile in tiles)
    {
        // get into position for the first tile
        MoveStageAsync(tile.X, tile.Y, tile.Z);

        // wait for the move to complete
        WaitForMotionEnd();


        // wait for the previous image to arrive from the camera.
        // we cannot expose again until this completes
        if (exposedTile != null)
        {
            WaitForReadout();
        }

        // expose over the first tile
        ExposeTile();

        exposedTile = tile;
    }
    if (exposedTile != null)
    {
        WaitForReadout();
    }
}