Sinclair Spectrum TAP file dumper

Question

The other day, I had occasion to want to look into an ancient piece of Z80 software written for a Sinclair Spectrum computer. Software for this machine was typically saved to audio tape, and today people use the TAP format to store and exchange Spectrum software. The format is a very literal translation of the original tape format and consists of a sequence of blocks (here rendered as TAPBlock class). Typical format is that there is a header (TAPHeader in this code) followed by a code block. An example is the Wizard's Lair game from 1984.

The purpose

The purpose of this software is to simply read the file and dump the contents into somewhat structured, human readable form. I was mostly interested in looking at strings of hex bytes (that's what passes for "human readable" in my house!) so that's the format I used here. Everything works as intended. Partial sample output is shown below for the game file mentioned above.

{ len = 19, flag = 0, {
        00 77 6c 20 20 20 20 20 20 20 20 c7 3e 02 00 c7 
        3e 
}, cksum = 25, calcsum = 25 }
{ type = 0, filename = "wl        ", data_len = 16071, param1 = 2, param2 = 16071 }
{ len = 16073, flag = 255, {
        00 01 0a 00 ec 31 30 0e 00 00 0a 00 00 0d 00 02 
        0d 00 fd 34 39 39 39 39 0e 00 00 4f c3 00 0d 00 
        ...
        33 36 37 36 0e 00 00 7c 5c 00 2c 31 39 39 0e 00 
        00 c7 00 00 3a fe 0d 
}, cksum = 122, calcsum = 122 }
{ len = 19, flag = 0, {
        03 77 6c 7a 20 20 20 20 20 20 20 38 07 50 c3 00 
        80 
}, cksum = 110, calcsum = 110 }
{ type = 3, filename = "wlz       ", data_len = 1848, param1 = 50000, param2 = 32768 }
{ len = 1850, flag = 255, {
        00 00 00 00 18 3c 7e 7e 7e 5a 7e 3c 24 3c 3c 18 
        00 03 07 01 06 06 0f 07 e7 f7 ff ff ff 9f 0f 88 
        ...
        00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
        00 00 00 00 00 00 00 00 
}, cksum = 209, calcsum = 209 }
{ len = 19, flag = 0, {
        03 77 6c 62 20 20 20 20 20 20 20 6e 02 60 ea c3 
        80 
}, cksum = 255, calcsum = 255 }
{ type = 3, filename = "wlb       ", data_len = 622, param1 = 60000, param2 = 32963 }
{ len = 624, flag = 255, {
        c0 38 00 3d 0e 72 0e 71 1c 43 1c 48 2f c0 43 1c 
        43 0e c0 72 0e 72 08 79 08 77 21 71 21 71 1c c0 
        ...
        bf 94 c0 bf 8f b8 8d b0 8f a9 8e c0 ad 8a bf 89 
        ff 50 35 0a 5a 38 64 5a a8 7a 5a c8 0c ff 
}, cksum = 170, calcsum = 170 }
{ len = 19, flag = 0, {
        03 77 6c 62 62 20 20 20 20 20 20 77 01 00 fa 80 
        80 
}, cksum = 148, calcsum = 148 }
{ type = 3, filename = "wlbb      ", data_len = 375, param1 = 64000, param2 = 32896 }
{ len = 377, flag = 255, {
        d9 e1 d9 c9 d9 e5 d9 21 34 eb 7e fe ff 28 f1 fe 
        c0 20 0b 23 7e 32 c9 fa 23 7e 32 ca fa 23 7e 32 
        ...
        44 42 00 00 3c 40 3c 02 42 3c 00 00 fe 10 10 10 
        10 10 2a b2 5c ed 5b 
}, cksum = 176, calcsum = 176 }

My questions

I didn't much like the way I have created the TAPHeader constructor. I considered doing something even more ugly with static_cast<TAPHeader>(data.data()) but the less said about that the better. Is there a better way to do this? Any other comments on ways to improve the code are also welcome. I'm using C++17 for this, but would not be averse to C++20 if that provides some compelling feature that would be useful here.

tapdump.cpp

#include <iostream>
#include <iterator>
#include <iomanip>
#include <fstream>
#include <functional>
#include <numeric>
#include <cstdint>
#include <vector>
#include <optional>

struct TAPHeader {
    uint8_t type;
    uint8_t filename[11];  // actually 10 bytes, but we add NUL terminator
    uint16_t data_len;
    uint16_t param1;
    uint16_t param2;

    explicit TAPHeader(const std::vector<uint8_t> v) {
        auto it = v.begin();
        type = *it++;
        for (int i=0; i<10; ++i) {
            filename[i] = *it++;
        }
        filename[10] = '\0';  // terminate filename string
        data_len = *it++;
        data_len |= *it++ << 8;
        param1 = *it++;
        param1 |= *it++ << 8;
        param2 = *it++;
        param2 |= *it++ << 8;
    }

    friend std::ostream& operator<<(std::ostream& out, const TAPHeader& hdr) {
        return out 
            << "{ type = " << (unsigned)hdr.type
            << ", filename = \"" << hdr.filename
            << "\", data_len = " << hdr.data_len
            << ", param1 = " << hdr.param1
            << ", param2 = " << hdr.param2
            << " }";
    }
};

class TAPBlock {
    uint16_t len;   // little-endian
    uint8_t flag;  // 0 = header, 0xff = body        
    uint8_t cksum; // simple xor of data, excluding len
    std::vector<uint8_t> data;
public:
    uint8_t calcsum() const {
        return std::accumulate(data.begin(), data.end(), flag, std::bit_xor<uint8_t>());
    }
    bool is_ok() const { return calcsum() == cksum; }

    std::optional<TAPHeader> get_header() const {
        if (flag == 0 && data.size() >= 17) {
            return TAPHeader{data};
        }
        return {};
    }


    bool read(std::istream& in) {
        uint8_t lo, hi;
        lo = in.get();
        hi = in.get();
        len = (hi << 8) | lo;
        flag = in.get();
        data.clear();
        data.reserve(len-2);
        for (auto count{len-2}; count; --count) {
            data.push_back(in.get());
        }
        cksum = in.get();
        return in.good();
    }

    friend std::ostream& operator<<(std::ostream& out, const TAPBlock blk) {
        out << "{ len = " << std::dec << blk.len << ", flag = " << (unsigned)blk.flag << ", {";
        if (1 || blk.flag) {
            int remaining{0};
            for (auto n : blk.data) {
                if (remaining == 0) {
                    remaining = 16;
                    out << "\n\t";
                }
                --remaining;
                out << std::setw(2) << std::setfill('0') << std::hex << (unsigned)n << ' ';
            }
            out << "\n}, ";
        } else {

        }
        return out << "cksum = " << std::dec << (unsigned)blk.cksum
            << ", calcsum = " << std::dec << (unsigned)blk.calcsum() << " }";
    }
};

int main(int argc, char *argv[])
{
    if (argc != 2) {
        std::cout << "Usage: dumptap tapfilename\n";
        return 1;
    }
    TAPBlock blk;
    std::ifstream in{argv[1]};
    while (blk.read(in)) {
        std::cout << blk << '\n';
        if (auto hdr = blk.get_header()) {
            std::cout << *hdr << '\n';
        }
    }
}

Answer 1

• Although it makes the code a bit longer, I think I'd start by writing a little "buffer" class, on this general order:

class buffer { 
    std::vector<uint8_t> &data;
    std::vector<uint8_t>::iterator pos;
public:
    buffer(std::vector<uint8_t> &data) 
        : data(data)
        , pos(data.cbegin()) 
    {}

    void read(char *dest, size_t len) {
        std::copy_n(pos, dest, len);        
        pos += len;
    }

    void read(uint16_t &dest) {
        dest = *pos++;
        dest |= *pos++ << 8;
    }
};

Using this, the ctor for TAPHeader comes out something like this:

    explicit TAPHeader(std::vector<uint8_t> const &data, TAPHeader &hdr) {
        buffer b(data);
        b.read(&type, sizeof(type));
        b.read(filename, sizeof(filename)-1);
        filename[10] = '\0';
        b.read(data_len);
        b.read(param1);
        b.read(param2);
    }

I find that enough cleaner and simpler to justify the code for buffer.

• For the flag member of TAPBlock, I'd probably do something like this:

enum Flag : uint8_t { header = 0, body = 0xff } flag;

Then get_header could come out something like this:

    std::optional<TAPHeader> get_header() const {
        const int min_header_size = 17;

        if (flag == header && data.size() >= min_header_size) {
            return TAPHeader{data};
        }
        return {};
    }

At least to me, this expresses the intent a bit more clearly.

• Right now, you print out the raw checksums (as read, and as computed) and leave it to the reader to verify that they match. Personally, I'd prefer to have the computer do that check (and consider omitting the message entirely if they match, as well as adding ANSI code to print it in bright red if they mismatch).

Answer 2

The parameters to the TAPHeader constructor and TAPBlock::operator<< could be passed as const references to avoid making copies of the objects being passed.

explicit TAPHeader(const std::vector<uint8_t> &v);
// ...
friend std::ostream& operator<<(std::ostream& out, const TAPBlock &blk);

In the TAPHeader constructor, you calculate the two byte length values as you read the bytes for it, but in TAPBlock::read, you read two bytes into local variables then use those to calculate the length. For consistency you should use the same style for both. read can be changed to start:

len = in.get();
len |= in.get() << 8;

then you wouldn't need the lo or hi variables.

Answer 3

Some comments to consider:

• In TAPBlock::read, I wouldn't try to get cute and declare the two variables lo and hi on the same line. Instead, I would do const uint8_t lo = in.get(); and then const uint8_t hi = in.get(); on the next line. (So just pay attention to const correctness).

• Also, TAPBlock::operator<< looks a little strange with its if-else. First, did you really mean if (1 || blk.flag) { ... }? That is, taking a logical OR with constant 1 seems unnecessary. Here also the else-branch does nothing and is useless. Perhaps you'd like to restructure this as something like if(!blk.flag) { return out ... } after which write no explicit else but just do what you need to do.

• If we wanted to, we could maybe write a small helper for the constructor of TAPHeader as the two lines that operate on data_len, param1 and param2 do the same thing (but maybe that will only clutter things, so it's OK). By the way, we could also write auto it = v.cbegin(); but that's just personal preference, I do see that v is passed in as const.

• We could add a precondition (e.g., assert(data.size() == 17 && "Unexpected input length")) to TAPHeader constructor and not just assume it is always dereferenceable. At the same time, why not make a constexpr constant for 17 so that it's not so magical as we use it at least twice in the code.

• Arguably, the loop for (auto count{ len - 2 }; count; --count) { ... } is not written as readable as it could be. Isn't it clearer to write for (auto count{0}; count < len - 2; ++count) { ... }. In fact, I suppose you could also just do a call to something like std::generate_n with a lambda that return in.get() to populate the data vector but that might be overkill and actually just hurt readability.